571795 - NSS should send an alert when it receives a zero-length record

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: 

When ssl3_GatherData encounters a record with length == 0, it loops back around to do a read for 0 bytes of data. That read will return EOF. This will cause ssl3_GatherData to return EOF. When the callers see that ssl3_GatherData returned EOF, they set the error to PR_END_OF_FILE_ERROR and return without sending an alert. 

Instead, ssl3_GatherData should return the record so that it can be processed and by ssl3_HandlRecord. ssl3_HandleRecord should reject the record with a decode_error alert.

I am willing to write the patch.

Reproducible: Always

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Assuming this report is accurate (I haven't checked), I believe it's a 
regression.  I wonder when that happened.  I'm pretty sure there was a time
when it did not, because I recall a few years ago someone did a test of 
numerous SSL/TLS implementations to see how they handled edge cases like 
this, and NSS fared better than most.  I believe it got this test right. 

Anyway, I accept your offer to write a patch.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Whether NSS should send an alert or merely silently ignore the record might 
be debatable.  But NSS certainly should not treat it as receiving EOF.

Comment 3

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

I know there was an interoperability report testing whether implementations accepted zero-length application_data. However, that test was testing the plaintext length (after de-padding, de-MACing, and de-compressing), not the ciphertext length (which would always have at least a MAC that would make the ciphertext length > 0).

NSS cannot silently ignore empty records because they aren't authenticated (zero length -> no MAC) and they affect the record sequence. However, I think it should report the error in an alert because it does report an error when the ciphertext is too large (see ssl3_GatherData).

I will work on the patch.

Comment 4

[Kuat Eshengazin]

TLS 1.2 (RFC 5246) 6.2.1. says that application data records of zero-length are OK:

 "Implementations MUST NOT send zero-length fragments of Handshake,
  Alert, or ChangeCipherSpec content types.  Zero-length fragments of
  Application data MAY be sent as they are potentially useful as a
  traffic analysis countermeasure."

Comment 5

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

Kuat, that is referring to TLSPlaintext.length. I am talking about TLSCiphertext.length. I filed bug 572179 for the TLSPlaintext.length issue. It needs a separate check.

Comment 6

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

Attachment: Patch to treat missing padding length byte like a bad MAC

Here is the patch. 75% of it is just changes in indentation.

Comment 7

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

Comment on attachment 453209 [details] [diff] [review]
Patch to treat missing padding length byte like a bad MAC

Patch 456135 in Bug 576902
contains a very similar, but better fix for this issue.

Comment 8

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

In order to make the patch for Bug 576902 more straightforward to review, I removed part of the fix for this bug. After that patch is applied, another patch will be needed to update ssl3_GatherData in a similar way to the (obsolete) patch attached to this bug.

Comment 9

[Florian Bender]

(In reply to Brian Smith (:bsmith) from comment #7)
> Patch 456135 in Bug 576902
> contains a very similar, but better fix for this issue.

How much of this bug is covered by Bug 576902? AFAICS, the underlying issues are different thus can be patched independently, aren't they?

Comment 10

[Leander Schwarz]

For TLS 1.3, RFC8446 only specifies sending of alerts and termination of the session on InnerPlaintext.length of zero, but does not specify action for zero-length fragments while saying they SHOULD not be sent.

[RFC8446, Section 5.4]:

Implementations MUST NOT send Handshake and Alert records that have a zero-length
TLSInnerPlaintext.content; if such a message is received, the
receiving implementation MUST terminate the connection with an
"unexpected_message" alert.

[RFC8446, Section 5.1]:

Implementations MUST NOT send zero-length fragments of Handshake
types, even if those fragments contain padding.

D141841 adds alerts and checks for detection of InnerPlaintext.length == 0 records. It also adds test cases for the expected behavior of zero-length fragment records for coverage.

https://hg.mozilla.org/projects/nss/rev/144c87accae8ac9e60946c7684cb378880a921a0