Closed Bug 1404824 Opened 2 years ago Closed 2 years ago

Firefox crashes with min TLS version 4 (1.3)

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 - wontfix
firefox58 --- fixed

People

(Reporter: franziskus, Assigned: keeler)

References

Details

(Whiteboard: [psm-assigned])

Crash Data

Attachments

(3 files)

Steps to reproduce:

* set security.tls.version.min to 4
* go to https://mitls.org
* wait a little

After a while Firefox (beta and nightly) will crash. Subsequently restarting Firefox with security.tls.version.min = 4 results in a startup crash.
Version: 56 Branch → Trunk
mitls.org won't negotiate TLS 1.3 for me, only TLS 1.2 :(
is there another site this reproduces on? I'm not seeing any crashes on https://tls13.com
Flags: needinfo?(franziskuskiefer)
I think it is the point here, when the TLS handshake succeeds Firefox handles everything properly.
When the handshake fails because of some random error like "No cipher overlap". Firefox should not crash.
> mitls.org won't negotiate TLS 1.3 for me, only TLS 1.2

Right, it get no cipher overlap. You have to stay on that error page for a while before it crashes.
Flags: needinfo?(franziskuskiefer)
Ah - here's what's happening: Firefox will eventually make a background request to "services.addons.mozilla.org" or another Mozilla domain where we set the flag nsISocketProvider::BE_CONSERVATIVE, which means we set the maximum TLS version to 1.2. Of course, if the minimum is 1.3, NSS returns an error from SSL_VersionRangeSet, so nsSSLIOLayerSetOptions fails and nsSSLIOLayerAddToSocket jumps to its failure cleanup area, which calls plaintextLayer's dtor function pointer, which is null.

So I guess there's two issues here: 1 is if the minimum configured TLS version is higher than 1.2, setting the BE_CONSERVATIVE flag will result in an inconsistent TLS version range being passed to SSL_VersionRangeSet. 2 is that if anything in nsSSLIOLayerAddToSocket fails, we apparently can end up trying to jump to 0x0, which isn't good.
Assignee: nobody → dkeeler
Priority: -- → P1
Whiteboard: [psm-assigned]
sounds very much like an edge case, we disable tls 1.3 on release for now, so I don't expect users to be manually setting 1.3 as minimum.
Comment on attachment 8914943 [details]
bug 1404824 - fix error-handling case of plaintext-layer popping in nsNSSSocketInfo

https://reviewboard.mozilla.org/r/186192/#review191870
Attachment #8914943 - Flags: review?(honzab.moz) → review+
Comment on attachment 8914944 [details]
bug 1404824 - reconcile inconsistent TLS version range settings by erring on the conservative side

https://reviewboard.mozilla.org/r/186194/#review191876

::: security/manager/ssl/nsNSSIOLayer.cpp:2627
(Diff revision 1)
>                static_cast<unsigned int>(range.max)));
>  
> +  // If the user has set their minimum version to something higher than what
> +  // we've now set the maximum to, this will result in an inconsistent version
> +  // range unless we fix it up. This will override their preference, but we only
> +  // do this for sites critical to the operation of the browser (e.g. update

how do we enforce to do this only for sites under our control?  

and is this actually save?  if there is a sec issue found in lower version(s) of TLS that could make our server (or better said - the client) vulnerable to spoofing/whatever, aren't we opening ourselves to that risk regardless users may set the pref to a higher version to mitigate?  Or when users know they are in a potentially non-secure network and want to force certain minimal tls version, allowing a lower version for something that critical as updates seems wrong to me.
Comment on attachment 8914945 [details]
bug 1404824 - add a test for the BE_CONSERVATIVE flag if TLS is restricted to 1.3 only

https://reviewboard.mozilla.org/r/186196/#review191880

depending on how the part2 patch goes.
Attachment #8914945 - Flags: review?(honzab.moz) → review+
Comment on attachment 8914944 [details]
bug 1404824 - reconcile inconsistent TLS version range settings by erring on the conservative side

https://reviewboard.mozilla.org/r/186194/#review191876

> how do we enforce to do this only for sites under our control?  
> 
> and is this actually save?  if there is a sec issue found in lower version(s) of TLS that could make our server (or better said - the client) vulnerable to spoofing/whatever, aren't we opening ourselves to that risk regardless users may set the pref to a higher version to mitigate?  Or when users know they are in a potentially non-secure network and want to force certain minimal tls version, allowing a lower version for something that critical as updates seems wrong to me.

beConservative and the tlsFlags are internal APIs. We only set them for platform/add-on updates or when we're doing things like bug 1401367. If there's a TLS 1.2 issue then we'll need to update everything anyway (both the servers to not use TLS 1.2 and the client to not set the beConservative flag). Until then, though, we want the client to be able to connect to our servers, which is what this ensures.
Comment on attachment 8914944 [details]
bug 1404824 - reconcile inconsistent TLS version range settings by erring on the conservative side

https://reviewboard.mozilla.org/r/186194/#review192242

Thanks for your comment.  Let's land this.
Attachment #8914944 - Flags: review?(honzab.moz) → review+
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ef54b47be70
fix error-handling case of plaintext-layer popping in nsNSSSocketInfo r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/c78938f9f8f8
reconcile inconsistent TLS version range settings by erring on the conservative side r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/aa5b5b65b4f9
add a test for the BE_CONSERVATIVE flag if TLS is restricted to 1.3 only r=mayhemer
You need to log in before you can comment on or make changes to this bug.