1290758 - NSS accepts handshake data that spans CCS (in < TLS 1.2 mode)

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Eric Rescorla (:ekr)]

Found with BoGo testing tool.

The security impact here is modest in that you would accept an unencrypted Finished but the attacker shouldn't have access to that.

Preliminary patch attached for TLS 1.2.
This patch doesn't test DTLS so that needs analysis, however, I believe that it is likely fine because we do not accept out-of-order packets.

Comment 1

[Eric Rescorla (:ekr)]

Attachment: 0001-Bug-1290758-Don-t-accept-CCS-when-you-have-pending-h.patch

MT: note that I only check for handshake_bytes being non-zero but it stays non-zero through packet reading, so this should be enough. I would appreciate review of this approach.

Comment 2

[Eric Rescorla (:ekr)]

+ davidben who asked me privately about this.

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8776411 [details] [diff] [review]
0001-Bug-1290758-Don-t-accept-CCS-when-you-have-pending-h.patch

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

Not sure what to do with this without a test.

Comment 4

[Eric Rescorla (:ekr)]

(In reply to Martin Thomson [:mt:] from comment #3)
> Comment on attachment 8776411 [details] [diff] [review]
> 0001-Bug-1290758-Don-t-accept-CCS-when-you-have-pending-h.patch
> 
> Review of attachment 8776411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not sure what to do with this without a test.

As I said in c3, please provide feedback on the approach and on whether this needs to be security sensitive. This is why I asked for feedback, not review. When I am ready for actual review of the patch, I will also provide a test, but in the meantime I tested it with BoGo (which is how I discovered it in the first place).

Comment 5

[Martin Thomson [:mt:]]

I'm fairly sure that I don't understand the bounds of the problem, hence the request for a test.

Per https://github.com/tlswg/tls13-spec/pull/566, I think that we need to take a bit of a harder look at this than just CCS.  It isn't clear to me that we are safe from someone smuggling extra handshake records from the next epoch into the same record as the TLS 1.3 Finished.

Without thinking about this too hard - and even with the expanded scope - this doesn't seem dire: we won't send data like that (we flush at the right times) and the risk is limited to handshake records.  I do worry that we will accept an EncryptedExtensions that appears in the clear though.  I thought that we had protection against this sort of drift, but I couldn't find anything.  We would need similar checks in tls13_SetCipherSpec() to be sure.

Comment 6

[Eric Rescorla (:ekr)]

This isn't about TLS 1.3. We have separate code for this in TLS 1.3, landed 
in bug 1274335. 

This is a defect in the TLS 1.2 code in which you accept a Finished message which
partly spans a CCS. I'll have to cook up a test for this, but roughly what it's going
to be is inserting some plaintext handshake data into the stream, resulting in a
Finished error rather than an unexpected data error (you can't get handshake
success without rather more screwing around with the NSS code than is probably
practical).

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8776411 [details] [diff] [review]
0001-Bug-1290758-Don-t-accept-CCS-when-you-have-pending-h.patch

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

This seems like a reasonable fix.  The only thing I can imagine being a problem here is that we might accept handshake messages that aren't encrypted.  We don't rely on confidentiality to protect the handshake.

Also: git config --global diff.context 8

Comment 8

[Matt Wobensmith [:mwobensmith][:matt:]]

Triage group marking sec-low as per EKR's statements in comment 0.

Comment 9

[Eric Rescorla (:ekr)]

Attachment: 0001-Bug-1290758-Don-t-accept-CCS-when-you-have-pending-h.patch

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8778619 [details] [diff] [review]
0001-Bug-1290758-Don-t-accept-CCS-when-you-have-pending-h.patch

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

::: external_tests/ssl_gtest/ssl_loopback_unittest.cc
@@ +153,5 @@
> +  TlsPreCCSHeaderInjector() {}
> +  virtual PacketFilter::Action FilterRecord(
> +      const RecordHeader& record_header,
> +      const DataBuffer& input,
> +      size_t* offset, DataBuffer* output) {

override (I've been trying to be good about putting these in for new code)

::: external_tests/ssl_gtest/test_io.cc
@@ +377,5 @@
>      case PacketFilter::DROP:
>        LOG("Droppped packet: " << packet);
>        break;
>      case PacketFilter::KEEP:
> +      LOG("PacketReceived: " << packet);

Do you want to keep this?  I found it pretty useful, but it's a bit spammy.  Maybe we can guard it with an environment variable.

::: external_tests/ssl_gtest/tls_filter.h
@@ +30,5 @@
>  
>    class Versioned {
>     public:
>      Versioned() : version_(0) {}
> +    Versioned(uint16_t version) : version_(version) {}

explicit

@@ +83,5 @@
>    // outparam with the new record contents if it chooses to CHANGE the record.
>    virtual PacketFilter::Action FilterRecord(const RecordHeader& header,
>                                              const DataBuffer& data,
> +                                            DataBuffer* changed) {
> +    return KEEP; }

Fix formatting.

::: lib/ssl/ssl3con.c
@@ +3984,5 @@
>          (void)SSL3_SendAlert(ss, alert_fatal, unexpected_message);
>          PORT_SetError(SSL_ERROR_RX_UNEXPECTED_CHANGE_CIPHER);
>          return SECFailure;
>      }
> +    if (ss->ssl3.hs.header_bytes) {

Comment please.  /* If there is any part of the next message present in the remainder of the message, then this is probably part of a Finished message. */  Or something.

Comment 11

[Eric Rescorla (:ekr)]

Landed as:  https://hg.mozilla.org/projects/nss/rev/4c3446ba625d

Comment 12

[Eric Rescorla (:ekr)]

Matt, you can unhide

Comment 13

[Daniel Veditz [:dveditz]]

Mistakenly thought we landed 3.28.1 on the ESR-45 branch