| Age | Commit message (Collapse) | Author |
|---|
|
|
|
While it isn't the case for the default implementations, custom DH and DSA
methods could conceivably populate private and public keys, which in turn
would result in leaks in the pub/priv decode methods.
ok jsing
|
|
ok jsing
|
|
ok jsing
|
|
Due to OPENSSL_NO_ENGINE the engine member of dh and dsa is currently
uninitialized. As a consequence, {DH,DSA}_get0_engine() will return a
garbage pointer, which is particularly bad because the only reason we
kept them in the first place is that they are used by some software...
A side effect of freeing with {DH,DSA}_free() instead of a hand-rolled
version is that we may call ->meth->finish() before ->meth->init() was
called. We need a NULL check for ->meth to be on the safe side in case
we should need to bring ENGINE back.
with nits from djm
ok deraadt djm
|
|
Use aint for the ASN1_INTEGER holding the key and astr for the ASN1_STRING
holding the parameters. This frees up key and params for their DER encoded
versions, matching the naming we use elsewhere much more closely.
ok jsing
|
|
|
|
ok jsing
|
|
|
|
This adds some missing error checks and fixes and unifies error codes
which were (as usual) all over the place or just plain nonsense. Use
an auxiliary variable for d2i invocations even though it is not really
needed here.
ok jsing
|
|
Use the same variable names throughout these functions and unify them
some more.
ok jsing
|
|
This brings these two messy functions into more usual shape. There is a
lot more that can be done in here. It is a step in the right direction.
ok jsing
|
|
Avoid creating an ASN1_STRING with negative length, set type, data
and length via ASN1_STRING_type_new() and ASN1_STRING_set0() instead
of doing this manually. Check return value for i2d_ASN1_INTEGER()
and use an intermediate ASN1_OBJECT instead of nested function calls.
Finally, clear sensitive data with freezero().
ok jsing
|
|
Provide bn_rand_in_range() which is a slightly tweaked version of what was
previously called bn_rand_range().
The way bn_rand_range() is called in libcrypto, the lower bound is always
expressible as a word. In fact, most of the time it is 1, the DH code uses
a 2, the MR tests in BPSW use 3 and an exceptinally high number appears in
the Tonelli-Shanks implementation where we use 32. Converting these lower
bounds to BIGNUMs on the call site is annoying so let bn_rand_interval()
do that internally and route that through bn_rand_in_range(). This way we
can avoid using BN_sub_word().
Adjust the bn_isqrt() test to use bn_rand_in_range() since that's the
only caller that uses actual BIGNUMs as lower bounds.
ok jsing
|
|
ok tb@
|
|
This removes lots of silly buffers and will allow us to make this API
go away.
ok jsing
|
|
from jsing
|
|
Requested by jsing
|
|
Some headers were included conditionally on OPENSSL_NO_DEPRECATED in hopes
that eventually the mess of everything includes everything will magically
resolve itself. Of course everyone would end up building openssl with
OPENSSL_NO_DEPRECATED over time... Right.
Surprisingly, the ecosystem has come to rely on these implicit inclusions,
so about two dozen ports would fail to build because of this. Patching this
would be easy but really not worth the effort.
ok jsing
|
|
(experts disagree whether they ever did)
|
|
Discussed with jsing
|
|
Geoff Thorpe added OPENSSL_NO_DEPRECATED nearly two decades ago. The hope
was that at some point some functions can be dropped. Most of the functions
marked deprecated are actually unused nowadays but unfortunately some of
them are still used in the ecosystem. Move them out of OPENSSL_NO_DEPRECATED
so we can define it without breaking the consumers in the next bump.
ERR_remove_state() is still used by a dozen or so ports. This isn't a big
deal since it is just a stupid wrapper for the not quite as deprecated
ERR_remove_thread_state(). It's not worth patching these ports.
Annoyingly, {DH,DSA}_generate_parameters() and RSA_generate_key() are still
used. They "make use" of the old-style BN_GENCB callback, which is therefore
more difficult to remove - in case you don't know know: that's the thing
responsible for printing pretty '.', '+' and '*' when you generate keys.
Most annoyingly, DH_generate_parameters() was added to rust-openssl in 2020
for "advanced DH support". This is very unfortunate since cargo bundles a
rust-openssl and updates it only every few years or so. As a consequence
we're going to be stuck with this nonsense for a good while.
ok beck jsing
|
|
ok jsing
|
|
This is currently pulled in via dsa.h and ecdsa.h, but only when
OPENSSL_NO_DEPRECATED is not defined. We should fix this in the
public header, too - let's wait a bit with that.
|
|
The private key is a random number in [1, q-1], so 1 must be allowed.
Since q is at least an 160-bit prime and 2^159 + 1 is not prime (159
is not a power of 2), the probability that this is hit is < 2^-159,
but a tiny little bit wrong is still wrong.
Found while investigating a report by bluhm
ok jsing
|
|
BN_clear_free() is a wrapper that calls BN_free() - call BN_free() directly
instead.
ok tb@
|
|
We have long had expensive checks for DSA domain parameters in
old_dsa_priv_decode(). These were implemented in a more complicated
way than necesary.
ok beck jsing
|
|
The DSA standard specifies an infinite loop: if either r or s is zero
in the signature calculation, a new random number k shall be generated
and the whole thing is to be redone. The rationale is that, as the
standard puts it, "[i]t is extremely unlikely that r = 0 or s = 0 if
signatures are generated properly."
The problem is... There is no cheap way to know that the DSA domain
parameters we are handed are actually DSA domain parameters, so even
if all our calculations are carefully done to do all the checks needed,
we cannot know if we generate the signatures properly. For this we would
need to do two primality checks as well as various congruences and
divisibility properties. Doing this easily leads to DoS, so nobody does
it.
Unfortunately, it is relatively easy to generate parameters that pass
all sorts of sanity checks and will always compute s = 0 since g
is nilpotent. Thus, as unlikely as it is, if we are in the mathematical
model, in practice it is very possible to ensure that s = 0.
Read David Benjamin's glorious commit message for more information
https://boringssl-review.googlesource.com/c/boringssl/+/57228
Thanks to Guido Vranken for reporting this issue, also thanks to
Hanno Boeck who apparently found and reported similar problems earlier.
ok beck jsing
|
|
Explicitly check against NULL and turn early return into goto err.
ok beck jsing
|
|
We already had some checks on both sides, but they were less precise
and differed between the functions. The code here is messy enough, so
any simplification is helpful...
ok beck jsing
|
|
When decoding a public or a private key, use dsa_check_key() to ensure
consistency of the DSA parameters. We do not always have sufficient
information to do that, so this is not always possible.
This adds new checks and replaces incomplete existing ones. On decoding
the private key we will now only calculate the corresponding public key,
if the sizes are sensible. This avoids potentially expensive operations.
ok beck jsing
|
|
This is a cheap check that ensures basid parameter consistency per
FIPS 186-4: 1 < g < q, that q has the allowed bit sizes 160, 224, 256
and that p is neither too small nor too large. Unfortunately, enforcing
the three allowed sizes for p is not possible since the default dsa key
generation has not respected this limitation.
Instead of checking that p and q are prime, we only check that they
are odd. Check that public and private keys, if set, are in the proper
range. In particular, disallow zero values.
Various versions of these checks have been added to the dsa code
over time. This consolidates and extends them and in a subsequent
commit wewill replace the incomplete checks. BoringSSL has a similar
function of the same name, thanks to David Benjamin for pointing it
out.
ok beck jsing
|
|
This has been missing for a while already and will be used in a
few upcoming commits.
ok beck jsing
|
|
discussed with jsing
|
|
|
|
|
|
This adds missing BN_CTX_start()/BN_CTX_end() calls, removes NULL checks
before BN_CTX_end()/BN_CTX_free() (since they're NULL safe) and calls
BN_free() instead of BN_clear_free() (which does the same thing).
Also replace stack allocated BIGNUMs with calls to BN_CTX_get(), using the
BN_CTX that is already available.
ok tb@
|
|
Rather than having complicated "attempt to reuse" dances, always allocate
priv_key/pub_key, then free and assign on success.
ok tb@
|
|
Libcrypto currently has a mess of *_lcl.h, *_locl.h, and *_local.h names
used for internal headers. Move all these headers we inherited from
OpenSSL to *_local.h, reserving the name *_internal.h for our own code.
Similarly, move dtls_locl.h and ssl_locl.h to dtls_local and ssl_local.h.
constant_time_locl.h is moved to constant_time.h since it's special.
Adjust all .c files in libcrypto, libssl and regress.
The diff is mechanical with the exception of tls13_quic.c, where
#include <ssl_locl.h> was fixed manually.
discussed with jsing,
no objection bcook
|
|
Since DSA_sign() and DSA_verify() ignore their type argument, don't bother
to determine it here. Check all size_t for overflow before passing them as
int arguments. Follow OpenSSL and add a check to see if the tbs blob's
length matches the one of the md, in case it is set on the EVP_PKEY_CTX.
Fix return value check of DSA_sign().
ok jsing
|
|
Change DSA_sign() to single exit and check the signed i2d_DSA_SIG() return
value before assigning it to an unsigned int.
In DSA_verify() let d2i_DSA_SIG() handle the allocation, split error check
of i2d_DSA_SIG() from signature check and change an unnecessary freezero()
to free.
ok jsing
|
|
|
|
ok jsing@
|
|
Found with CodeChecker
ok jsing@
|
|
CBIGNUM_it is supposed to be the "clear bignum" or "secure" bignum - that
is one which zeros its memory after use and ensures that the constant time
flags are set... in LibreSSL we always do both of these things for BIGNUMs,
so just use BIGNUM_it instead.
ok tb@
|
|
|
|
DSA_size() and ECDSA_size() have a very special hack. They fudge up an
ASN1_INTEGER with a size which is typically > 100 bytes, backed by a
buffer of size 4. This was "fine", however, since they set buf[0] = 0xff,
where the craziness that was i2c_ASN1_INTEGER() only looks at the first
octet (one may then ask why a buffer of size 4 was necessary...).
This changed with the rewrite of i2c_ASN1_INTEGER(), which doesn't
respect this particular hack and rightly assumes that it is fed an
actual ASN1_INTEGER...
Instead, create an appropriate signature and use i2d to determine its
size.
Fixes an out-of-bounds read flagged by ASAN and oss-fuzz.
ok jsing
|
|
This script is not used at all and files are edited by hand instead.
Thus remove misleading comments incl. the obsolete script/config.
Feedback OK jsing tb
|
|
ok tb@
|
|
These are mostly security-level related, but there are also ASN1_TIME
and ASN_INTEGER functions here, as well as some missing accessors.
ok jsing
|
