Closed Bug 1472747 Opened 7 years ago Closed 11 days ago

Incorrect handling of too big and too small Finished messages

Categories

(NSS :: Libraries, defect, P2)

Product:
NSS
Component:
Libraries
Version:
3.38
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: hkario, Unassigned)

Details

(Whiteboard: [tls13])

Attachments

(1 file)

Bug 1472747 - wrong alert for malformed TLS 1.3 Finished. r=#nss-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Using NSS c84a61acb17d. When NSS server receives a malformed Finished message (with payload that doesn't match the PRF used in connection), it aborts with illegal_parameter error instead of the TLS 1.3 mandated decode_error (as the message doesn't match the format specified in section 4.4.4 https://tools.ietf.org/html/draft-ietf-tls-tls13-28#page-75 ) Additionally, if the message is very long (>147000B), NSS will send a second alert: decode_error. Reproducer: openssl req -x509 -newkey rsa -keyout localhost.key \ -out localhost.crt -subj /CN=localhost -nodes -batch mkdir /tmp/nssdb certutil -N -d sql:/tmp/nssdb --empty-password openssl pkcs12 -export -passout pass: -out /tmp/localhost.p12 \ -inkey /tmp/localhost.key -in /tmp/localhost.crt -name localhost pk12util -i /tmp/localhost.p12 -d sql:/tmp/nssdb -W '' selfserv -d sql:/tmp/nssdb -p 4433 -V tls1.0: -H 1 -n localhost 2> server.err > server.log & nss_pid=$! git clone https://github.com/tomato42/tlsfuzzer pushd tlsfuzzer git clone https://github.com/tomato42/tlslite-ng .tlslite-ng ln -s .tlslite-ng/tlslite tlslite git clone https://github.com/warner/python-ecdsa .python-ecdsa ln -s .python-ecdsa/ecdsa ecdsa # necessary to negotiate the currently supported draft in NSS # works because draft-26 and draft-28 are binary compatible sed -i 's/TLS_1_3_DRAFT = (127, 26)/TLS_1_3_DRAFT = (127, 28)/' tlslite/constants.py PYTHONPATH=. python scripts/test-tls13-finished.py popd kill $nss_pi tlsfuzzer output: (snip) padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 0, pad_right 16777183 ... Error encountered while processing node <tlsfuzzer.expect.ExpectClose object at 0x7fa936b2b310> (child: None) with last message being: <tlslite.messages.Message object at 0x7fa93681b910> Error while processing Traceback (most recent call last): File "scripts/test-tls13-finished.py", line 421, in main runner.run() File "/home/hkario/dev/tlsfuzzer/tlsfuzzer/runner.py", line 210, in run RecordHeader2))) AssertionError: Unexpected message from peer: Alert(fatal, decode_error) padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 48, pad_right 0 ... Error encountered while processing node <tlsfuzzer.expect.ExpectAlert object at 0x7fa936b39350> (child: <tlsfuzzer.expect.ExpectClose object at 0x7fa936b39590>) with last message being: <tlslite.messages.Message object at 0x7fa936814590> Error while processing Traceback (most recent call last): File "scripts/test-tls13-finished.py", line 421, in main runner.run() File "/home/hkario/dev/tlsfuzzer/tlsfuzzer/runner.py", line 212, in run node.process(self.state, msg) File "/home/hkario/dev/tlsfuzzer/tlsfuzzer/expect.py", line 1080, in process raise AssertionError(problem_desc) AssertionError: Expected alert description "decode_error" does not match received "illegal_parameter" sanity ... OK Fuzzing TLS 1.3 Finished messages version: 1 Test end successful: 642 failed: 68 'empty - cipher TLS_AES_128_GCM_SHA256' 'empty - cipher TLS_AES_256_GCM_SHA384' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 0, pad_right 1' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 0, pad_right 2' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 0, pad_right 4' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 0, pad_right 8' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 0, pad_right 16' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 0, pad_right 32' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 0, pad_right 48' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 0, pad_right 16348' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 0, pad_right 16777183' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 1, pad_right 0' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 1, pad_right 1' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 2, pad_right 0' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 4, pad_right 0' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 8, pad_right 0' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 8, pad_right 8' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 12, pad_right 0' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 16, pad_right 0' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 32, pad_right 0' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 48, pad_right 0' 'padding - cipher TLS_AES_128_GCM_SHA256, pad_byte 0, pad_left 16348, pad_right 0' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 0, pad_right 1' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 0, pad_right 2' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 0, pad_right 4' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 0, pad_right 8' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 0, pad_right 12' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 0, pad_right 16' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 0, pad_right 32' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 0, pad_right 48' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 0, pad_right 16332' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 0, pad_right 16777167' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 1, pad_right 0' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 1, pad_right 1' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 2, pad_right 0' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 4, pad_right 0' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 8, pad_right 0' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 8, pad_right 8' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 16, pad_right 0' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 32, pad_right 0' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 48, pad_right 0' 'padding - cipher TLS_AES_256_GCM_SHA384, pad_byte 0, pad_left 16332, pad_right 0' 'truncation - cipher TLS_AES_128_GCM_SHA256, start 0, end 12' 'truncation - cipher TLS_AES_128_GCM_SHA256, start 0, end -1' 'truncation - cipher TLS_AES_128_GCM_SHA256, start 0, end -2' 'truncation - cipher TLS_AES_128_GCM_SHA256, start 0, end -4' 'truncation - cipher TLS_AES_128_GCM_SHA256, start 0, end -8' 'truncation - cipher TLS_AES_128_GCM_SHA256, start 0, end -16' 'truncation - cipher TLS_AES_128_GCM_SHA256, start 0, end -32' 'truncation - cipher TLS_AES_128_GCM_SHA256, start 1, end None' 'truncation - cipher TLS_AES_128_GCM_SHA256, start 2, end None' 'truncation - cipher TLS_AES_128_GCM_SHA256, start 4, end None' 'truncation - cipher TLS_AES_128_GCM_SHA256, start 8, end None' 'truncation - cipher TLS_AES_128_GCM_SHA256, start 16, end None' 'truncation - cipher TLS_AES_128_GCM_SHA256, start 32, end None' 'truncation - cipher TLS_AES_256_GCM_SHA384, start 0, end 12' 'truncation - cipher TLS_AES_256_GCM_SHA384, start 0, end -1' 'truncation - cipher TLS_AES_256_GCM_SHA384, start 0, end -2' 'truncation - cipher TLS_AES_256_GCM_SHA384, start 0, end -4' 'truncation - cipher TLS_AES_256_GCM_SHA384, start 0, end -8' 'truncation - cipher TLS_AES_256_GCM_SHA384, start 0, end -16' 'truncation - cipher TLS_AES_256_GCM_SHA384, start 0, end -32' 'truncation - cipher TLS_AES_256_GCM_SHA384, start 1, end None' 'truncation - cipher TLS_AES_256_GCM_SHA384, start 2, end None' 'truncation - cipher TLS_AES_256_GCM_SHA384, start 4, end None' 'truncation - cipher TLS_AES_256_GCM_SHA384, start 8, end None' 'truncation - cipher TLS_AES_256_GCM_SHA384, start 16, end None' 'truncation - cipher TLS_AES_256_GCM_SHA384, start 32, end None' NSS output: Received incorrect handshakes hash values from peer. selfserv: HDX PR_Read returned error -12252: SSL received a malformed Finished handshake message. selfserv: HDX PR_Read returned error -12252:
Priority: -- → P2
QA Contact: jjones
Whiteboard: [tls13]
Severity: normal → S3

tls13_VerifyFinished was sending illegal_parameter for Finished messages
with wrong-length payloads. RFC 8446 requires decode_error for length
errors.

The tlsfuzzer test-tls13-finished.py was marked exp_pass:false because it
expected decode_error but received illegal_parameter. This caused an
intermittent CI failure: when none of the wrong-length test cases were
sampled, all 42 tests passed and the exp_pass:false caused the CI runner
to mark the script as FAILED.

Padding test cases with total Finished body > MAX_HANDSHAKE_MSG_LEN (131071
bytes) are excluded because they hit a separate rejection path in
ssl3_HandleHandshake (ssl3con.c). NSS correctly sends decode_error for
those too, but the tlsfuzzer runner's generator error-handling skips the
ExpectAlert node and jumps to ExpectClose, which then unexpectedly reads
the buffered alert.

Attachment #9550413 - Attachment description: Bug 1472747 - wrong alert for malformed TLS 1.3 Finished. r=djackson → Bug 1472747 - wrong alert for malformed TLS 1.3 Finished. r=#nss-reviewers

Pushed by jschanck@mozilla.com:
https://hg.mozilla.org/projects/nss/rev/683c49a0f7ed
wrong alert for malformed TLS 1.3 Finished. r=nss-reviewers,keeler

Status: NEW → RESOLVED
Closed: 11 days ago
Resolution: --- → FIXED
Pushed by jschanck@mozilla.com: https://hg.mozilla.org/projects/nss/rev/002f8482a612 Backed out changeset 683c49a0f7ed
Pushed by jschanck@mozilla.com: https://hg.mozilla.org/projects/nss/rev/106caff9567d wrong alert for malformed TLS 1.3 Finished. r=nss-reviewers,keeler
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: