Big Chacha20 records are not rejected by server

RESOLVED FIXED in 3.32

Status

NSS
Libraries
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: Hubert Kario, Assigned: mt)

Tracking

3.29
3.32

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

7 months ago
When server receives an application data record that decrypts to more than 2**14 bytes, it is not rejected as long as it is no larger than 2**14+1024 bytes.

Reproducer:
git clone https://github.com/tomato42/tlsfuzzer.git
pushd tlsfuzzer
git clone https://github.com/warner/python-ecdsa .python-ecdsa
ln -s .python-ecdsa/ecdsa ecdsa
git clone https://github.com/tomato42/tlslite-ng.git .tlslite-ng
pushd .tlslite-ng
popd
ln -s .tlslite-ng/tlslite tlslite
popd
openssl req -x509 -newkey rsa -keyout localhost.key -out localhost.crt -nodes -batch -subj /CN=localhost
openssl pkcs12 -export -passout pass:  -out localhost.p12 -inkey localhost.key -in localhost.crt
mkdir nssdb
certutil -N -d sql:nssdb --empty-password
pk12util -i localhost.p12 -d sql:nssdb -W ''
selfserv -n localhost -p 4433 -d sql:./nssdb -V tls1.0: -H 1 -U 0 -G

# in another terminal, same directory
PYTHONPATH=tlsfuzzer python tlsfuzzer/scripts/test-chacha20.py 'too big plaintext' 'too big plaintext - max compress'

Result:
too big plaintext ...
Error encountered while processing node <tlsfuzzer.expect.ExpectAlert object at 0x2d1cf50> (child: <tlsfuzzer.expect.ExpectClose object at 0x2d1cf90>) with last message being: <tlslite.messages.Message object at 0x2c105d0>
Error while processing
Traceback (most recent call last):
  File "tlsfuzzer/scripts/test-chacha20.py", line 348, in main
    runner.run()
  File "/tmp/tmp.vw4ec1W6eU/tlsfuzzer/tlsfuzzer/runner.py", line 167, in run
    RecordHeader2)))
AssertionError: Unexpected message from peer: ApplicationData(len=162)


Expected:
OK

(the server replies with Alert message decompression_failure)

Additional info:
RFC5246:
   There is always an active compression
   algorithm; however, initially it is defined as
   CompressionMethod.null.
   If the decompression function encounters a
   TLSCompressed.fragment that would decompress to a length in excess of
   2^14 bytes, it MUST report a fatal decompression failure error.
(Reporter)

Comment 1

6 months ago
Tim, could you take a look on this?
Flags: needinfo?(ttaubert)

Updated

6 months ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

6 months ago
I see Tim is currently on PTO, so adding Martin, too, who was involved with reviewing.

Do you agree with Hubert's report?
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 3

6 months ago
This doesn't appear to be a problem that is specific to ChaCha.  But Hubert's report is correct.  We only generate the SSL_ERROR_RX_RECORD_TOO_LONG when the record exceeds MAX_FRAGMENT_LENGTH (=16384) + 1024.  That's a straight-up error.  I don't see any comment suggesting that this is for compatibility reasons, so I think that it's probably safe to fix.

The code:
    if (isTLS && databuf->len > (MAX_FRAGMENT_LENGTH + 1024)) {
        SSL3_SendAlert(ss, alert_fatal, record_overflow);
        PORT_SetError(SSL_ERROR_RX_RECORD_TOO_LONG);
        return SECFailure;
    }

The fixed code:
    if (isTLS && databuf->len > MAX_FRAGMENT_LENGTH) {
        SSL3_SendAlert(ss, alert_fatal, record_overflow);
        PORT_SetError(SSL_ERROR_RX_RECORD_TOO_LONG);
        return SECFailure;
    }

Tim, would you agree?  You can r+ the above if you like.
Flags: needinfo?(martin.thomson)
(Reporter)

Comment 4

6 months ago
For what it's worth, that fix does look good to me - the check is performed post-decompression so there shouldn't be any issues from that point either.

Comment 5

5 months ago
assigning to Martin who provided a fix
Assignee: nobody → martin.thomson
(In reply to Martin Thomson [:mt:] from comment #3)
> Tim, would you agree?  You can r+ the above if you like.

I read the spec again carefully and you're right. At this point we should have 2^14 as the max buffer length. All possible overhead should've been removed at this point.

r=me

(Setting ni? so this isn't lost.)
Flags: needinfo?(ttaubert) → needinfo?(martin.thomson)
(Assignee)

Comment 7

5 months ago
Looking at this with the benefit of time, this definitely needs a test.  I need to stop being so lazy about fixes.

https://nss-review.dev.mozaws.net/D343
Flags: needinfo?(martin.thomson)
(Assignee)

Comment 8

5 months ago
https://hg.mozilla.org/projects/nss/rev/85b67377d2a844ae5f2dd2fa4c99335df244cbcb
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 3.32
You need to log in before you can comment on or make changes to this bug.