1243641 - Increase compatibility of TLS extended master secret, don't send an empty TLS extension last in the handshake

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Masatoshi Kimura [:emk]]

See bug 1224875 comment #5 and onwards for details. Examples of affected servers:
https://apps.reg.uga.edu
https://appserver.lasalle.edu.co
https://correo.uchile.cl

Comment 1

[Martin Thomson [:mt:]]

Attachment: apps.reg.uga.edu-chrome.pcapng.gz

Comment 2

[Martin Thomson [:mt:]]

Attachment: apps.reg.uga.edu-firefox.pcapng.gz

I have no idea why this ClientHello is no good compared to the Chrome one.  There are only a few differences of note: 

 - Chrome has a couple of extra extensions (that won't be used given that the server seems to implement TLS 1.0 with *no* extensions)
 - Chrome has a session ID (probably because it connected before)
 - The order of extensions is different

We don't seem to be crossing any size boundary, Chrome has a bigger Hello.  We include the cipher suite that Chrome ultimately connects with.

Comment 3

[Richard Barnes [:rbarnes]]

I note that the example servers listed above are all based on Tomcat.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Chrome puts the EMS extension much earlier in the Client Hello than Firefox (at least on my linux machine), so I modified NSS to have it earlier in Firefox as well (just after SNI to match). Sure enough, it connected just fine.

Comment 5

[Eric Rescorla (:ekr)]

Did it connect w/ or w/o EMS? I assume w/o?

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Correct - without EMS (and without any extensions, it looks like).

Comment 7

[Matt Wobensmith [:mwobensmith][:matt:]]

More broken sites:

https://grep.elasticpath.com
https://mealapps.flaglerschools.com
https://mypassword.cau.edu
https://www.portaldefacturas.com
https://www.sqlpower.ca

Comment 8

[Eric Rescorla (:ekr)]

Attachment: 0001-Trial-patch.patch

Matt, can you see if this patch helps?

Comment 9

[Matt Wobensmith [:mwobensmith][:matt:]]

I built Nightly today, using your patch, and I'm still seeing the same problem.

However, Keeler mentioned that making this change locally seemed to fix it. 

So, somehow I don't trust my build, or whatever I'm not doing right here.

Keeler, can you see if this change works for you? It should be the same as what you tried in comment 4, yes?

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: 1243641.patch

I think that patch may have been to the wrong section - this is what works for me.

Comment 11

[Eric Rescorla (:ekr)]

Oops, you're right. Try Keeler's

Comment 12

[Matt Wobensmith [:mwobensmith][:matt:]]

Keeler's patch works great. Thank you!

Comment 13

[Matt Wobensmith [:mwobensmith][:matt:]]

While this change will fix these broken sites, does anyone feel that we risk breaking other sites? I'm happy to run a compatibility test, just for this.

Comment 14

[Masatoshi Kimura [:emk]]

(In reply to Matt Wobensmith [:mwobensmith][:matt] from comment #13)
> While this change will fix these broken sites, does anyone feel that we risk
> breaking other sites? I'm happy to run a compatibility test, just for this.

Those site should be already broken with Chrome/IE/Edge. So I don't think we need to care.

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Masatoshi Kimura [:emk] from comment #14)
> (In reply to Matt Wobensmith [:mwobensmith][:matt] from comment #13)
> > While this change will fix these broken sites, does anyone feel that we risk
> > breaking other sites? I'm happy to run a compatibility test, just for this.
> 
> Those site should be already broken with Chrome/IE/Edge. So I don't think we
> need to care.

It would still be good to know. Matt - if you have the time, that would be great.

Comment 16

[Matt Wobensmith [:mwobensmith][:matt:]]

Canary pass is OK, no breakage. Fix looks good.

Comment 17

[Masatoshi Kimura [:emk]]

[Tracking Requested - why for this release]: Firefox fails to connect some servers while other browsers succeed.

I consider to backout bug 1224875 on branches unless NSS takes this fix and backported.

Comment 18

[Masatoshi Kimura [:emk]]

Keeler, could you request review based on comment #16?

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: 1243641-TLS-EMS-reoder.patch

Comment 20

[Martin Thomson [:mt:]]

Comment on attachment 8720382 [details] [diff] [review]
1243641-TLS-EMS-reoder.patch

Review of attachment 8720382 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 22

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/4bc07bbf4b40

Now, does this need uplift?

Comment 23

[Masatoshi Kimura [:emk]]

Not needed if bug 1224875 backout is approved. Otherwise it would be needed.

Comment 24

[Ryan Sleevi]

Sorry I did not chime in sooner - we (Chrome) have encountered a host of compatibility issues when an empty extension is the last extension, which is the case for EMS. Similarly, given the present code, I expect enabling SCTs would also break these same servers, since the client offer is an empty extension. We carry patches in downstream Chrome (not yet updated past NSS 3.21) that explicitly ensure no empty extension is the last extension.

Just something worth noting for if/when Firefox enables SCT support. And to be careful of with TLS 1.3 support.

Comment 25

[Eric Rescorla (:ekr)]

Thanks for the heads-up. The good news is that the 1.3 extensions are non-empty

Comment 26

[Masatoshi Kimura [:emk]]

Could you backport this patch to NSS 3.22 and 3.21 based on your suggestion in bug 1224875?
(Sorry, I don't know about the release management of NSS.)

Comment 27

[Kai Engert [:KaiE:]]

It might be too late for NSS 3.21.x and Firefox 45.

I'd recommend to disable EMS in Firefox 45.

Backporting to a NSS 3.22.x release for Firefox 46 seems more realistic.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Tracking as it is an important regression. We did dot releases in the past for this kind of issues.

Comment 29

[Wan-Teh Chang]

Comment on attachment 8720382 [details] [diff] [review]
1243641-TLS-EMS-reoder.patch

Review of attachment 8720382 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a comment to document this workaround.

::: lib/ssl/ssl3ext.c
@@ +317,5 @@
>   */
>  static const
>  ssl3HelloExtensionSender clientHelloSendersTLS[SSL_MAX_EXTENSIONS] = {
>      { ssl_server_name_xtn,        &ssl3_SendServerNameXtn        },
> +    { ssl_extended_master_secret_xtn,       &ssl3_SendExtendedMasterSecretXtn},

We should add a comment to point out the importance of
listing this extension here (the last extension cannot
be empty), with known broken servers (Tomcat), their
versions if known, and this bug report. A future NSS
maintainer should have easy access to this info to determine
if this interop workaround is no longer necessary.

Nit: there are too many spaces before "&ssl3_SendExtendedMasterSecretXtn".

Comment 30

[Sylvestre Ledru [:Sylvestre]]

We disabled the change in 45.

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

From comment 23, it sounds like we need to uplift this to 46. If that sounds right, can you request uplift to aurora? Thanks.

Comment 32

[Dana Keeler (she/her) (use needinfo) [:keeler]]

My understanding is for this to be uplifted to 46, we'll either need to uplift NSS 3.23 to 46 or we'll need to land this on whatever NSS version is in 46 and update that. (Either way, I imagine it will involve at least one other bug.) Kai, I know you have a lot on your plate right now, but can you weigh in on the best approach here? (Note that there's also Wan-Teh's request in comment 29 to add a comment regarding the workaround.)

Comment 33

[Masatoshi Kimura [:emk]]

I thought NSS 3.22.3 would take this per comment #27. If not (and NSS 3.23 would not uplift either), I'll request backout again in bug 1224875.

Comment 34

[Kai Engert [:KaiE:]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #32)
> (Note that there's also Wan-Teh's
> request in comment 29 to add a comment regarding the workaround.)

David, can you please add a patch with the suggested comment?

Comment 35

[Kai Engert [:KaiE:]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #32)
> My understanding is for this to be uplifted to 46, we'll either need to
> uplift NSS 3.23 to 46 or we'll need to land this on whatever NSS version is
> in 46 and update that.

Correct.


> Kai, I know you have a lot on your plate right now, but can you
> weigh in on the best approach here?

For me, it's easiest, if you can simply upgrade Firefox 46 to use NSS 3.23.
But it's up to you, if you want it, and if you can get approvals.

But you'd have to check with Kathleen, whether or not the NSS 3.23 root CA changes are allowed to be shipped with Firefox 46.

If you want to use NSS 3.23, please help by obtaining an Aurora+ approval by Friday, so it can be landed prior to next week's Aurora/Beta branch uplifts.

If you prefer just the minimal change, we'll have to create a NSS 3.22.3 release. I'm willing to help with making that release, if preferred. If that's what you decide, then for timing reasons, I'd prefer to make that release after March 8, and seek an beta+ approval for that after March 8.

Comment 36

[Kai Engert [:KaiE:]]

Attachment: aurora46-to-nss-3.23.txt

(In reply to Kai Engert (:kaie) from comment #35)
> If you want to use NSS 3.23, please help by obtaining an Aurora+ approval by
> Friday, so it can be landed prior to next week's Aurora/Beta branch uplifts.

I'm attaching a placeholder, that you can use to request aurora approval, if you want to go this route, and if Kathleen is OK with it.

Comment 37

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie) from comment #34)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #32)
> > (Note that there's also Wan-Teh's
> > request in comment 29 to add a comment regarding the workaround.)
> 
> David, can you please add a patch with the suggested comment?

It would be good to get that done today, because I had planned to tag the final NSS 3.23 release today.

Comment 38

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: 1243641-comment.patch

Hi Kai, here's the comment patch.
As for uplifting 3.23 vs. creating a 3.22.3 branch, I think it would be safest to go with the latter and uplifting it to beta next week. I'll open a bug for that.

Comment 39

[Kai Engert [:KaiE:]]

Comment on attachment 8725757 [details] [diff] [review]
1243641-comment.patch

comment checked in for NSS 3.24
https://hg.mozilla.org/projects/nss/rev/bbd0fdb260f1

Comment 40

[Kai Engert [:KaiE:]]

Attachment: backport-1243641-to-3.22.x.patch

David requested for this to be backport to NSS 3.22.x

Since there's a slight context difference (amount of entries in the struct), let's do the formal approach and get an r+ on the backport patch.

Comment 41

[Martin Thomson [:mt:]]

Comment on attachment 8729546 [details] [diff] [review]
backport-1243641-to-3.22.x.patch

Review of attachment 8729546 [details] [diff] [review]:
-----------------------------------------------------------------

This discovery made me sad,  but I was already there. Java.

Comment 42

[Kai Engert [:KaiE:]]

Comment on attachment 8729546 [details] [diff] [review]
backport-1243641-to-3.22.x.patch

3.22.x branch for 3.22.3

https://hg.mozilla.org/projects/nss/rev/a55ab5e1258c

Comment 43

[Dana Keeler (she/her) (use needinfo) [:keeler]]

The NSS update in bug 1252930 has landed on 46 and 47, so we're good here.

Comment 44

[Masatoshi Kimura [:emk]]

Removing the stale ni.