Closed Bug 1084986 Opened 5 years ago Closed 5 years ago

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

Categories

(NSS :: Libraries, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
3.17.4

People

(Reporter: davemgarrett, Unassigned)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
Blocks: 1096695
Blocks: 1098371
Adding error codes for disabled TLS 1.0 and even newer versions would also be useful.
No longer blocks: 1096695
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.
Attachment #8523007 - Attachment description: Add a SSL error code to indicate that the peer negotiated a version which is locally disabled → Add an SSL error code to indicate that the peer negotiated a version which is locally disabled
This patch will also require a patch from bug 1054349.
Attachment #8523340 - Flags: review?(wtc)
Depends on: 1054349
(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.
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.
Makes sense. I'll change the summary to be more general, then.
Summary: add an SSL_ERROR_SSL3_DISABLED error → add an error code specifically for locally disabled SSL/TLS versions
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 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.
(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.
- 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.
Attachment #8523007 - Attachment is obsolete: true
Attachment #8523340 - Attachment is obsolete: true
Attachment #8523007 - Flags: review?(wtc)
Attachment #8523340 - Flags: review?(wtc)
Attachment #8526734 - Flags: review?(wtc)
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.
Attachment #8526734 - Flags: review?(wtc) → review-
Blocks: 1107731
(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.
No longer blocks: 1098371
No longer depends on: 1054349
Attached patch Add SSL_GetServerHelloVersion (obsolete) — Splinter Review
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.
Attachment #8526734 - Attachment is obsolete: true
Attachment #8532500 - Flags: review?(wtc)
> In ssl3_HandleClientHello(),
Correction: In ssl3_HandleServerHello()
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
Depends on: 1111556
I separated the API change to bug 1111556.
Attachment #8532500 - Attachment is obsolete: true
Attachment #8532500 - Flags: review?(wtc)
Attachment #8536523 - Flags: review?(wtc)
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. :-\
Flags: needinfo?(VYV03354)
(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?
Flags: needinfo?(VYV03354) → needinfo?(kaie)
The change has been landed into m-c.
Flags: needinfo?(kaie)
Blocks: 1113780
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.
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 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?
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 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.
Attachment #8536523 - Attachment is obsolete: true
Attachment #8536523 - Flags: review?(wtc)
Marking as resolved fixed.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.18
mass change target milestone to 3.17.4
Target Milestone: 3.18 → 3.17.4
Adjust the subject, because the error code has existed previously, but is now being used.
Summary: add an error code specifically for locally disabled SSL/TLS versions → Use a better error code to report locally disabled SSL/TLS versions
You need to log in before you can comment on or make changes to this bug.