571797 - NSS should not send the decryption_failed alert

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

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

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.70 Safari/533.4
Build Identifier: 

RFC 5246 (the TLS 1.2 specification) says that decryption_failed MUST NOT be sent (period); instead bad_record_mac should be sent. Currently, ssl3_HandleRecord will return bad_record_mac for everything except TLS (1.0). Instead, it should just always use bad_record_mac:

-  SSL3_SendAlert(ss, alert_fatal,
-                 isTLS ? decryption_failed : bad_record_mac);
+  SSL3_SendAlert(ss, alert_fatal, bad_record_mac);

See http://tools.ietf.org/html/rfc5246#page-31

Reproducible: Always

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

OK, since the proposed change is backwardly compatible with SSL 3.0, 
I have no problem applying this TLS 1.2 change retroactively to all versions
of SSL 3.x in libSSL.

Comment 2

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

Attachment: Patch that implements the TLS-1.2-mandated for all versions

This patch sends bad_record_mac instead of decryption_failed. It makes sure that the caller cannot distinguish the reason why bad_record_mac was sent, as noted in the pre-existing comment "always log mac error, in case attacker can read server logs."

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 453227 [details] [diff] [review]
Patch that implements the TLS-1.2-mandated for all versions

r=nelson

I believe we can change/eliminate the definition of the symbol decryption_failed because ssl3prot.h is considered a private header, IINM.  If it was a public header, we could not change it.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Bug 571797: NSS should not send the decryption_failed alert
Patch contributed by Brian Smith <[e-mail]>, r=nelson

Checking in ssl3con.c;   new revision: 1.141; previous revision: 1.140
Checking in ssl3prot.h;  new revision: 1.19; previous revision: 1.18
Checking in sslerr.h;    new revision: 1.11; previous revision: 1.10

Comment 5

[Wan-Teh Chang]

Comment on attachment 453227 [details] [diff] [review]
Patch that implements the TLS-1.2-mandated for all versions

The changes to ssl3_HandleRecord in this patch require
more thought because they cause NSS to execute code that
may be unsafe under error conditions.

1. If the crSpec->decode call fails, I believe plaintext->len
will remain 0:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.140&mark=8875-8877#8874

If so, then it seems bad to fall through and execute this
code with plaintext->len equal to 0, as we will be reading
plaintext->buf[-1]:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.140&mark=8891-8892#8874

It's also not clear if it's fine to call ssl3_ComputeRecordMAC
with a plaintext->len of 0.

2. If the ssl3_ComputeRecordMAC call fails:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.140&mark=8916-8918#8914

I see nothing wrong with the original code, other than
it sets the error code to SSL_ERROR_MAC_COMPUTATION_FAILURE
instead of SSL_ERROR_BAD_MAC_READ:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.140&mark=8920,8922,8933,8935#8914

In summary, unless we must fall through to avoid timing attacks,
or you have actually tested the new execution paths under error
conditions, it is safer to keep the structure of the original code
and simply change the alert and error code.

Comment 6

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

(In reply to comment #5)
> 1. If the crSpec->decode call fails, I believe plaintext->len
> will remain 0:
> 
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3con.c&rev=1.140&mark=8875-8877#8874
> 
> If so, then it seems bad to fall through and execute this
> code with plaintext->len equal to 0, as we will be reading
> plaintext->buf[-1]:

Wan-Teh,

Thank you for pointing out the problem.

Please see my patch in Bug 571795, which handles the case where plaintext->len is zero, since, AFAICT, crSpec->decode can succeed when a zero-length input is decrypted to a zero-length output.

I am also open to either making this change:

- if (cipher_def->type == type_block) {
+ if (cipher_def->type == type_block && !padIsBad) {

or to returning the code to its former structure. I changed the structure of the code in the first place because the reason for setting the error code to SSL_ERROR_BAD_MAC_READ in all cases was documented down below already, and the new control flow makes it clear that all the error cases are reported identically as suggested in the TLS 1.2 spec.

Please let me know what you want to do.

Comment 7

[Wan-Teh Chang]

Brian,

The ssl3con.c change in your patch in bug 571795 will avoid the
reading of plaintext->buf[-1].  But we still need to inspect the
subsequent code to convince ourselves that it is safe.

Rather than adding && !padIsBad to all subsequent code, I
suggest that we either use goto statements to jump to some
common error handling code, or we repeat the error handling
code after every error as the original code does.

Why do you think setting the SSL_ERROR_MAC_COMPUTATION_FAILURE
error code is bad?  It is our own error, not invalid input received
from the peer.  Just curious...

Comment 8

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

Attachment: Patch to correct issue that WTC mentioned

(In reply to comment #7)
> Rather than adding && !padIsBad to all subsequent code, I
> suggest that we either use goto statements to jump to some
> common error handling code, or we repeat the error handling
> code after every error as the original code does.
> 
> Why do you think setting the SSL_ERROR_MAC_COMPUTATION_FAILURE
> error code is bad?  It is our own error, not invalid input received
> from the peer.  Just curious...

Originally, I made a very simple change like I mentioned in the description:

-  SSL3_SendAlert(ss, alert_fatal,
-                 isTLS ? decryption_failed : bad_record_mac);
+  SSL3_SendAlert(ss, alert_fatal, bad_record_mac);

But, then I inspected the code and I saw that if you were to see a SSL_ERROR_MAC_COMPUTATION_FAILURE in a log file, then you would know that the decryption succeeded. I think that could almost never be an issue, but it does seem to go against the idea of making failed decryption indistinguishable from a bad MAC. Consequently, I rewrote the patch so that the issue wouldn't even come up, which resulted in patch 453227.

When the code is added for TLS 1.2 AEAD mode (for AES-GCM), all the code from the call to crSpec->decode through the call to NSS_SecureMemcmp will almost definitely need to be factored out into a separate function, and that function probably shouldn't send alerts itself. So, IMO, it's not worth changing the structure of the code back to the way it was before.

I have attached a patch (to be applied on top of the one already checked in) that adds the one !padIsBad check to ensure that the padding-checking code gets skipped if the decryption failed. Note that after the padding-checking code, it was already possible for padIsBad to be true and/or for plaintext->len to be zero. And also note that when padIsBad is set, it is never unset back to PR_FALSE.

Comment 9

[Wan-Teh Chang]

Comment on attachment 453817 [details] [diff] [review]
Patch to correct issue that WTC mentioned

r=wtc.  Thanks for the patch.

Should padIsBad be renamed macIsBad?

I checked in this patch on the NSS trunk (NSS 3.12.7).

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.142; previous revision: 1.141
done

The reason I think it's better to log an accurate error code for
ssl3_ComputeRecordMAC failures is that invalid input from
the peer won't cause that function to fail.  So it's not necessary
to obfuscate the reason for ssl3_ComputeRecordMAC failures.

The importance of accurate error reporting is not apparent
until we need to debug an NSS problem remotely.