1326130 - BoGo AppDataAfterChangeCipherSpec-DTLS* fails

Status: NEW

(NSS :: Libraries, defect, P3)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

The BoGo tests AppDataAfterChangeCipherSpec-DTLS* fail with NSS returning SSL_ERROR_RX_UNEXPECTED_APPLICATION_DATA.  I looked into it a while ago and left myself this note in config.json: "Is this a protocol violation that Boring tolerates? NSS does not."

Specifically, the TLS versions of the test do expect an error (and get one), but the DTLS versions have this comment in the Go source:

> // BoringSSL's DTLS implementation will drop the out-of-order
> // application data.

So this might be a BoGo/Boring bug, but filing it here in case it's not and to get a bug number.

Comment 1

[Eric Rescorla (:ekr)]

I think this may actually be a bug in NSS. Is the sequence of events:

CCS [epoch 0]
App Data [epoch 1]
Finished [epoch 1]

If so, then I think that yes, NSS should be special-casing this and dropping the app data rather than generating an alert, because this is something that can happen if you send CCS and Finished in separate packets and then you get re-ordering. Shouldn't be a security issue, though.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

The sequence of records BoGo sends to NSS, after the initial handshake:

Handshake, epoch 0, sequence 5
      CCS, epoch 0, sequence 6
 App Data, epoch 1, sequence 0
Handshake, epoch 1, sequence 1
 App Data, epoch 1, sequence 2

The next thing is NSS responding with an alert (epoch 1, sequence 1).  I'm just looking at a hex dump so I can't see the encrypted handshake, but I'm assuming that (1,1) is a Finished.

If (1,0) and (1,1) had been reordered by the network, the sequence numbers would be the other way around; I don't know what NSS does in that case.

Comment 3

[Eric Rescorla (:ekr)]

Yeah, this looks like the situation I was expecting. Martin, comments?

Comment 4

[Martin Thomson [:mt:]]

I would be perfectly happy for NSS to reject that.  The record clearly appears in sequence before the handshake data, which is not permitted by the spec.

I'd be interested in whether reordering causes this to be accepted or not.

If (1,0) is lost, then I might also be OK with that being rejected, but I suspect we won't be rejecting that.

We really need to write some test cases for this stuff, perhaps:

(1,0), (1,1)
(1,1), (1,0)
(1,1) with (1,0) lost

Note that we can do this with simple rewriting rules now, at least for TLS 1.3.

Comment 5

[Eric Rescorla (:ekr)]

MT, the RSN isn't relevant here. The handshake doesn't attempt to enforce RSN sequentiality. So I believe that even if the RSNs were "as expected" you would still get rejection.

Comment 6

[Eric Rescorla (:ekr)]

Wan-Teh, can you give us your opinion of the security impact of this, if any?

Comment 7

[Wan-Teh Chang]

Eric: I don't think your proposed fix has security concerns.

I suggest you handle this new case in dtls_IsRelevant(), which
should return PR_FALSE.