Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello

On Tue, Nov 3, 2015 at 11:16 AM Matt Caswell via RT wrote: > Whilst investigating this I noticed another bug which is actually > probably more significant. My eyeball only look at the BoringSSL source > suggests that it is there too, so I'm not sure why you haven't seen it > in

Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello

On Tue, Nov 3, 2015 at 12:42 PM David Benjamin wrote: > I'm not sure that fix quite works though. If BIO_flush completes > asynchronously (hrm, it's missing an rwstate update), then I believe you'll > be in a state where you *do* want to repeat the init_off / init_num

Re: [openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello

On Wed, Nov 4, 2015 at 7:04 AM Matt Caswell via RT <r...@openssl.org> wrote: > > > On 03/11/15 17:43, David Benjamin via RT wrote: > > > I'm not sure that fix quite works though. If BIO_flush completes > > asynchronously > > Ahhh, yes good point. Updated patc

[openssl-dev] [openssl.org #4119] DTLS resets handshake hash too frequently for ClientHello

Hey folks, We found a small DTLS bug while writing some tests. I think it affects 1.0.1 and 1.0.2 too, so I thought I'd send you a note. (Note sure about master. I'm unfamiliar with the new state machine mechanism.) In DTLS, each ClientHello is supposed to reset the handshake hash (in case of

[openssl-dev] [openssl.org #4120] CertificateStatus message is optional

It seems unlikely I'll be getting around to doing another newsletter, but while I'm reporting bugs, here's another that came to mind: RFC 6066 is somewhat obnoxious and allows the server to decline to send CertificateStatus even after negotiating the extension.

[openssl-dev] [openssl.org #4184] Memory leak in DSA redo case

dsa_do_sign retries the operation if |r| or |s| end up zero. This results in leaking the first iteration's value of |ret| since you end up clobbering the previous allocation.

[openssl-dev] [openssl.org #4185] Bug in EVP_MD_CTX_copy_ex's malloc failure handling

EVP_MD_CTX_copy_ex is implemented with memcpy, followed by manually fixing up |out->pctx| and |out->md_data|. https://git.openssl.org/gitweb/?p=openssl.git;a=blob;f=crypto/evp/digest.c;h=5da0e01039a6da039942db9f1bf8b70753f509f2;hb=HEAD#l292 If allocating |out->md_data| fails, then both

Re: [openssl-dev] [openssl.org #4185] Bug in EVP_MD_CTX_copy_ex's malloc failure handling

On Thu, Dec 17, 2015 at 2:43 PM Kurt Roeckx via RT <r...@openssl.org> wrote: > On Wed, Dec 16, 2015 at 11:34:56PM +0000, David Benjamin via RT wrote: > > EVP_MD_CTX_copy_ex is implemented with memcpy, followed by manually > fixing > > up |out->pctx| and |out

Re: [openssl-dev] [openssl.org #4558] Performance issue with DTLS packet reassembly

On Mon, Jun 13, 2016 at 4:04 AM Matt Caswell via RT wrote: > On Thu Jun 02 23:24:44 2016, paul.d...@oracle.com wrote: > > The DTLS packet reassembly code has a performance problem that could > > result in a DoS attack being possible. > > > > > > > > The DTLS packet reassembly

[openssl-dev] [openssl.org #4572] SSL_set_bio and friends

I recently made some changes around BoringSSL's SSL_set_bio, etc. which you all might be interested in. The BIO management has two weird behaviors right now: 1. The existence of bbio is leaked in the public API when it should be an implementation detail. (Otherwise you're stuck with it for DTLS

Re: [openssl-dev] [openssl.org #4362] chacha-x86.pl has stricter aliasing requirements than other files

I don't think that will work. The SSL code uses in-place buffers extensively, so in == out definitely needs to be defined. The question is only whether out < in is also acceptable. Either way, for BoringSSL, I've gone ahead and tightened our aliasing constraints to forbid out < in and require

[openssl-dev] [openssl.org #4577] X509_LU_RETRY and X509_LU_FAIL are slightly confused

Hey folks, It seems the X509_LU_* values (now an X509_LOOKUP_TYPE enum in master) are slightly confused. See this commit message and diff for details: https://boringssl.googlesource.com/boringssl/+/da7f0c65efb72556f8fc92e460e6c90cd1b1add7%5E%21/ The relevant point is that X509_LU_RETRY doesn't

Re: [openssl-dev] [openssl.org #4572] SSL_set_bio and friends

On Fri, Jun 17, 2016 at 8:48 AM Matt Caswell via RT <r...@openssl.org> wrote: > > > On 14/06/16 21:30, David Benjamin via RT wrote: > > For OpenSSL master, I believe it'd also work to add an s->rbio != s->wbio > > check to SSL_set_rbio, but I think those are wors

[openssl-dev] [openssl.org #4278] DH_CHECK_PUBKEY_INVALID should be 0x4, not 0x3

The recently-added DH_CHECK_PUBKEY_INVALID was set to 0x3, but DH_CHECK_PUBKEY_* values are flags, so it should be 0x4 to avoid colliding with DH_CHECK_PUBKEY_TOO_SMALL (0x01) and DH_CHECK_PUBKEY_TOO_LARGE (0x02). See DH_check_pub_key's *ret |= logic.

[openssl-dev] [openssl.org #4277] DSAPublicKey should use dsa_cb in 1.1.0

DSAPublicKey lost the dsa_cb in https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=ea6b07b54c1f8fc2275a121cdda071e2df7bd6c1 This results in d2i_DSAPublicKey using crypto/asn1's default allocation logic rather than calling into DSA_new. I believe it should use ASN1_SEQUENCE_cb. I've

[openssl-dev] [openssl.org #4341] [PATCH] Consistently use arm_arch.h constants in armcap assembly code.

Patch attached. This is just a little cleanup change to fix not everything using the OPENSSL_armcap constants. (Existing ones already are using them, so I'm assuming this is okay.) David -- Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4341 Please log in as guest with password guest

Re: [openssl-dev] [openssl.org #4323] chacha-armv4.pl bugs

On Sun, Feb 21, 2016 at 3:27 PM Andy Polyakov via RT wrote: > Hi, > > > The partial-block tail code in chacha-armv4.pl also seems to have > problems. > > My colleague Steven and I made an attempt to debug it, but we're not > > familiar enough with ARM to fix it. > > > > From

Re: [openssl-dev] [openssl.org #4346] poly1305-x86.pl's AVX2 code

On Fri, Feb 26, 2016 at 4:48 PM Andy Polyakov via RT wrote: > > There seems to be a bug in the AVX2 codepath in poly1305-x86.pl. I have > not > > attempted to debug this, but I have attached a test file which produces > > different output in normal and AVX2 codepaths. Our

[openssl-dev] [openssl.org #4346] Re: poly1305-x86.pl's AVX2 code

On Thu, Feb 25, 2016 at 6:16 PM David Benjamin wrote: > From looking at valgrind, this pattern seems to give good coverage. I > used valgrind --tool=callgrind --dump-instr=yes --collect-jumps=yes and > then kcachegrind to inspect the output. (kcachegrind is a bit heavy for >

[openssl-dev] [openssl.org #4346] poly1305-x86.pl's AVX2 code

There seems to be a bug in the AVX2 codepath in poly1305-x86.pl. I have not attempted to debug this, but I have attached a test file which produces different output in normal and AVX2 codepaths. Our existing poly1305 implementation agrees with the former. $ OPENSSL_ia32cap=0 ./poly1305_test PASS

[openssl-dev] [openssl.org #4323] chacha-armv4.pl bugs

Hi Andy, The partial-block tail code in chacha-armv4.pl also seems to have problems. My colleague Steven and I made an attempt to debug it, but we're not familiar enough with ARM to fix it. >From playing with it in a debugger, it doesn't look like @t[3] contains the length. We suspect something

[openssl-dev] [openssl.org #4305] ChaCha20 assembly bugs

Hi folks, I've started playing with the ChaCha20 assembly that was recently checked in and found a few problems. Most of these do not affect OpenSSL as you only ever call ChaCha20_ctr32 on a whole number of blocks. But this isn't documented as a constraint in internal/chacha.h and the assembly

[openssl-dev] [openssl.org #4362] chacha-x86.pl has stricter aliasing requirements than other files

I'm unclear on what EVP_CIPHER's interface guarantees are, but our EVP_AEAD APIs are documented to allow in/out buffers to alias as long as out is <= in. This matches what callers might expect from a naive implementation. Our AES-GCM EVP_AEADs, which share code with OpenSSL, have tended to match

[openssl-dev] [openssl.org #4364] [PATCH] ASN1_get_object should not accept large universal tags.

See attached. OpenSSL can't actually represent large universal tags because it collides with the V_ASN1_NEG flag, yet it happily parses them in high tag number form. d2i_ASN1_TYPE interprets 1f82020100 as a negative zero, rather than an element with tag [UNIVERSAL 258]. I've intentionally made

Re: [openssl-dev] [openssl.org #4362] chacha-x86.pl has stricter aliasing requirements than other files

The current state is that, as far as I can tell, overlapping requirements are undocumented (or is it somewhere and I missed it?) and, for ChaCha, architecture-specific. I think something certainly needs to be done. Either changing chacha-x86.pl and allowing any out <= in overlap, or declaring that

Re: [openssl-dev] [openssl.org #4401] [PATCH] plug potential memory leak(s) in OpenSSL 1.1 pre 4 in 'ec_lib.c'

By the way, returning the original subject, I don't believe there is a leak here. If EC_GROUP_copy fails, dest still exists and is owned by the caller. It's the caller's obligation to call EC_GROUP_free and that will release the partially-copied EC_GROUP. (Which will, with this patch, cause a

Re: [openssl-dev] [openssl.org #4483] Wrong results with Poly1305 functions

On Tue, Mar 29, 2016 at 9:47 AM Andy Polyakov via RT wrote: > > In the non-SIMD paths, I believe this is fine because $r0's and $r1's > > cleared high bits mean we should have plenty of slack to leave that > > unreduced. (And indeed its normally not reduced on input from the >

Re: [openssl-dev] [openssl.org #4439] poly1305-x86.pl produces incorrect output

On Sun, Mar 20, 2016 at 5:05 PM Andy Polyakov via RT wrote: > No, it doesn't depend on call pattern. Please confirm that attached > patch solves the problem. Thanks. > (Right, sorry, I meant that the test vectors I have seem to only with their corresponding call patterns.)

Re: [openssl-dev] [openssl.org #4439] poly1305-x86.pl produces incorrect output

On Sun, Mar 20, 2016 at 10:47 PM David Benjamin wrote: > On Sun, Mar 20, 2016 at 5:05 PM Andy Polyakov via RT > wrote: > >> No, it doesn't depend on call pattern. Please confirm that attached >> patch solves the problem. Thanks. >> > > (Right, sorry, I

Re: [openssl-dev] [openssl.org #4483] Wrong results with Poly1305 functions

On Fri, Mar 25, 2016 at 3:07 PM Andy Polyakov via RT wrote: > > For x86-64, this seems to be the bug: > > > > $ git diff > > diff --git a/crypto/poly1305/asm/poly1305-x86_64.pl > b/crypto/poly1305/asm/ > > poly1305-x86_64.pl > > index 3c810c5..bc14ed1 100755 > > ---

Re: [openssl-dev] [openssl.org #4483] Wrong results with Poly1305 functions

On Fri, Mar 25, 2016 at 3:26 PM David Benjamin wrote: > Right. What I meant is that a fully reduced h has $h2 < 4. Is it possible > that $h2, after that adc, ends up at 4, exceeding that bound? If it were, > that would require one more reduction. > > In the non-SIMD paths, I

[openssl-dev] [openssl.org #4439] poly1305-x86.pl produces incorrect output

Hi folks, You know the drill. See the attached poly1305_test2.c. $ OPENSSL_ia32cap=0 ./poly1305_test2 PASS $ ./poly1305_test2 Poly1305 test failed. got: 2637408fe03086ea73f971e3425e2820 expected: 2637408fe13086ea73f971e3425e2820 I believe this affects both the SSE2 and AVX2 code. It does

Re: [openssl-dev] [openssl.org #4439] poly1305-x86.pl produces incorrect output

On Thu, Mar 17, 2016 at 5:22 PM David Benjamin via RT <r...@openssl.org> wrote: > I'm probably going to write something to generate random inputs and stress > all your other poly1305 codepaths against a reference implementation. I > recommend doing the same in your own test harnes

[openssl-dev] [openssl.org #4460] [PATCH] BIO_METHODs should be const

Patch attached. This is a mechanical change. BIO_new takes a non-const BIO_METHOD and the various BIO_METHODs defined in the library are also non-const, so they don't get placed in .rodata. The change to BIO_new and the BIO struct should be source-compatible. Fixing the in-library BIO_METHODs is

Re: [openssl-dev] [openssl.org #4483] Wrong results with Poly1305 functions

For x86-64, this seems to be the bug: $ git diff diff --git a/crypto/poly1305/asm/poly1305-x86_64.pl b/crypto/poly1305/asm/ poly1305-x86_64.pl index 3c810c5..bc14ed1 100755 --- a/crypto/poly1305/asm/poly1305-x86_64.pl +++ b/crypto/poly1305/asm/poly1305-x86_64.pl @@ -97,6 +97,7 @@ $code.=<<___;

Re: [openssl-dev] [openssl.org #4394] OpenSSL 1.1.0 state machine can't read handshake headers async

On Mon, Mar 7, 2016 at 5:08 PM David Benjamin via RT <r...@openssl.org> wrote: > No patch for this one since I'm not that familiar with your state machine. > If the peer sends handshake messages fragmented across records such that > the handshake message header is split o

[openssl-dev] [openssl.org #4388] [PATCH] Don't be sensitive to the order of ALPN and NPN.

If the server consumer configures NPN and not ALPN, OpenSSL should still resolve NPN against clients which advertises it. (NB: Chrome will be removing NPN soon, so hopefully there won't be any such consumers.) Losing the alpn_select_cb check makes OpenSSL depend on whether ALPN or NPN comes first

[openssl-dev] [openssl.org #4392] [PATCH] Resolve DTLS cookie and version before session resumption.

Session resumption involves a version check, so version negotiation must happen first. Currently, the DTLS implementation cannot do session resumption in DTLS 1.0 because the ssl_version check always checks against 1.2. Switching the order also removes the need to fixup ssl_version in DTLS

[openssl-dev] [openssl.org #4394] OpenSSL 1.1.0 state machine can't read handshake headers async

No patch for this one since I'm not that familiar with your state machine. If the peer sends handshake messages fragmented across records such that the handshake message header is split over two records AND the two records are received in different steps asynchronously, OpenSSL fails to reassemble

[openssl-dev] [openssl.org #4391] [PATCH] Tighten up logic around ChangeCipherSpec.

ChangeCipherSpec messages have a defined value. They also may not occur in the middle of a handshake message. The current logic will accept a ChangeCipherSpec with value 2. It also would accept up to three bytes of handshake data before the ChangeCipherSpec which it would discard (because

[openssl-dev] [openssl.org #4393] [PATCH] Call EC_GROUP_order_bits in priv2opt.

The private key is a scalar and should be sized by the order, not the degree. (Unlike my other recent emails, this has nothing to do with BoringSSL tests. :-) ) David -- Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4393 Please log in as guest with password guest if prompted

[openssl-dev] [openssl.org #4387] [PATCH] Fix V2ClientHello handling

The V2ClientHello code creates an empty compression list, but the compression list must explicitly contain the null compression (and later code enforces this). As a result, all V2ClientHellos currently get rejected on master. The SendV2ClientHello-Sync test in BoringSSL's test suite can be used

[openssl-dev] [openssl.org #4390] [PATCH] Don't send signature algorithms when client_version is below TLS 1.2.

Per RFC 5246, Note: this extension is not meaningful for TLS versions prior to 1.2. Clients MUST NOT offer it if they are offering prior versions. However, even if clients do offer it, the rules specified in [TLSEXT] require servers to ignore extensions they do not understand.

[openssl-dev] [openssl.org #4395] OpenSSL doesn't reject out-of-context empty records

ssl3_get_record silently discards empty records without much context, which means OpenSSL will happily accept, e.g., empty app data records mid-handshake or empty records of bogus type. They get silently discarded and never returned to the caller, so this is harmless, just a little odd. This is

[openssl-dev] [openssl.org #4389] [PATCH] The NewSessionTicket message is not optional.

Per RFC 4507, section 3.3: This message [NewSessionTicket] MUST be sent if the server included a SessionTicket extension in the ServerHello. This message MUST NOT be sent if the server did not include a SessionTicket extension in the ServerHello. The presence of the NewSessionTicket

Re: [openssl-dev] [openssl.org #4393] [PATCH] Call EC_GROUP_order_bits in priv2opt.

On Tue, Mar 29, 2016 at 12:17 PM Emilia Käsper wrote: > While we're at this, shouldn't we then also check the length in oct2priv? > (And > either reject or reduce mod n.) Afaics it accepts arbitrary BNs currently, > which means some keys can be parsed but cannot be re-encoded?

Re: [openssl-dev] [openssl.org #4623] OpenSSL master regression in handling malformed Client Key Exchange messages in RSA key exchange

On Fri, Jul 22, 2016 at 7:30 PM Hubert Kario via RT wrote: > On Friday, 22 July 2016 17:14:43 CEST Stephen Henson via RT wrote: > > On Fri Jul 22 14:56:11 2016, hka...@redhat.com wrote: > > > the issue is present in master 0ed26acce328ec16a3aa and looks to have > > > been > > >

Re: [openssl-dev] [openssl.org #4572] SSL_set_bio and friends

On Fri, Jul 29, 2016 at 9:21 AM Matt Caswell via RT wrote: > On Tue Jun 14 20:30:09 2016, david...@google.com wrote: > > I recently made some changes around BoringSSL's SSL_set_bio, etc. > > which you > > all might be interested in. The BIO management has two weird behaviors >

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

On Sun, Jul 31, 2016 at 6:18 PM Michel via RT wrote: > > I was able to trigger a crash simply by chaining an encrypt BIO with a > memory BIO containing a large plaintext and then stream 100 bytes out of it > at a time. BIO_read would consistently return 128 and, by the time the

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

Hey folks, I do not believe this fix works and introduces buffer overflows. Looking at this change: https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=abdb460d8abe68fedcf00b52d2ba4bf4b7c1725c It makes EVP_CipherUpdate write to out directly. While not unreasonable (this BIO probably

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

I may not have time to fully digest the change before the release date, but I'm not sure this snippet quite works: if (ctx->read_start == ctx->read_end) { /* time to read more data */ ctx->read_end = ctx->read_start = &(ctx->buf[BUF_OFFSET]); ctx->read_end += BIO_read(next,