571732 - Wrong record length checks in ssl3_HandleRecord; Wrong alert sent in ssl3_GatherData

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: 

A patch is forthcoming; this patch also includes the changes from my patch for bug 571699.

The check for maximum ciphertext fragment length is already done in ssl3_GatherData, so the duplicate check in ssl3_HandleRecord is redundant. That's why I added the assert. However, previously the check in ssl3_HandleRecord was only done for TLS 1.0 or later. There's no reason not to do the check for SSL 3.0 too.

I moved the check against the maximum ciphertext length in ssl3_HandleRecord up so that the following change could take advantage of the knowledge that the ciphertext is not too big.

Consider the case MAX_FRAGMENT_LENGTH <= plaintext->space <= cText->buf->len <= MAX_FRAGMENT_LENGTH + 2048. Then the plaintext buffer would not get resized and then we wouldn't have enough space to decrypt the ciphertext into it. Luckily, it seems that the buffer is only allocated by sslBuffer_Grow and sslBuffer_Grow never allocates less than MAX_FRAGMENT_LENGTH + 2048, so there is nothing to worry about (at least on the trunk; I didn't check older versions). However, the check is misleading, so I fixed it.

While it may be true that the version of zlib in NSS's CVS never expands its input by more than SSL3_COMPRESSION_MAX_EXPANSION, that isn't necessarily the case for every deflate implementation or possibly even the system's version of zlib.

The check for the maximum decompressed plaintext was wrong. First, the check needs to be done for SSL 3.0 as well as TLS. Second, the maximum decompressed plaintext length is MAX_FRAGMENT_LENGTH, not MAX_FRAGMENT_LENGTH + 1024.

ssl3_GatherData did not send the correct alert when it detected an overly long record. I changed it to send record_overflow even for SSL 3.0 because it is an unrecoverable error and because it is more informative.

As far as I can tell, none of these result in buffer overflows or other security problems.

Reproducible: Always

Comment 1

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

Attachment: Patch addressing these issues

Comment 2

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

Attachment 456135 [details] [diff] in Bug 576902 fixes all the buffer checks, leaving only the alert issue.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 450904 [details] [diff] [review]
Patch addressing these issues

Brian, a review question for you concerning this patch.
Why did you change all the specread locks to specwrite locks?
Is it because you perceive they protect crSpec->decompressContext 
which is written during this passage?

I think of that as protected by a different lock.  
Locks are being discussed in several other bugs.  
Perhaps we need a lock pow-wow with you, Wan-Teh, Adam and me.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

oh, prehaps it was because of this line in that passage:

  ssl3_BumpSequenceNumber(&cwSpec->write_seq_num);

If that's the sole reason, I think we should move those sequence numbers 
out of the spec structure.  I think they don't really belong there anyway.

After further review, I'm more convinced than ever that the decompressContext
pointer ITSELF is indeed protected by the spec lock, but it is never changed
in this passage of code, so the reader lock suffices.  The context structure 
is protected by the xmit or recv buf lock, I believe, or should be, and that 
is already held before this function is called.

Comment 5

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

Both of those were my motivation. The (perhaps stale) documentation in notes.txt says the lock protects "the pointer and the data in the spec and any data referenced by the spec." I would like to join in the lock discussion if/when it is held.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 450904 [details] [diff] [review]
Patch addressing these issues

Brian, 
Do you agree that, if we change function ssl3_BumpSequenceNumber so that it 
atomically increments spec->write_seq_num, then it is unnecessary to convert
all those shared reader-lock calls into exclusive writer-lock calls?

Comment 7

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

I think it would be confusing to leave these members inside the spec structure and not use a spec write lock when they are modified. In particular, it is confusing to have the record reading/writing code modify the decode/encode & decompression/compression contexts using just the read lock (without a write lock) but then depend on no other code modifying the contexts without a write lock. If the xmit/recv locks protect these items then they should be moved outside the spec structure to be alongside the other items protected by those locks.

It seems like the pending sequence numbers are always 0 (zero), so we can just reset the relevant sequence number to zero after we set crSpec=prSpec or cwSpec=pwSpec. It seems like the compression/decompression contexts and decode/encode contexts could be reset then as well, instead of being per-spec items. Then it would be clear that the current uses of ssl_GetSpecReadLock are correct. But, if we can't show that it is safe to move these items out of the spec structure then it isn't clear to me how we can prove that they don't need to be protected by a spec write lock when modified.

Comment 8

[Leander Schwarz]

D138529 updates record size checks and alerts to RFC 8446 / TLS 1.3 specifications.

https://hg.mozilla.org/projects/nss/rev/f4d2f39068002a69fb0fd98863fc65ff7236b33b