Bug: chromium:40925630
Change-Id: Ide72960600747f5ce9a9213a9103510fee3e3806
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67527
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
After getting further with 672efb1f8e it turns out that there are more
SVE2 instruction forms that we need to be able to recognise.
Change-Id: Ia180c12ecf702e2c5536adbe1c30ac5ebd43fe75
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65727
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: Adam Langley <agl@google.com>
Change-Id: Ia6fffb4c1fbe9edc62a4c22b45408e41ac6ae086
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67547
Reviewed-by: David Benjamin <davidben@google.com>
Auto-Submit: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
The docs still describe the old implementation, but our PRNG has changed
drastically since then.
Change-Id: I51c34833a364a1d6bd70cf5d3b6cfb87b4aa06e7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67569
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Noncharacters are weird. They're code points and generally expected to
pass through string APIs and such, but they're also not meant to be used
for "open interchange". We reject them, while most Unicode APIs accept
them. They're public API nowadays, so document this.
Change-Id: I56aa436ae954b591d9a00b6560617e1ad5c26d95
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67568
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Some CLs crossed in-flight.
Change-Id: Id98cfb7c5af67275b99df627534a633f9ae3d14a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67587
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
We're using it in parts of EVP already and this is much more readable.
Change-Id: I42f30b83331cafdabd4f5d995b61176458e906bc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67567
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Matching the various pre-generated builds, the CMake build no longer
actually requires Go. The only things that need it are:
- Running tests
- Builds with -DFIPS=1
- Builds with the experimental symbol prefixing thing
Bug: 542
Change-Id: I52bb427f54dd6e5719cfe77773e87fc394410380
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67367
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Combined with https://crrev.com/c/5346536, this should, I believe, avoid
needing to re-download the toolchains over and over on every CI run once
the caches all fill.
Change-Id: I4991cf61dd164d7d39da91184ba7051ac59ce3f1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67347
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Change-Id: Iaa166136b4b7700e59c3a7643ec1b4aacf43c647
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66747
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Change-Id: I569110a8648f0504897b9ea94b115cd038149ace
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67327
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
https://boringssl-review.googlesource.com/c/boringssl/+/66288
allowed C++ runtime in libssl. The build script of bssl-sys crate
should indicate that the crate requires a C++ runtime. Use
libc++ on MacOS and libstdc++ on other unix like systems by
default. Introduce a new environment variable to configure C++
runtime to use.
Change-Id: Ib445955012126080dd03ad7b650287ea9dde10b0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67147
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Change-Id: I3663a5efe1f71192e69e3e04821a481043d145bb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67467
Commit-Queue: Aaron Knobloch <aknobloch@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Auto-Submit: Aaron Knobloch <aknobloch@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is sliiiightly weird because we can't quite decide whether these
are headers with the wrong extension or standalone C files. But since
most of our build does the former, I've done this for now. I expect
we'll need to iterate on this one a bit.
Bug: 542
Change-Id: Ib50332c0804efb5a1aa37fe445f129156260835a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67300
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
CMake doesn't use these, but a lot of our other build systems need it.
Bug: 542
Change-Id: I74388751b832921ac121abd3d5755880f352a449
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67299
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This avoids needing to rebase the source lists. It also means that
libcrypto.a and libssl.a end up directly in the build directory, which
makes it a bit easier to pass it to, say, gcc -L when testing things.
That file is, alas, getting a bit large. delocate is a pretty large
amount of code. I tried to abstract things into functions to toss into a
cmake/delocate.cmake, but CMake is really bad at making abstractions.
Bug: 542
Change-Id: I084d7a6bdd4c21ac27859b8b0c9d7a84829f2823
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67298
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The CMake build doesn't use it, but this will be needed for the other
builds to be driven by util/pregenerate.
Bug: 542
Change-Id: If95cbcef1803e30ffc5ab7c9227fdcc6c53adf34
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67296
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
We originally embedded test data because of deficiencies in Android's
build. Android had no way to specify test data with tests. That has
since been resolved, and the embedding mechanism has gotten unwieldy.
This unifies pki_test and crypto_test's test data story, and does so in
a way that all tests can participate in. (We can now use FileTest in
decrepit_test.)
Update-Note: This will require some tweaks to downstream builds. We no
longer emit an (unwieldy) crypto_test_data.cc file. Instead, tests will
expect test data be available at the current working directory. This can
be overridden with the BORINGSSL_TEST_DATA_ROOT environment variable.
Callers with more complex needs can build with
BORINGSSL_CUSTOM_GET_TEST_DATA and then link in an alternate
implementation of this function.
On the off chance some project needs it, I've kept the
embed_test_data.go script around for now, but I expect we can delete it
in the future.
Fixed: 681
Change-Id: If181ce043e1eea3148838f1bb4db9ee4bfda0d08
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67295
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
We now have (some of) our test sources in an easily parseable form.
run_android_tests.go no longer needs to crawl the source tree.
Note this required fixing the .gitignore rules. If a .gitignore line
doesn't have a slash at the start or middle, it applies to
subdirectories as well. This is confusing, so I just stuck a leading
slash in front of all of them.
Bug: 681
Change-Id: I389c2a0560594fbd23c60b5b614b0ccfedf28926
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67293
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Since we can now handle glob patterns, make those shorter.
Bug: 542
Change-Id: I971d9785bce0db7b2e8c41c8c82468afde64540d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67292
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This is primarily in preparation for pki_test's data list. That thing is
80% of source.cmake. glob patterns are normally not great, but since
we're checking the result in, that should be fine.
Bug: 542
Change-Id: I6ccf69f4a2ce08b153de5eb9dfb2f9b01654e1ce
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67290
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
The error list was updated in between when I uploaded and landed the
generated version.
Bug: 542
Change-Id: I4d0efdca20264fd2a6508dd8ff4065bd903d5a79
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67428
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This adds a tool for managing pre-generated files, aligning our CMake
and non-CMake builds. The plan is roughly:
The source of truth for the file lists will (eventually) be build.json.
This describes the build in terms of the files that we directly edit.
However, we have a two-phase build. First a pregeneration step
transforms some of the less convenient inputs into checked in files.
Notably perlasm files get expanded. This produces an equivalent JSON
structure with fewer inputs. The same tool then outputs that structure
into whatever build systems we want.
This initial version pre-generates err_data.c and perlasm files. I've
not wired up the various build formats, except for CMake (for the CMake
build to consume) and JSON (for generate_build_files.py to parse).
build.json is also, for now, only a subset of the build. Later changes
The upshot of all this is we no longer have a Perl build dependency!
Perl is now only needed when working on BoringSSL. It nearly removes the
Go one, but Go is still needed to run and (for now) build the tests.
To keep the generated files up-to-date, once this lands, I'll update our
CI to run `go run ./util/pregenerate -check` which asserts that all
generated files are correct. From there we can land the later changes in
this patch series that uses this more extensively. My eventual goal is
to replace generate_build_files.py altogether and the
"master-with-bazel" branch. Instead we'll just have sources.bzl,
sources.gni, etc. all checked into the tree directly. And then the
normal branch will just have both a CMake and Bazel build in it.
Update-Note: generate_build_files.py no longer generates assembly files
or err_data.c. Those are now checked into the tree directly.
Bug: 542
Change-Id: I71f5ff7417be811f8b7888b345279474e6b38ee9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67288
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This is one step towards flattening the libcrypto build into the
top-level CMakeLists.txt file. (This is to align the CMake build with
our other builds, and also make it easier to consume a CRYPTO_SOURCES
variable without having to prepend "../" in front of each value.)
This also avoids a rather annoying corner of CMake: files generated in a
different directory don't work well, which is why we had all this mess
with EXTERNAL_OBJECT, GENERATED, and bcm_o_target. Globbing it into one
file is a bit unwieldy, but avoids this. (CMake is incredibly bad at
custom rules.)
Bug: 542
Change-Id: Ia5038511af339a0eae2af56875a42581eb1ed15b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67287
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Lots of code relies on this, so we ought to document it. A null
STACK_OF(T) is treated as an immutable empty list.
Change-Id: I10d0ba8f7b33c7f3febaf92c2cd3da25a0eb0f80
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67407
Reviewed-by: Theo Buehler <theorbuehler@gmail.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Change-Id: Ie16e9ab0897305089672720efa4530d43074f692
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67387
Auto-Submit: Theo Buehler <theorbuehler@gmail.com>
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This aligns with the generated build, where fips_shared_support.c is
part of crypto_sources, not the delocated part.
It is conceptually part of bcm, but our generated builds currently
only separate on basis of delocated/partial-linked vs. not delocated.
Bug: 542
Change-Id: Ib8de3fb0a7778c9000c3b4fca978d43cb9a29d12
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67267
Reviewed-by: Adam Langley <agl@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Since we are saying this will die when standardized, let us
ensure users of this code from this location take notice
and action before using it.
We then selectively allow it in the speed tool and in our tests.
If we like this approach, I'll go back and apply it to kyber
(which will have some other fallout consequences to fix) but this
one should be painless right now.
This can also be applied to Dilithium when it comes back.
Future experimentals could be added in this manner.
Change-Id: Ie3b41cf16278868562ef1c8b28f2caed5e0e2dd1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66887
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
OpenSSH needs this. Features that have been intentionally omitted for
now:
- X9.42-style Diffie-Hellman ("DHX"). We continue not to support this.
Use ECDH or X25519 instead.
- SPKI and PKCS#8 serialization. Use ECDH or X25519 instead. The format
is a bit ill-defined. Moreover, until we solve the serialization
aspects of https://crbug.com/boringssl/497, adding them would put this
legacy algorithm on path for every caller.
- Most of the random options like stapling a KDF, etc. Though I did add
EVP_PKEY_CTX_set_dh_pad because it's the only way to undo OpenSSL's
bug where they chop off leading zeros by default.
- Parameter generation. Diffie-Hellman parameters should not be
generated at runtime.
This means you need to bootstrap with a DH object and then wrap it in an
EVP_PKEY. This matches the limitations of the EVP API in OpenSSL 1.1.x.
Unfortunately the OpenSSL 3.x APIs are unsuitable for many, many
reasons, so I expect when we get further along in
https://crbug.com/boringssl/535, we'll have established some patterns
here that we can apply to EVP_PKEY_DH too.
Change-Id: I34b4e8799afb266ea5602a70115cc2146f19c6a7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67207
Reviewed-by: Theo Buehler <theorbuehler@gmail.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
We've historically settled on treating asserts as not in scope for our
constant-time goals. Production binaries are expected to be optimized
builds, with debug assertions turned off. (We have a handful of
assertions in perf-sensitive code that you definitely do not want to run
with.) Secret data has invariants too, so it is useful to be able to
write debug assertions on them.
However, combined with our default CMake build being a debug build, this
seems to cause some confusion with researchers sometimes. Also, if we
ever get language-level constant-time support, we would need to resolve
this mismatch anyway. (I assume any language support would put enough
into the type system to force us to declassify any intentional branches
on secret-by-data-flow bools, notably those we assert on.) So I'm
inclined to just make our asserts constant-time.
There are two issues around asserts, at least with our valgrind-based
validation:
The first is that a couple of asserts over secret data compute their
condition leakily. We can just fix these. The only such ones I found
were in bn_reduce_once and bn_gcd_consttime.
The second is that almost every assert over secret data will be flagged
as an invalid branch by valgrind. However, presuming the condition
itself was computed in constant time, this branch is actually safe. If
we were willing to abort the process when false, the assert is clearly
publicly true. We just need to declassify the boolean to assert on it.
assert(constant_time_declassify_int(expr)) is really long, so I made an
internal wrapper macro declassify_assert(expr). Not sure if that's the
best name. constant_time_declassify_assert(expr) is kinda long.
constant_time_assert(expr) fits with the rest of that namespace, but
reads as if we're somehow running an assert without branching, when the
whole point is that we *are* branching and need to explicitly say it's
okay to.
Fixed: 339
Change-Id: Ie33b99bf9a269b11d2c48d246cc4934be7e239ff
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65467
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Mostly bits of DSA and RSA keygen, flagged when we make the PRNG output
secret by default. There's still a ton of RSA to resolve, mostly because
our constant-time bignum strategy does not interact well with valgrind
when handling RSA's secret-value / public-bit-length situation. Also
RSA's ASN.1 serialization is unavoidably leaky.
Bug: 676
Change-Id: I08d273959065c4db6fd44180a6ac56a82f862fe8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65447
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This API allocates internally and can leave a corrupted |alg| behind.
Change it to return an int so that callers can check for an error.
Also fix its only caller in rsa_md_to_algor().
This is an ABI change but will not break any callers.
Also add a small regress test for this API.
Change-Id: I7a5d1729dcd4c7726c3d4ead3740d478231f3611
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67187
Commit-Queue: Theo Buehler <theorbuehler@gmail.com>
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
The Python scripts (which probably don't work anymore) don't need to be
shipped to the test. Neither to the .h files. Also we have some random
files that are remnants of being around Chromium.
We also don't need the ship the .key files.
Change-Id: I847b449accee1c4005304380b47f3ff876a09fa5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67291
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This seems to be unnecessary.
Change-Id: I0439739543d6593aadc87fc97e4ad5870616730e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67268
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The RSA, etc., APIs have some discussion on threading expectations. We
should have the same text on DH and DSA.
While I'm here, const-correct DSA_SIG in some legacy DSA APIs.
Change-Id: I6ad43c9347c320dc0b1c8e73850fa07c41e028ea
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67247
Reviewed-by: Theo Buehler <theorbuehler@gmail.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This function exists because callers sometimes write
EVP_PKEY_type(EVP_PKEY_id(pkey)), which is equivalent to
EVP_PKEY_base_id(pkey).
In OpenSSL, all this existed so that a type parsed as EVP_PKEY_RSA2
could still be mapped to EVP_PKEY_RSA. We haven't supported this since
2015, so this purely exists as a way to check that the key type exists.
In doing so, it currently pulls in the full implementation of every key
type.
I could replicate the list of keys, but that is one more place we have
to keep things up-to-date. Instead, just make this function the
identity. Looking through callers, it did not appear anyone depended on
the error condition.
Update-Note: EVP_PKEY_type used to return NID_undef when given a garbage
key type. Given it is only ever used in concert with EVP_PKEY_id, this
is unlikely to impact anyone. If it does, we can do the more tedious
option.
Bug: 497
Change-Id: Ibf68a07ef6906398df0fec425c869c107b8c90f4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67109
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
These are effectively just APIs for creating Ed25519 and X25519 keys. We
may want to rethink this a bit later, but for now let's just do this.
Bug: 497
Change-Id: I01ae06fa86af96da993fd41611472838475bf094
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67128
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
EVP_PKEY_set_type needs to pull in every supported EVP_PKEY type, but
most of our calls within the library already know what type they're
working with. Have them call evp_pkey_set_method directly.
Bug: 497
Change-Id: I17cb9a0dff0da55206686bce1d8e1df4773f6f4d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67127
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
We say "all tests passed" but we actually mean only the unit tests. Now
the output is:
Running Go tests
ok boringssl.googlesource.com/boringssl/ssl/test/runner/hpke (cached)
ok boringssl.googlesource.com/boringssl/util/ar (cached)
ok boringssl.googlesource.com/boringssl/util/fipstools/acvp/acvptool/testmodulewrapper (cached)
ok boringssl.googlesource.com/boringssl/util/fipstools/delocate (cached)
Running unit tests
ssl_test [shard 1/10]
...
pki_test [shard 8/10]
All unit tests passed!
Running SSL tests
0/0/5481/5481/5481
PASS
ok boringssl.googlesource.com/boringssl/ssl/test/runner 21.110s
all_tests.go really should be called unit_tests.go, but renaming it will
probably be too annoying.
Change-Id: I7ff6684221930e19152ab3400419f4e5209aaf46
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67107
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
tb noticed that our X509_ALGOR_set_md differs from OpenSSL because we
never set EVP_MD_FLAG_DIGALGID_ABSENT. That is, we include an explicit
NULL parameter, while OpenSSL omits it.
RFC 4055, section 2.1 says:
There are two possible encodings for the AlgorithmIdentifier
parameters field associated with these object identifiers. The two
alternatives arise from the loss of the OPTIONAL associated with the
algorithm identifier parameters when the 1988 syntax for
AlgorithmIdentifier was translated into the 1997 syntax. Later the
OPTIONAL was recovered via a defect report, but by then many people
thought that algorithm parameters were mandatory. Because of this
history some implementations encode parameters as a NULL element
while others omit them entirely. The correct encoding is to omit the
parameters field; however, when RSASSA-PSS and RSAES-OAEP were
defined, it was done using the NULL parameters rather than absent
parameters.
...
To be clear, the following algorithm identifiers are used when a NULL
parameter MUST be present:
...
My read of this text is:
1. The correct encoding of, say, SHA-256 as an AlgorithmIdentifer *was*
to omit the parameter. So if you're using it in, I dunno, CMS, you
should omit it.
2. Due to a mishap, RSASSA-PSS originally said otherwise and included
it. Additionally, there are some implementations that only work if
you include it.
3. Once the mistake was discovered, PSS chose to preserve the mistake,
rather than undo it.
This means that the correct encoding of SHA-256 as an AlgorithmIdentifer
is *different* depending on whether you're doing PSS or CMS.
Fortunately, there are only two users of this function, one inside the
library and one in Android. Both are trying to encode PSS, so the
current behavior is correct. Nonetheless, we should document this.
Also, because this is a huge mess, we should also add an API for
specifically encoding RSA-PSS. From there, we can update Android to call
that function and remove X509_ALGOR_set_md.
Amusingly, RSASSA-PKCS1-v1_5 *also* differs from the "correct" encoding.
RFC 8017, Appendix B.1 says:
The parameters field associated with id-sha1, id-sha224, id-sha256,
id-sha384, id-sha512, id-sha512/224, and id-sha512/256 should
generally be omitted, but if present, it shall have a value of type
NULL.
This is to align with the definitions originally promulgated by NIST.
For the SHA algorithms, implementations MUST accept
AlgorithmIdentifier values both without parameters and with NULL
parameters.
Exception: When formatting the DigestInfoValue in EMSA-PKCS1-v1_5
(see Section 9.2), the parameters field associated with id-sha1,
id-sha224, id-sha256, id-sha384, id-sha512, id-sha512/224, and
id-sha512/256 shall have a value of type NULL. This is to maintain
compatibility with existing implementations and with the numeric
information values already published for EMSA-PKCS1-v1_5, which are
also reflected in IEEE 1363a [IEEE1363A].
Finally, there's EVP_marshal_digest_algorithm, used in PKCS#8 and OCSP.
I suspect we're doing that one wrong. I've left a TODO there to dig into
that one.
Bug: 710
Change-Id: I46b11f8c56442a9badd186c7f04bb366147ed98f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67088
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Nick Harper
Adam Langley
Brian Ledger
David Benjamin
Adam Langley
Kenichi Ishibashi
Aaron Knobloch
Theo Buehler
Bob Beck