978831 - Do not allow the server to change certificates during TLS renegotiation

Status: NEW

(Core :: Security: PSM, defect, P3)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Our UI presents certificates as if there is only one certificate possibly used on the connection. However, the TLS protocol allows, and we support, the server to use different certificates during renegotiations. Thus, the UI doesn't map well to what is happening underneath.

Recently, to work around a harder-to-fix bug in their implementation, the Chrome team implemented a workaround wherein they prevented the server from changing certificates. Also, that workaround is suggested at https://secure-resumption.com/#countermeasures. Also that restriction is recommended as part of RFC 5746: https://tools.ietf.org/html/rfc5746#section-5.

According to rsleevi and wtc, there haven't been any known compatibility issues with their deployment of the restriction, though as of last Thursday they were planning to remove it. Since it doesn't cause any compatibility issues and because it simplifies UI issues and because it is good defense-in-depth, we should implement the restriction now, even if Chrome decides to remove it.

Comment 1

[Michael Holloway [:mdholloway]]

It looks like there's an existing BlockServerCertChangeForSpdy function that could be modified to reject a changed certificate on renegotation regardless of protocol.  If I'm on the right track here (and your thinking on this hasn't changed since this was filed) let me know and I'll put in a patch!

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Yes, that's right.

Comment 3

[Michael Holloway [:mdholloway]]

Attachment: proposed patch

I've attached a proposed patch.  The tricky part has been finding a way of testing it; I tried intentionally misconfiguring a server along these lines (https://code.google.com/p/chromium/issues/detail?id=360406#c26) for testing purposes but couldn't find a setup Apache would accept that would allow me to produce a different server cert for renegotiation.  

At the very least, there are no apparent ill effects when browsing normally with the patch applied and signing in/out of secure sites.  

Is there a set of tests I can run this against, either locally or on the try server?

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8703372 [details] [diff] [review]
proposed patch

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

Looks good to me, but I've been away from this code for a long time so somebody else will also have to look at it.

::: security/manager/ssl/SSLServerCertVerification.cpp
@@ +781,1 @@
>                               CERTCertificate* serverCert)

NIT: Fix the indention. If it all fits on one line <= 80 characters, then move it onto one line. Otherwise, adjust the alignment of the second line.

@@ +1501,5 @@
>  
>    Time now(Now());
>    PRTime prnow(PR_Now());
>  
> +  if (BlockServerCertChange(socketInfo, serverCert) != SECSuccess)

NIT: The old code's style was wrong. There should be braces around the "return SECFailure;" Please fix this while you are making your change.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Michael Holloway [:mdholloway] from comment #3)
> The tricky part has been finding a way of
> testing it; I tried intentionally misconfiguring a server along these lines
> (https://code.google.com/p/chromium/issues/detail?id=360406#c26) for testing
> purposes but couldn't find a setup Apache would accept that would allow me
> to produce a different server cert for renegotiation.

Ideally, there would be an automated test using PSM automated testing framework. However, I've forgotten so much about it that somebody else will have to help you with that part.

Anyway, I think this is a good change to make. Google Chrome does (did, at one point, anyway) do the same.

Comment 6

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

The main test server we use is implemented (mostly) here:

https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/tlsserver/lib/TLSServer.cpp

You would need to modify that to have an additional host that has renegotiation enabled and uses a different certificate when the client actually performs a renegotiation. Then, I would add a new xpcshell test in security/manager/ssl/tests/unit/ and do something like this:

function run_test() {
  do_get_profile();
  add_tls_server_setup("BadCertServer", "bad_certs");
  add_connection_test("renegotiation-with-different-cert.example.com", PRErrorCodeSuccess);
  // somehow ensure the second connection attempts renegotiation (we may be clearing session information in between calls to add_connection_test, so this may not work without modification)
  add_connection_test("renegotiation-with-different-cert.example.com", SSL_ERROR_RENEGOTIATION_NOT_ALLOWED);
  run_next_test();
}

You'll also (probably) need to add at least one more certificate in https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/bad_certs

That's a high level of what I think it would take to test this. Let me know if I need to go into more detail on anything. Thanks for working on this, and don't hesitate to reach out if I can be of assistance.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Dana, do you know if NSS actually allows the certificate to be changed during renegotiation? That's the main thing I'm unsure about.

Comment 8

[Michael Holloway [:mdholloway]]

Thanks for the advice, Brian and Dana!  FYI, I'm traveling for work this week and probably won't have a chance to look at this again until next Sunday at the earliest; I haven't fallen off the planet.  

I also noticed before I left that there's a related-looking unit test here (https://github.com/mozilla/gecko-dev/blob/0cccf0ed585deed2a261432dc2e3828684e9fae6/security/nss/external_tests/ssl_gtest/tls_agent.cc#L463-L468) although I haven't yet had time to really wrap my head around the related code; should I update this to also test the case of a cert change, if possible?

Comment 9

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

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #7)
> Dana, do you know if NSS actually allows the certificate to be changed
> during renegotiation? That's the main thing I'm unsure about.

Oh, yeah, I don't know if that's actually possible.

(In reply to Michael Holloway [:mdholloway] from comment #8)
> I also noticed before I left that there's a related-looking unit test here
> (https://github.com/mozilla/gecko-dev/blob/
> 0cccf0ed585deed2a261432dc2e3828684e9fae6/security/nss/external_tests/
> ssl_gtest/tls_agent.cc#L463-L468) although I haven't yet had time to really
> wrap my head around the related code; should I update this to also test the
> case of a cert change, if possible?

That's NSS testing code. For the purposes of this bug, I don't think it will be particularly useful to modify it.