We have a ton of per-file rotation functions, often with generic names
that do not tell you whether they are uint32_t vs uint64_t, or rotl vs
rotr.
Additionally, (x >> r) | (x << (32 - r)) is UB at r = 0.
(x >> r) | (x << ((-r) & 31)) works for 0 <= r < 32, which is what
cast.c does. GCC and Clang recognize this pattern as a rotate, but MSVC
doesn't. MSVC does, however, provide functions for this.
We usually rotate by a non-zero constant, which makes this moot, but
rotation comes up often enough that it's worth extracting out. Some
particular changes to call out:
- I've switched sha256.c from rotl to rotr. There was a comment
explaining why it differed from the specification. Now that we have
both functions, it's simpler to just match the specification.
- I've dropped all the inline assembly from sha512.c. Compilers should
be able to recognize rotations in 2021.
Change-Id: Ia1030e8bfe94dad92514ed1c28777447c48b82f9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49765
Reviewed-by: Adam Langley <agl@google.com>
Node seems uncommonly sensitive to this, so let's write these functions
in a way that stays in sync and test this. See also
https://boringssl-review.googlesource.com/c/boringssl/+/49585
This does incur a cost across all BoringSSL consumers that use these
functions: as a result of Node indiscriminately exposing every cipher,
we end up pulling more and more ciphers into these getters. But that
ship sailed long ago, so, instead, document that EVP_get_cipherby*
should not be used by size-conscious callers.
EVP_get_digestby* probably should have the same warning, but I've left
it alone for now because we don't quite have the same proliferation of
digests as ciphers. (Though there are things in there, like MD4, that
ought to be better disconnected.)
Change-Id: I61ca406c146279bd05a52bed6c57200d1619c5da
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49625
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
EVP_MD_nid, in OpenSSL, is the same as EVP_MD_type. EVP_MD_type seems to
be the preferred spelling, so put EVP_MD_nid in the deprecated bucket.
Also add an EVP_MD_do_all alias to EVP_MD_do_all_sorted.
Change-Id: I4e7b800902459ac5cb9ef0df65d73da94afdf927
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48365
Reviewed-by: Adam Langley <agl@google.com>
Similar to
https://boringssl-review.googlesource.com/c/boringssl/+/46405,
SHA256_Final and SHA224_Final hit array size warnings in the new GCC.
The array sizes are, strictly speaking, purely decoration, but this is a
good warning so we should be clean with it on.
That same change is difficult to apply to md32_common.h because
md32_common.h generates the functions for us. md32_common.h is already
strange in that it is multiply-included and changes behavior based on
macros defined by the caller.
Instead, replace it with inline functions, which are a bit more
conventional and typesafe. This allows each hash function to define the
function prototype. Use this to add an unsized helper for SHA-256.
Bug: 402
Change-Id: I61bc30fb58c54dd40a55c9b1ebf3fb9adde5e038
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47807
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Peter Foley <pefoley@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We have loads of variations of these. Align them in one set. This avoids
the HOST_* macros defined by md32_common.h, so it'll be a little easier
to make it a more conventional header.
Change-Id: Id47fe7b51a8f961bd87839f8146d8a5aa8027aa6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/46425
Reviewed-by: Adam Langley <agl@google.com>
Electron builds Node.js with BoringSSL. They want to match OpenSSL as
much as possible and thus have a patch[1] that adds AES-256 CFB mode.
However, that patch makes libcrypto depend on libdecrepit, which can't
be done in general. This change lands the AES-256 CFB support in
libdecrepit without the libcrypto bit and, in order for BoringSSL to
remain consistent, without advertising support in
EVP_CIPHER_do_all_sorted. This will let Electron reduce the size of
their patch a bit.
[1] https://github.com/electron/electron/blob/master/patches/boringssl/expose_aes-cfb.patch
Change-Id: If628d22a595b354623439c587542e414e43e4045
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/37264
Reviewed-by: Adam Langley <agl@google.com>
We usually name output parameters 'out'. (Someone made a C++ templating
change in Chromium which messed up const-ness, saw the compile error,
and thought it was in MD5_Final.) Also tag the parameters with the
sizes.
Sadly, there's a bit of goofiness around SHA224_Final/SHA256_Final and
SHA384_Final/SHA512_Final, but they're just documentation anyway.
(Though it does touch on the mess that is sha->md_len which would be
nice to clear through somehow.)
Change-Id: I1918b7eecfe13f13b217d01d4414ac2358802354
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/35484
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Independent of the underlying CPU architecture, casting unaligned
pointers to uint64_t* is undefined. Just use a memcpy. The compiler
should be able to optimize that itself.
Change-Id: I39210871fca3eaf1f4b1d205b2bb0c337116d9cc
Reviewed-on: https://boringssl-review.googlesource.com/c/34872
Reviewed-by: Adam Langley <agl@google.com>
EVP_get_cipherbyname should work on everything that EVP_do_all_sorted
lists, and conversely, there should be nothing that
EVP_get_cipherbyname works on that EVP_do_all_sorted doesn't list.
node.js uses these APIs to enumerate and instantiate ciphers.
Change-Id: I87fcedce62d06774f7c6ee7acc898326276be089
Reviewed-on: https://boringssl-review.googlesource.com/c/33984
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This uses the x86 trap flag and libunwind to test CFI works at each
instruction. For now, it just uses the system one out of pkg-config and
disables unwind tests if unavailable. We'll probably want to stick a
copy into //third_party and perhaps try the LLVM one later.
This tester caught two bugs in P-256 CFI annotations already:
I47b5f9798b3bcee1748e537b21c173d312a14b42 and
I9f576d868850312d6c14d1386f8fbfa85021b347
An earlier design used PTRACE_SINGLESTEP with libunwind's remote
unwinding features. ptrace is a mess around stop signals (see group-stop
discussion in ptrace(2)) and this is 10x faster, so I went with it. The
question of which is more future-proof is complex:
- There are two libunwinds with the same API,
https://www.nongnu.org/libunwind/ and LLVM's. This currently uses the
system nongnu.org for convenience. In future, LLVM's should be easier
to bundle (less complex build) and appears to even support Windows,
but I haven't tested this. Moreover, setting the trap flag keeps the
test single-process, which is less complex on Windows. That suggests
the trap flag design and switching to LLVM later. However...
- Not all architectures have a trap flag settable by userspace. As far
as I can tell, ARMv8's PSTATE.SS can only be set from the kernel. If
we stick with nongnu.org libunwind, we can use PTRACE_SINGLESTEP and
remote unwinding. Or we implement it for LLVM. Another thought is for
the ptracer to bounce SIGTRAP back into the process, to share the
local unwinding code.
- ARMv7 has no trap flag at all and PTRACE_SINGLESTEP fails. Debuggers
single-step by injecting breakpoints instead. However, ARMv8's trap
flag seems to work in both AArch32 and AArch64 modes, so we may be
able to condition it on a 64-bit kernel.
Sadly, neither strategy works with Intel SDE. Adding flags to cpucap
vectors as we do with ARM would help, but it would not emulate CPUs
newer than the host CPU. For now, I've just had SDE tests disable these.
Annoyingly, CMake does not allow object libraries to have dependencies,
so make test_support a proper static library. Rename the target to
test_support_lib to avoid
https://gitlab.kitware.com/cmake/cmake/issues/17785
Update-Note: This adds a new optional test dependency, but it's disabled
by default (define BORINGSSL_HAVE_LIBUNWIND), so consumers do not need
to do anything. We'll probably want to adjust this in the future.
Bug: 181
Change-Id: I817263d7907aff0904a9cee83f8b26747262cc0c
Reviewed-on: https://boringssl-review.googlesource.com/c/33966
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Maybe someday we'll be able to turn on that warning. (The EVP_CIPHER
hooks take size_t while the functions took long.)
Change-Id: Ic4da44efca9419a7f703e232d3f92638eb4ab37a
Reviewed-on: https://boringssl-review.googlesource.com/c/34084
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Postgres contains a “pqcrypto” module that showcases the worst of 90's
crypto, including Blowfish and CAST5 in CFB, CBC, and ECB modes. (Also,
64-bit keys for both of those.)
In order to minimise the patching needed to build Postgres, put these
things in decrepit.
Change-Id: I8390c5153dd7227eef07293a4363878d79df8b21
Reviewed-on: https://boringssl-review.googlesource.com/c/34044
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Everyone calls this with NULL anyway. People never actually use
lh_CONF_VALUE_* functions (or any other lh_* functions for that matter).
Also remove unused X509V3_EXT_CRL_add_conf prototype.
This removes one of the last mentions of LHASH_OF in public headers.
Update-Note: X509V3_EXT_conf_nid calls that pass a non-NULL first
parameter will fail to compile.
Change-Id: Ia6302ef7b494efeb9b63ab75a18bc340909dcba3
Reviewed-on: https://boringssl-review.googlesource.com/32117
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
This one is a little thorny. All the various block cipher modes
functions and callbacks take a void *key. This allows them to be used
with multiple kinds of block ciphers.
However, the implementations of those callbacks are the normal typed
functions, like AES_encrypt. Those take AES_KEY *key. While, at the ABI
level, this is perfectly fine, C considers this undefined behavior.
If we wish to preserve this genericness, we could either instantiate
multiple versions of these mode functions or create wrappers of
AES_encrypt, etc., that take void *key.
The former means more code and is tedious without C++ templates (maybe
someday...). The latter would not be difficult for a compiler to
optimize out. C mistakenly allowed comparing function pointers for
equality, which means a compiler cannot replace pointers to wrapper
functions with the real thing. (That said, the performance-sensitive
bits already act in chunks, e.g. ctr128_f, so the function call overhead
shouldn't matter.)
But our only 128-bit block cipher is AES anyway, so I just switched
things to use AES_KEY throughout. AES is doing fine, and hopefully we
would have the sense not to pair a hypothetical future block cipher with
so many modes!
Change-Id: Ied3e843f0e3042a439f09e655b29847ade9d4c7d
Reviewed-on: https://boringssl-review.googlesource.com/32107
Reviewed-by: Adam Langley <agl@google.com>
- In base.h, if BORINGSSL_PREFIX is defined, include
boringssl_prefix_symbols.h
- In all .S files, if BORINGSSL_PREFIX is defined, include
boringssl_prefix_symbols_asm.h
- In base.h, BSSL_NAMESPACE_BEGIN and BSSL_NAMESPACE_END are
defined with appropriate values depending on whether
BORINGSSL_PREFIX is defined; these macros are used in place
of 'namespace bssl {' and '}'
- Add util/make_prefix_headers.go, which takes a list of symbols
and auto-generates the header files mentioned above
- In CMakeLists.txt, if BORINGSSL_PREFIX and BORINGSSL_PREFIX_SYMBOLS
are defined, run util/make_prefix_headers.go to generate header
files
- In various CMakeLists.txt files, add "global_target" that all
targets depend on to give us a place to hook logic that must run
before all other targets (in particular, the header file generation
logic)
- Document this in BUILDING.md, including the fact that it is
the caller's responsibility to provide the symbol list and keep it
up to date
- Note that this scheme has not been tested on Windows, and likely
does not work on it; Windows support will need to be added in a
future commit
Change-Id: If66a7157f46b5b66230ef91e15826b910cf979a2
Reviewed-on: https://boringssl-review.googlesource.com/31364
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
This often causes confusion since, for various silly reasons (intrinsic
ref-counting, FOO_METHOD, and RSA's cached Montgomery bits), the thread
safety of some functions don't match the usual const/non-const
distinction. Fix const-ness where easy and document it otherwise.
Change-Id: If2037a4874d7580cc79b18ee21f12ae0f47db7fd
Reviewed-on: https://boringssl-review.googlesource.com/31344
Reviewed-by: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
We currently write a mix of "if (FOO)" and "if(FOO)". While the former looks
more like a usual language, CMake believes everything, even "if" and "else", is
just a really really funny function call (a "command").
We should pick something for consistency. Upstream CMake writes "if(FOO)", so
go with that one.
Change-Id: I67e0eb650a52670110b417312a362c9f161c8721
Reviewed-on: https://boringssl-review.googlesource.com/30807
Reviewed-by: Adam Langley <agl@google.com>
CMake targets are visible globally but gtest_main has boringssl-specific
behavior that isn't appropriate for general use.
This change makes it possible to use boringssl and abseil-cpp in the
same project (since abseil-cpp expects gtest_main to exist and be useful
for its own tests).
Change-Id: Icc81c11b8bb4b1e21cea7c9fa725b6c082bd5369
Reviewed-on: https://boringssl-review.googlesource.com/24604
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Fixes failed compile with [-Werror=implicit-fallthrough=], which is
default on gcc-7.x on distributions like fedora.
Enabling no implicit fallthrough for more than just clang as well to
catch this going forward.
Change-Id: I6cd880dac70ec126bd7812e2d9e5ff804d32cadd
Signed-off-by: Vincent Batts <vbatts@redhat.com>
Reviewed-on: https://boringssl-review.googlesource.com/20564
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The changes to delocate.go are needed because modes/ does things like
return the address of a module function. Both of these need to be
changed from referencing the GOT to using local symbols.
Rather than testing whether |ghash| is |gcm_ghash_avx|, we can just keep
that information in a flag.
The test for |aesni_ctr32_encrypt_blocks| is more problematic, but I
believe that it's superfluous and can be dropped: if you passed in a
stream function that was semantically different from
|aesni_ctr32_encrypt_blocks| you would already have a bug because
|CRYPTO_gcm128_[en|de]crypt_ctr32| will handle a block at the end
themselves, and assume a big-endian, 32-bit counter anyway.
Change-Id: I68a84ebdab6c6006e11e9467e3362d7585461385
Reviewed-on: https://boringssl-review.googlesource.com/15064
Reviewed-by: Adam Langley <agl@google.com>
For now, this is the laziest conversion possible. The intent is to just
get the build setup ready so that we can get everything working in our
consumers. The intended end state is:
- The standalone build produces three test targets, one per library:
{crypto,ssl,decrepit}_tests.
- Each FOO_test is made up of:
FOO/**/*_test.cc
crypto/test/gtest_main.cc
test_support
- generate_build_files.py emits variables crypto_test_sources and
ssl_test_sources. These variables are populated with FindCFiles,
looking for *_test.cc.
- The consuming file assembles those variables into the two test targets
(plus decrepit) from there. This avoids having generate_build_files.py
emit actual build rules.
- Our standalone builders, Chromium, and Android just run the top-level
test targets using whatever GTest-based reporting story they have.
In transition, we start by converting one of two tests in each library
to populate the three test targets. Those are added to all_tests.json
and all_tests.go hacked to handle them transparently. This keeps our
standalone builder working.
generate_build_files.py, to start with, populates the new source lists
manually and subtracts them out of the old machinery. We emit both for
the time being. When this change rolls in, we'll write all the build
glue needed to build the GTest-based tests and add it to consumers'
continuous builders.
Next, we'll subsume a file-based test and get the consumers working with
that. (I.e. make sure the GTest targets can depend on a data file.)
Once that's all done, we'll be sure all this will work. At that point,
we start subsuming the remaining tests into the GTest targets and,
asynchronously, rewriting tests to use GTest properly rather than
cursory conversion here.
When all non-GTest tests are gone, the old generate_build_files.py hooks
will be removed, consumers updated to not depend on them, and standalone
builders converted to not rely on all_tests.go, which can then be
removed. (Unless bits end up being needed as a malloc test driver. I'm
thinking we'll want to do something with --gtest_filter.)
As part of this CL, I've bumped the CMake requirements (for
target_include_directories) and added a few suppressions for warnings
that GTest doesn't pass.
BUG=129
Change-Id: I881b26b07a8739cc0b52dbb51a30956908e1b71a
Reviewed-on: https://boringssl-review.googlesource.com/13232
Reviewed-by: Adam Langley <agl@google.com>
Most C standard library functions are undefined if passed NULL, even
when the corresponding length is zero. This gives them (and, in turn,
all functions which call them) surprising behavior on empty arrays.
Some compilers will miscompile code due to this rule. See also
https://www.imperialviolet.org/2016/06/26/nonnull.html
Add OPENSSL_memcpy, etc., wrappers which avoid this problem.
BUG=23
Change-Id: I95f42b23e92945af0e681264fffaf578e7f8465e
Reviewed-on: https://boringssl-review.googlesource.com/12928
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
One of them is used in the new minimal SSL BIO, but cURL doesn't consume
it, so let's just leave it out. A consumer using asynchronous
certificate lookup is unlikely to be doing anything with SSL BIOs.
Change-Id: I10e7bfd643d3a531d42a96a8d675611d13722bd2
Reviewed-on: https://boringssl-review.googlesource.com/12686
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Macros need a healthy dose of parentheses to avoid expression-level
misparses. Most of this comes from the clang-tidy CL here:
https://android-review.googlesource.com/c/235696/
Also switch most of the macros to use do { ... } while (0) to avoid all
the excessive comma operators and statement-level misparses.
Change-Id: I4c2ee51e347d2aa8c74a2d82de63838b03bbb0f9
Reviewed-on: https://boringssl-review.googlesource.com/11660
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Adam Langley