Oh, I think I see the problem. We'll fix this and add a test for it. Thanks for the report! Glad we caught this one quickly.

Some of new logic didn't for ServerHellos being allowed to omit extensions blocks. We'll fix this and add a test for it.

I captured a net-export if you’d like to see it, but since you responded so quickly and sound like you’re able to reproduce it too, I’ll chuck it unless you really want it.

Yup, reproduced and fix ready. Putting together a test since we apparently never had test coverage here, which is embarrassing. :-(

This should fix it:
https://boringssl-review.googlesource.com/c/17904

The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl/+/b853f315dd5b3682bf9d90b2efd007c0c559e142

commit b853f315dd5b3682bf9d90b2efd007c0c559e142
Author: David Benjamin <davidben@google.com>
Date: Fri Jul 14 23:17:40 2017

Fix handling of ServerHellos with omitted extensions.

Due to SSL 3.0 legacy, TLS 1.0 through 1.2 allow ClientHello and
ServerHello messages to omit the extensions field altogether, rather
than write an empty field. We broke this in
https://boringssl-review.googlesource.com/c/17704/ when we needed to a
second ServerHello parsing path.

Fix this and add some regression tests to explicitly test both the
omitted and empty extensions ClientHello and ServerHello cases.

Bug:  chromium:743218 
Change-Id: I8297ba608570238e19f12ea44a9fe2fe9d881d28
Reviewed-on: https://boringssl-review.googlesource.com/17904
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

[modify] https://crrev.com/b853f315dd5b3682bf9d90b2efd007c0c559e142/ssl/handshake_client.cc
[modify] https://crrev.com/b853f315dd5b3682bf9d90b2efd007c0c559e142/ssl/test/runner/handshake_client.go
[modify] https://crrev.com/b853f315dd5b3682bf9d90b2efd007c0c559e142/ssl/test/runner/handshake_messages.go
[modify] https://crrev.com/b853f315dd5b3682bf9d90b2efd007c0c559e142/ssl/test/runner/runner.go
[modify] https://crrev.com/b853f315dd5b3682bf9d90b2efd007c0c559e142/ssl/test/runner/common.go
[modify] https://crrev.com/b853f315dd5b3682bf9d90b2efd007c0c559e142/ssl/test/runner/handshake_server.go

CQ willing, https://chromium-review.googlesource.com/c/572621/ should pull the change into Chromium.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4686061bdb21e7a828c297a818c87708121a798d

commit 4686061bdb21e7a828c297a818c87708121a798d
Author: David Benjamin <davidben@chromium.org>
Date: Sat Jul 15 02:38:32 2017

Roll src/third_party/boringssl/src 52586f952..14308731e

https://boringssl.googlesource.com/boringssl/+log/52586f952ecb624f823eb9ee5ef2eb41e644e7f5..14308731e5446a73ac2258688a9688b524483cb6

This picks up, among other things, the fix to some ServerHello parsing.

Bug:  743218 
Change-Id: I45316af58f675f59ae141f9f1379690010bf04c7
Reviewed-on: https://chromium-review.googlesource.com/572621
Reviewed-by: Adam Langley <agl@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486971}
[modify] https://crrev.com/4686061bdb21e7a828c297a818c87708121a798d/DEPS
[modify] https://crrev.com/4686061bdb21e7a828c297a818c87708121a798d/third_party/boringssl/BUILD.generated.gni
[modify] https://crrev.com/4686061bdb21e7a828c297a818c87708121a798d/third_party/boringssl/linux-x86_64/crypto/cipher_extra/chacha20_poly1305_x86_64.S
[modify] https://crrev.com/4686061bdb21e7a828c297a818c87708121a798d/third_party/boringssl/mac-x86_64/crypto/cipher_extra/chacha20_poly1305_x86_64.S

Awesome. Thanks, David!

Verified this issue on Windows 10, Ubuntu 14.04 and Mac 10.12.5 with chrome #61.0.3159.5, able to connect to https://invest.ameritrade.com/, without observing any SSL errors.Hence adding TE-Verified labels.

Deleted: 743218.png 169 KB

	743218.png 169 KB View Download