OpenSSL has a large exported API surface for exporting the policy tree
out of an X509_STORE_CTX. As far as I can tell, no one uses any of these
APIs. Remove them.
Update-Note: It is no longer possibly to see the policy tree after an
X.509 verification. As far as we can tell, this feature is unused.
Change-Id: Ieab374774805e90106555ce4e4155f8451ceb5b9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53327
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Unfortunately, these functions are not const-correct, even the
accessors. Document what they should have been, especially since
mutating an X509_NAME_ENTRY directly won't even update the 'modified'
bit correctly.
Do a pass at adding consts to our code internally, but since the
functions return non-const pointers, this isn't checked anywhere. And
since serializing an X509_NAME is not always thread-safe, there's a
limit to how much we can correctly mark things as const.
Bug: 426
Change-Id: Ifa3d8bafb5396fbe7b91416f234de4585284c705
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53326
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The X509_NAME representation is a little odd because it flattens the
RDN sequence, but maintains RDN indices (X509_NAME_ENTRY_set) to recover
the information. add_entry and delete_entry both have logic to maintain
invariants. Test that they work.
Change-Id: I7d4a033db0a82a1f13888bbfca29076b589cc214
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53325
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
I found no uses of this. If we have to put it back, it's no big deal,
but one less group of functions to have to document. (SEQUENCE OF
AlgorithmIdentifier is mostly a weird PKCS#7 thing.)
Update-Note: X509_ALGORS is removed. If someone was relying on it, we
can re-export it.
Change-Id: I2b8e8d0d1d56d2debf99687023bc3621e92f6b08
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53307
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
We have no X509_REQ_up_ref function, and neither does OpenSSL. That
means this reference count is unreachable, so remove it.
Change-Id: I6a0899b8fcb0716d47c89174b561e90d99a9b54b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53306
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Types which do not contain an X509_NAME can be const. X509_NAME still
requires some thought. (i2d_X509_NAME can mutate the object and isn't
even thread-safe when modified.)
Bug: 407
Change-Id: Iceafa2b4ea9c4194cfcc3044d90393b5d91f7c11
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53305
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
It seems no one has ever exercised this mode to notice it doesn't work.
Add a test for this, which catches the memory leak. See also
https://github.com/openssl/openssl/issues/18677
Change-Id: I8890febc71decd553a040bdda8739f68ed616c39
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53285
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
ASN1_STRING_set saves having to manually manage the ASN1_STRING's
buffer. Also size that buffer correctly and align the free_s vs tmps
mismatch.
We can also assume OPENSSL_gmtime writes to the passed-in struct tm.
Every other caller already assumes this, and both POSIX (gmtime_r) and C
(gmtime_s) agree this is valid.
Finally, the cleanup logic is much simpler if we do all the time stuff
before handling the maybe-object-reuse bits.
Change-Id: I25befc8fa93a1a60246c6abcea6e5d58ee563b48
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53227
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Switch to the CBS functions, which do all the checks together. This
resolves a TODO that ASN1_STRING_print_ex was inconsistently checking
for invalid codepoints. It also removes an optimization when
round-tripping UTF-8. This optimization was incorrect if the input was
invalid.
Finally, this removes UTF8_getc, which no longer has any callers.
(I've left UTF8_putc for now because CBB would force a malloc on every
character, even with CBB_init_fixed. We should either decide we don't
care, or make it possible to stack-allocate the cbb_buffer_st.)
Update-Note: This will make ASN1_STRING_print_ex newly fail, but such
inputs should be unreachable from the parser as of an earlier change.
Change-Id: I52d747c500c6f5f9ef659cdee3ef5d241f38ed21
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53226
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This checks validity for UTF8String, BMPString, and UniversalString.
When we detach the core types from ASN1_ITEM, this will likely also be
reshuffled around, probably into type-specific functions. But for now
just get the checks in place.
Update-Note: Invalid strings in X.509 certificates and other ASN.1
structures will now be rejected. This change is less risky than it seems
because most strings in X.509 are in X509_NAME, which already rejected
invalid instances of these string types (but not other string types)
during canonicalization. See https://crbug.com/boringssl/412 for a
discussion of that mess.
Bug: 427
Change-Id: I0d7e24dfd841703d2a8581ec4e78ed5bc3862d75
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53225
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This reuses code we already have, and also avoids another union.
Bug: 301
Change-Id: I2f4ab564cf220818ca96d9c99a122c1e5cdc9e7a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53228
Reviewed-by: Adam Langley <agl@google.com>
Our free functions all tolerate NULL.
Change-Id: Ifcb3185c8d2f34afb83f7286c3463136edc926fa
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53125
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
No sense in keeping two around. This does cause the functions to reject
some previously accepted invalid inputs. These were intentionally
accepted by
https://boringssl-review.googlesource.com/c/boringssl/+/13082 for
an old version of M2Crypto, but I belive we no longer need to be
compatible with that.
Update-Note: ASN1_TIME_print, ASN1_UTCTIME_print, and
ASN1_GENERALIZEDTIME_print will no longer accept various invalid inputs.
Change-Id: I4606d0b39585a19eb4b984ac809706e497a3f799
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53090
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This may be okay because of the strict aliasing character type rule, but
easier not to think about it.
Bug: 301
Change-Id: I5eec356a5411a67036425e953e56529bac81ad4a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53091
Reviewed-by: Adam Langley <agl@google.com>
The comment says (in the now outdated orflags terms) that we don't need
to worry about this case because is_first/is_last only affect ASCII
codepoints, but it's easier to just set it correctly.
Change-Id: Ib6db66adb162a555da50f563ffc9af9da4a878ec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53126
Reviewed-by: Adam Langley <agl@google.com>
This is a mechanical change generated from the following command:
find crypto/{asn1,pem,x509,x509v3} -name '*.c' -o -name '*.h' | xargs sed -i -e 's/return (\([^;()]*\));/return \1;/'
Change-Id: I957295af96c4aa08d6006e27093fd3a07fb6fe75
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53089
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Some things that I noticed paging through.
Change-Id: I41e5e12b743fd996f5ff91d19f6edc074169f629
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53088
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This CL is the result of the following commands:
for d in asn1 x509 x509v3 pem; do
go run util/convert_comments.go crypto/$d/*.h
go run util/convert_comments.go crypto/$d/*.c
done
Change-Id: If78433f68cb2f913b0de06ded744a5a65540e1cf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53087
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This CL runs the same command as in the preceding CL, but with
'IncludeBraces: true' added to .clang-format. I've split this out
separately because the documentation says:
> Setting this option to true could lead to incorrect code formatting
> due to clang-format’s lack of complete semantic information. As such,
> extra care should be taken to review code changes made by this option.
I've also kept InsertBraces out of .clang-format for now because it's a
fairly recent option, and clang-format fails when it sees unrecognized
options.
Change-Id: I305ea7bb2633704053a1f8de1e11b037b9fc8a76
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53086
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Previously, we did not clang-format a few directories because we had
left them largely untouched. clang-format them now so we're finally more
uniform.
This CL is the result of the following commands:
for d in asn1 x509 x509v3 pem; do
clang-format -i crypto/$d/*.h
clang-format -i crypto/$d/*.c
done
(Written in this funny way because crypto/pem/*.h doesn't match
anything.)
Change-Id: I7f4ca9b3a9c8f07d6556e00e9e84b3c0880ee12e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53085
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Certain applications of Trust Token need to be able to generate
a large number of keys, instead of storing them all, we provide
an API to take a secret that can be used to generate keys
in a deterministic manner.
Change-Id: I2b61958d1e949a3a47a3c91ab3a866c2e33a9d1d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53011
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
Change-Id: Id5fda00fe27eb9bc8313dd81a5b0c720323e3903
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53045
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Refuse to parse times that are invalid according to RFC 5280, with
a few exceptions for compatibility. This can affect test code that
relies on making and parsing certificates that contain invalid times.
Update-Note: Certificates containing invalid ASN.1 times will no longer parse.
Bug: 491, 427
Change-Id: I2a3fe3a4d359ac662340a225d05b360718eb8c29
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52665
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
This results in one change, which is the comparison function goes from:
typedef char *OPENSSL_STRING;
static int sk_strcmp(const OPENSSL_STRING *a,
const OPENSSL_STRING *b);
which is:
static int sk_strcmp(char *const *a, char *const *b)
into:
static int sk_strcmp(const char **a, const char **b)
Neither is correct (both consts should be there), but switching the
defintion is necessary to attach the 'const' to 'char' itself. Otherwise
it wouldn't see through the typedef. Fixing the rest of the calling
convention will finish the job. Plan there is, when
I00d13c949a535c0d60873fe4ba2e5604bb585cca lands, I'll switch Envoy to
call that. Then we should be clear to const-correct the callback.
(While STACK_OF(OPENSSL_STRING) is used externally, nothing external
touches the comparison function.)
Bug: 498
Change-Id: I77bdf2a72b2553bf9409a1d39326890ed5c3582c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53008
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Starting version 2019 16.8 (released November 2020), MSVC actually
implements parts of C11, though disabled by default. You have to pass
/std:c11, and then alignas, alignof, and noreturn all work.
When built that way, better to use the real ones, so check for
__STDC_VERSION__ first. It would be nice to mandate that so we can
remove the polyfill, but for now just opportunistically use it when we
can.
Sadly, even in this mode, they still don't implement C11 atomics, so the
refcounting implementation will still be slow.
Change-Id: I28dab4a339c368f7d8f8da5aa7aee1cb344803d3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53006
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
MSVC 2022's C4191 warns on most function pointer casts. Fix and/or
silence them:
connect.c is an unavoidable false positive. We're casting the pointer to
the correct type. The problem was that the caller is required to first
cast it to the wrong type in OpenSSL's API, due to the BIO_callback_ctrl
calling convention. Suppress it.
LHASH_OF(T) and STACK_OF(T)'s defintions also have a false positive.
Suppress that warning. Calling the functions through the casted types
would indeed be UB, but we don't do that. We use them as goofy
type-erased types. The problem is there is no function pointer
equivalent of void*. (Using actual void* instead trips a GCC warning.)
The sk_sort instance is a true instance of UB. The problem is qsort
lacks a context parameter. I've made sk_sort call qsort_s on _MSC_VER,
to silence the warning. Ideally we'd fix it on other platforms, but
qsort_r and qsort_s are a disaster. See
https://stackoverflow.com/a/39561369
Fixed: 495
Change-Id: I0dca80670c74afaa03fc5c8fd7059b4cfadfac72
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53005
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
I'm not sure what the history of this is, but it seems to work
just fine in MSVC now.
Change-Id: Iebdc365486bb30a61a1001f705aef7dcaa2a9fcd
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52985
Reviewed-by: Adam Langley <agl@google.com>
bind uses this function.
Change-Id: I97ba86d9f75597bff125ae0b56952effc397e6b8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53010
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This split in methods probably needs rearranging. This means most
callers (i.e. ones that don't use EVP_PKEY_CTX_new_id) will no longer
pick up the HKDF EVP method.
Bug: 497
Change-Id: I72ef255a472c458e8db62461f7b3ac65fa488535
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52830
Reviewed-by: Adam Langley <agl@google.com>
I don't think these are all UB by C's rules, but it's easier not to
think about the pointers. Still more to go, but these were some easy
ones.
Bug: 301
Change-Id: Icdcb7fb40f85983cbf566786c5f7dbfd7bb06571
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52905
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
OpenSSL 1.1.1 added HKDF support, but by sticking it into
EVP_PKEY_derive, the API meant for Diffie-Hellman-like primitives.
Implement it for OpenSSL compatibility.
This does unfortunately mean anything using EVP now pulls in HKDF. HKDF
isn't much code, but we should make EVP more static-linker-friendly.
(Filed https://crbug.com/boringssl/497)
Change-Id: I90b9b0d918129829eb36ba9d50ff4d8580346ff0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52829
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
https://boringssl-review.googlesource.com/c/boringssl/+/52825 lost a
tmp.width = top line. Without it, tmp.width was set by
bn_one_to_montgomery. Since we always size modular arithmetic by the
modulus, tmp.width (and am.width) will actually always be top, and
there's actually no need to zero pad it.
We don't capture this in the type system or BIGNUM width convention, so
better to set the width explicitly. The original code did it at the end,
but I think doing it right when we zero pad it is better, as that's when
the size gets set.
But we can go a step further. The manual zero padding code came from
OpenSSL, which still had the bn_correct_top invariant. Our BIGNUMs are
resizable, so just call bn_resize_words, immediately after the
computation.
(bn_resize_words will not reallocate the data because the BIGNUMs have
the STATIC_DATA flag set. bn_wexpand will internally allow expanding up
to dmax, or top.)
Change-Id: I2403afa7381b8a407615c6730fba9edaa41125c6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52906
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(It's a pain because we don't have it setup in CMake, but perhaps we
should have a builder for the configuration that doesn't have bcm.c.)
Change-Id: Ic408f0a86c9d42346244d6a7b7e9e664b58fc70c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52845
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Both implementations need to compute the first 32 powers of a. There's a
commented out naive version in rsaz_exp.c that claims to be smaller, but
1% slower. (It doesn't use squares when it otherwise could.)
Instead, we can write out the square-based strategy as a loop. (I wasn't
able to measure a difference between any of the three versions, but this
one's compact enough and does let us square more and gather5 less.)
Change-Id: I7015f2a78584cd97f29b54d0007479bdcc3a01ba
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52828
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
The unrolled loops appear to have negligible perf impact:
Before:
Did 18480 RSA 2048 signing operations in 10005085us (1847.1 ops/sec)
Did 2720 RSA 4096 signing operations in 10056337us (270.5 ops/sec)
After:
Did 18480 RSA 2048 signing operations in 10012218us (1845.7 ops/sec) [-0.1%]
Did 2700 RSA 4096 signing operations in 10003972us (269.9 ops/sec) [-0.2%]
Change-Id: I29073c373a03a9798f6e04016626e6ab910e893a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52826
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
RSAZ has a very similar bug to mont5 from
https://boringssl-review.googlesource.com/c/boringssl/+/52825 and may
return the modulus when it should return zero. As in that CL, there is
no security impact on our cryptographic primitives.
RSAZ is described in the paper "Software Implementation of Modular
Exponentiation, Using Advanced Vector Instructions Architectures".
The bug comes from RSAZ's use of "NRMM" or "Non Reduced Montgomery
Multiplication". This is like normal Montgomery multiplication, but
skips the final subtraction altogether (whereas mont5's AMM still
subtracts, but replaces MM's tigher bound with just the carry bit). This
would normally not be stable, but RSAZ picks a larger R > 4M, and
maintains looser bounds for modular arithmetic, a < 2M.
Lemma 1 from the paper proves that NRMM(a, b) preserves this 2M bound.
It also claims NRMM(a, 1) < M. That is, conversion out of Montgomery
form with NRMM is fully reduced. This second claim is wrong. The proof
shows that NRMM(a, 1) < 1/2 + M, which only implies NRMM(a, 1) <= M, not
NRMM(a, 1) < M. RSAZ relies on this to produce a reduced output (see
Figure 7 in the paper).
Thus, like mont5 with AMM, RSAZ may return the modulus when it should
return zero. Fix this by adding a bn_reduce_once_in_place call at the
end of the operation.
Change-Id: If28bc49ae8dfbfb43bea02af5ea10c4209a1c6e6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52827
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This partially fixes a bug where, on x86_64, BN_mod_exp_mont_consttime
would sometimes return m, the modulus, when it should have returned
zero. Thanks to Guido Vranken for reporting it. It is only a partial fix
because the same bug also exists in the "rsaz" codepath. That will be
fixed in the subsequent CL. (See the commented out test.)
The bug only affects zero outputs (with non-zero inputs), so we believe
it has no security impact on our cryptographic functions. BoringSSL
calls BN_mod_exp_mont_consttime in the following cases:
- RSA private key operations
- Primality testing, raising the witness to the odd part of p-1
- DSA keygen and key import, pub = g^priv (mod p)
- DSA signing, r = g^k (mod p)
- DH keygen, pub = g^priv (mod p)
- Diffie-Hellman, secret = peer^priv (mod p)
It is not possible in the RSA private key operation, provided p and q
are primes. If using CRT, we are working modulo a prime, so zero output
with non-zero input is impossible. If not using CRT, we work mod n.
While there are nilpotent values mod n, none of them hit zero by
exponentiating. (Both p and q would need to divide the input, which
means n divides the input.)
In primality testing, this can only be hit when the input was composite.
But as the rest of the loop cannot then hit 1, we'll correctly report it
as composite anyway.
DSA and DH work modulo a prime, where this case cannot happen.
Analysis:
This bug is the result of sloppiness with the looser bounds from "almost
Montgomery multiplication", described in
https://eprint.iacr.org/2011/239. Prior to upstream's
ec9cc70f72454b8d4a84247c86159613cee83b81, I believe x86_64-mont5.pl
implemented standard Montgomery reduction (the left half of figure 3 in
the paper).
Though it did not document this, ec9cc70f7245 changed it to implement
the "almost" variant (the right half of the figure.) The difference is
that, rather than subtracting if T >= m, it subtracts if T >= R. In
code, it is the difference between something like our bn_reduce_once,
vs. subtracting based only on T's carry bit. (Interestingly, the
.Lmul_enter branch of bn_mul_mont_gather5 seems to still implement
normal reduction, but the .Lmul4x_enter branch is an almost reduction.)
That means none of the intermediate values here are bounded by m. They
are only bounded by R. Accordingly, Figure 2 in the paper ends with
step 10: REDUCE h modulo m. BN_mod_exp_mont_consttime is missing this
step. The bn_from_montgomery call only implements step 9, AMM(h, 1).
(x86_64-mont5.pl's bn_from_montgomery only implements an almost
reduction.)
The impact depends on how unreduced AMM(h, 1) can be. Remark 1 of the
paper discusses this, but is ambiguous about the scope of its 2^(n-1) <
m < 2^n precondition. The m+1 bound appears to be unconditional:
Montgomery reduction ultimately adds some 0 <= Y < m*R to T, to get a
multiple of R, and then divides by R. The output, pre-subtraction, is
thus less than m + T/R. MM works because T < mR => T' < m + mR/R = 2m.
A single subtraction of m if T' >= m gives T'' < m. AMM works because
T < R^2 => T' < m + R^2/R = m + R. A single subtraction of m if T' >= R
gives T'' < R. See also Lemma 1, Section 3 and Section 4 of the paper,
though their formulation is more complicated to capture the word-by-word
algorithm. It's ultimately the same adjustment to T.
But in AMM(h, 1), T = h*1 = h < R, so AMM(h, 1) < m + R/R = m + 1. That
is, AMM(h, 1) <= m. So the only case when AMM(h, 1) isn't fully reduced
is if it outputs m. Thus, our limited impact. Indeed, Remark 1 mentions
step 10 isn't necessary because m is a prime and the inputs are
non-zero. But that doesn't apply here because BN_mod_exp_mont_consttime
may be called elsewhere.
Fix:
To fix this, we could add the missing step 10, but a full division would
not be constant-time. The analysis above says it could be a single
subtraction, bn_reduce_once, but then we could integrate it into
the subtraction already in plain Montgomery reduction, implemented by
uppercase BN_from_montgomery. h*1 = h < R <= m*R, so we are within
bounds.
Thus, we delete lowercase bn_from_montgomery altogether, and have the
mont5 path use the same BN_from_montgomery ending as the non-mont5 path.
This only impacts the final step of the whole exponentiation and has no
measurable perf impact.
In doing so, add comments describing these looser bounds. This includes
one subtlety that BN_mod_exp_mont_consttime actually mixes bn_mul_mont
(MM) with bn_mul_mont_gather5/bn_power5 (AMM). But this is fine because
MM is AMM-compatible; when passed AMM's looser inputs, it will still
produce a correct looser output.
Ideally we'd drop the "almost" reduction and stick to the more
straightforward bounds. As this only impacts the final subtraction in
each reduction, I would be surprised if it actually had a real
performance impact. But this would involve deeper change to
x86_64-mont5.pl, so I haven't tried this yet.
I believe this is basically the same bug as
https://github.com/golang/go/issues/13907 from Go.
Change-Id: I06f879777bb2ef181e9da7632ec858582e2afa38
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52825
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
For now, it contains a call to set the service indicator so it should
live in the module. In term it would be nice to move it back out and
have the service indicator set in RSA and ECDSA functions themselves
once the ECDSA functions can take an indicator of the hash function
used.
Change-Id: I2a3c262f66b1881a96ae3e49784a0dc9fc8c4589
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52705
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
clang-format gets very confused by the comments in these tables. (The
comments seem to have already gotten a little messed up from upstream's
reformatted.) Reformat them ahead of time. I removed the tag2str number
comments as they aren't really doing much good at this point.
Also remove the last entry in tag2bits because it's not actually used.
ASN1_tag2bit only reads the first 31 entries.
Change-Id: If50770fd79b9d6ccab5558d24b0ee3a27c81a452
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52731
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
The original implementation uses a table generated by a Perl script,
and then relies on some subset of ASN1_STRFLGS_* constants overlapping
with CHARTYPE_* constants, while masking off the ones that don't align.
Allocating ASN1_STRFLGS_* constants is already complex with the
XN_FLAG_* interaction. Avoid the additional CHARTYPE_* interaction by
just writing out what it's recognizing in code. If you ignore
CHARTYPE_PRINTABLESTRING (which is unused), that table is just
recognizing 9 characters anyway.
Also this gets charmap.h out of the way so I can clang-format every file
in here without having to constantly exclude it.
Change-Id: I73f31324e4b8a815887afba459e50ed091a9f999
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52729
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
David Benjamin
Adam Langley
Steven Valdez
Bob Beck