1084986 - Use a better error code to report locally disabled SSL/TLS versions

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Dave Garrett]

https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/SSLerrs.h
https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslcon.c#2749

When a Firefox user attempts to connect to a server only supporting SSL3, they receive a generic ssl_error_no_cypher_overlap error rather than one indicating the issue is SSL3 being disabled.

NSS has an SSL_ERROR_SSL2_DISABLED error code and message, yet SSL_ERROR_NO_CYPHER_OVERLAP is what it emits here. An SSL_ERROR_SSL3_DISABLED error code and message should be added to NSS and Firefox so that these errors contain enough information to diagnose the issue properly.

Comment 1

[Dave Garrett]

Adding error codes for disabled TLS 1.0 and even newer versions would also be useful.

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Add an SSL error code to indicate that the peer negotiated a version which is locally disabled

Comment 3

[Masatoshi Kimura [:emk]]

Comment on attachment 8523007 [details] [diff] [review]
Add an SSL error code to indicate that the peer negotiated a version which is locally disabled

FYI the current version of the patch is not limited to SSLv3. If the client set the higher minimum version, all lower versions will result the same new error code.

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Expose peer negotiated version to the caller

This patch will also require a patch from bug 1054349.

Comment 5

[Dave Garrett]

(In reply to Masatoshi Kimura [:emk] from comment #3)
> FYI the current version of the patch is not limited to SSLv3. If the client
> set the higher minimum version, all lower versions will result the same new
> error code.

There is a specific error for SSL2, so I was thinking having a corresponding one specifically for SSL3 would be the best route. Having one that could either mean no SSL3 (expected) or no TLS1 (due to user setting) could produce ambiguous errors. Alternatively, I guess you could remove the SSL2 error and have a single one for failed version negotiation and expect that version to be looked up specifically for showing the error message. Either way, I think consistency is best.

Comment 6

[Masatoshi Kimura [:emk]]

SSL2 uses a completely different packet format than any later versions. Unlike SSL2, there is no technical reason to treat SSL3 or later versions in the different manner.
The second patch will give a precise information about what exact version the server tried to negotiate for SSL3 or later.

Comment 7

[Dave Garrett]

Makes sense. I'll change the summary to be more general, then.

Comment 8

[Wan-Teh Chang]

Comment on attachment 8523340 [details] [diff] [review]
Expose peer negotiated version to the caller

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

::: lib/ssl/ssl3con.c
@@ +901,5 @@
>  	PORT_SetError(SSL_ERROR_SSL_DISABLED);
>  	return SECFailure;
>      }
>  
> +    ss->version = PR_MIN(peerVersion, ss->vrange.max);

This violates a common assumption that a function has no
side effects when it fails. So I'll need to investigate
this further.

The caller of this function has all the info to perform
this calculation. Why don't we let the caller do this?

Comment 9

[Wan-Teh Chang]

Comment on attachment 8523007 [details] [diff] [review]
Add an SSL error code to indicate that the peer negotiated a version which is locally disabled

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

::: lib/ssl/sslerr.h
@@ +197,5 @@
>  SSL_ERROR_NEXT_PROTOCOL_NO_PROTOCOL     = (SSL_ERROR_BASE + 130),
>  
>  SSL_ERROR_INAPPROPRIATE_FALLBACK_ALERT  = (SSL_ERROR_BASE + 131),
>  
> +SSL_ERROR_SSL_VERSION_DISABLED          = (SSL_ERROR_BASE + 132),

I wonder if we can just recycle the SSL_ERROR_UNSUPPORTED_VERSION error code,
which is only used by the SSL 2.0 implementation right now.

Note that "_DISABLED" may be wrong in the future if we remove our SSL 3.0
support completely. So "_UNSUPPORTED" seems like a more general description
of the error.

I agree the SSL_ERROR_NO_CYPHER_OVERLAP error code is misleading and
should not be used for no protocol version overlap.

Comment 10

[Masatoshi Kimura [:emk]]

(In reply to Wan-Teh Chang from comment #8)
> The caller of this function has all the info to perform
> this calculation. Why don't we let the caller do this?

(In reply to Wan-Teh Chang from comment #9)
> I wonder if we can just recycle the SSL_ERROR_UNSUPPORTED_VERSION error code,
> which is only used by the SSL 2.0 implementation right now.

Good idea. I'll update patches.

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Use SSL_ERROR_UNSUPPORTED_VERSION and expose peer negotiated version

- Moved the version set logic to the caller. But didn't cap the version at ss->vrange.max. Otherwise the client will not see the correct version the server tried to negotiate if the version is too high.
- Recycled SSL_ERROR_UNSUPPORTED_VERSION instead of adding a new error code.
- Incorporated a achange from bug 1054349.

Comment 12

[Wan-Teh Chang]

Comment on attachment 8526734 [details] [diff] [review]
Use SSL_ERROR_UNSUPPORTED_VERSION and expose peer negotiated version

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

Thank you for the new patch. Now I understand your approach.

A problem with this approach is that we'll need to document
what the output of SSL_GetChannelInfo means after a handshake
failure. It seems better to add a new function that returns
the key info in the ServerHello that caused the handshake to
fail.

A simpler solution would be to split SSL_ERROR_UNSUPPORTED_VERSION
into two error codes: SSL_ERROR_PEER_VERSION_TOO_OLD and
SSL_ERROR_PEER_VERSION_TOO_NEW.

In any case, I like the SSL_ERROR_NO_CYPHER_OVERLAP =>
SSL_ERROR_UNSUPPORTED_VERSION part of this patch, and I will
check in that part first.

::: security/nss/lib/ssl/ssl3con.c
@@ +6286,5 @@
>      rv = ssl3_NegotiateVersion(ss, version, PR_FALSE);
>      if (rv != SECSuccess) {
>      	desc = (version > SSL_LIBRARY_VERSION_3_0) ? protocol_version 
>  						   : handshake_failure;
> +	errCode = SSL_ERROR_UNSUPPORTED_VERSION;

If we split SSL_ERROR_UNSUPPORTED_VERSION into two errors, we
can still hardcode errCode to SSL_ERROR_UNSUPPORTED_VERSION here,
or we can say:
    errCode = PORT_GetError();
This will prevent the ssl_MapLowLevelError call under the alert_loser
label from changing the error code.

Comment 13

[Wan-Teh Chang]

Attachment: Use SSL_ERROR_UNSUPPORTED_VERSION instead of SSL_ERROR_NO_CYPHER_OVERLAP in ssl3_NegotiateVersion, by Masatoshi Kimura

Patch checked in: https://hg.mozilla.org/projects/nss/rev/b4bab5502261

Comment 14

[Masatoshi Kimura [:emk]]

(In reply to Wan-Teh Chang from comment #12)
> A simpler solution would be to split SSL_ERROR_UNSUPPORTED_VERSION
> into two error codes: SSL_ERROR_PEER_VERSION_TOO_OLD and
> SSL_ERROR_PEER_VERSION_TOO_NEW.

We (PSM and Fx UI) would like to distinguish not only between "too low" and "too high", but also between "SSL 3.0" and "TLS 1.0" (if range.minversion >= TLS 1.1).
An obvious solution is adding an error code for every TLS version, but I would not like to add a new error code every time a new TLS version is introduced.
I'll consider to add a new function.

> In any case, I like the SSL_ERROR_NO_CYPHER_OVERLAP =>
> SSL_ERROR_UNSUPPORTED_VERSION part of this patch, and I will
> check in that part first.

Thank you.

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: Add SSL_GetServerHelloVersion

ssl3_SendServerHello() will send the value in ss->version.
In ssl3_HandleClientHello(),
1. If the client version is equal to or higher than the server version, ssl3_NegotiateVersion() will take the lower value (that is, the server version).
2. If the client version is lower than the server version, ssl3_NegotiateVersion() will fail. I added a code to set the server version to ss->version on failure.

So ss->version should have the appropriate value in all cases.

Comment 16

[Masatoshi Kimura [:emk]]

> In ssl3_HandleClientHello(),
Correction: In ssl3_HandleServerHello()

Comment 17

[Dave Garrett]

Comment on attachment 8532500 [details] [diff] [review]
Add SSL_GetServerHelloVersion

>+/* Set the TLS version the server tried to negotiate.
>+ * The version will be set even when the handshake was failed
>+ * with SSL_ERROR_UNSUPPORTED_VERSION.
>+ */
>+SSL_IMPORT SECStatus SSL_GetServerHelloVersion(PRFileDesc *fd,
>+                                               PRUint16 *version);

apparent typo:  Set -> Get

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: Set the version the server tried to negotiate on failure

I separated the API change to bug 1111556.

Comment 19

[:Gijs (he/him)]

Masatoshi-san, can you clarify which parts of this bug and bug 1111556 are required to have accurate information for the error page in Firefox? With bug 1084025 on 37, and perhaps soon 36 as well, it looks like the error we currently rely on is set to become largely meaningless in terms of distinguishing sslv3 errors from other ssl-related errors, and that will impact Firefox's ability to give useful feedback to the user.

We would really like to be able to ship updated error information and styling for the ssl v3 case in 36 and/or 37 using an accurate error code. We're already halfway through the 37 cycle, and I'm worried that we will miss even landing this there. :-\

Comment 20

[Masatoshi Kimura [:emk]]

(In reply to :Gijs Kruitbosch from comment #19)
> Masatoshi-san, can you clarify which parts of this bug and bug 1111556 are
> required to have accurate information for the error page in Firefox? With
> bug 1084025 on 37, and perhaps soon 36 as well, it looks like the error we
> currently rely on is set to become largely meaningless in terms of
> distinguishing sslv3 errors from other ssl-related errors, and that will
> impact Firefox's ability to give useful feedback to the user.

The landed part <https://hg.mozilla.org/projects/nss/rev/b4bab5502261> will already improve the accuracy. Kai, is it possible to cherry-pick this change on m-c if this bug and bug 1107731 are unlikely to be fixed soon?

Comment 21

[Masatoshi Kimura [:emk]]

The change has been landed into m-c.

Comment 22

[Kai Engert (:KaiE:)]

FYI, we never cherry pick NSS fixes into Mozilla branches, we always must land full releases, because Linux distributions require that the NSS snapshots used by Mozilla are available as NSS release versions, because Linux distributions package NSS separately from Firefox.

Comment 23

[Kai Engert (:KaiE:)]

Because NSS 3.18 is not ready for release, and because Firefox 36 is already in beta, we'd have to create an interim NSS release with this bugfix. I have suggested that to the NSS team, to see if anyone objects.

Comment 24

[Kai Engert (:KaiE:)]

Comment on attachment 8536523 [details] [diff] [review]
Set the version the server tried to negotiate on failure

Masatoshi, do you still need this patch for this bug, or can we move it to a separate bug? It seems you were planning to work on a new API in bug 1111556, but then you marked 1111556 as a duplicate of this bug, which I don't understand. Was that duplicate status an accident?

And can this bug bug marked as resolved?

Comment 25

[Masatoshi Kimura [:emk]]

I'm fine with separating the unreviewed part to a new bug. It is not urgent for Firefox 36.
I duped bug 1111556 against bug 1084669, not this bug.

Comment 26

[Kai Engert (:KaiE:)]

Comment on attachment 8536523 [details] [diff] [review]
Set the version the server tried to negotiate on failure

ok, please move this patch and its review/discussion to a separate bug, thanks.

Comment 27

[Kai Engert (:KaiE:)]

Marking as resolved fixed.

Comment 28

[Kai Engert (:KaiE:)]

mass change target milestone to 3.17.4

Comment 29

[Kai Engert (:KaiE:)]

Adjust the subject, because the error code has existed previously, but is now being used.