1512605 - Incorrect alert description after unencrypted Finished msg

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[robert.kolcun]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36

Steps to reproduce:

Info:
When unencrypted Finished msg is sent instead of encrypted one, server should reject this connection with unexpected_message Alert. NSS rejects this connection with bad_record_mac alert.

Steps to reproduce:
git clone https://github.com/tomato42/tlsfuzzer.git
pushd tlsfuzzer
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

# in another terminal, same directory
PYTHONPATH=tlsfuzzer python tlsfuzzer/scripts/test-tls13-finished-plaintext.py


Actual results:

tlsfuzzer output:

sanity ...
OK

Unencrypted Finished message ...
Error encountered while processing node <tlsfuzzer.expect.ExpectAlert object at 0x7f5a506099b0> (child: <tlsfuzzer.expect.ExpectClose object at 0x7f5a506099e8>) with last message being: <tlslite.messages.Message object at 0x7f5a50609f28>
Error while processing
Traceback (most recent call last):
  File "scripts/test-tls13-finished-plaintext.py", line 187, in main
    runner.run()
  File "/home/rkolcun/tls_repo/tlsfuzzer/tlsfuzzer/runner.py", line 219, in run
    node.process(self.state, msg)
  File "/home/rkolcun/tls_repo/tlsfuzzer/tlsfuzzer/expect.py", line 1273, in process
    raise AssertionError(problem_desc)
AssertionError: Expected alert description "unexpected_message" does not match received "bad_record_mac"

sanity ...
OK

Unencrypted Finished message in TLS 1.3 conversation.
Check that unencrypted Finished message is rejected
with an unexpected_message Alert by a server.

version: 1

Test end
successful: 2
failed: 1
  'Unencrypted Finished message'

NSS output:

selfserv: HDX PR_Read returned error -12273:
SSL received a record with an incorrect Message Authentication Code.



Expected results:

Pass the tests. NSS should send Alert message with an 'unexpected_message' description.

Comment 1

[Eric Rescorla (:ekr)]

Why do you believe that it should supply this alert. It seems like there are two problems:

- The message isn't valid for the current cipher suite
- It is of the wrong type.

We are triggering on the first, logically prior error. Why do you think that that's wrong?

Comment 2

[J.C. Jones [:jcj] (he/they)]

Robert, can you respond to Eric (comment 1)?

Comment 3

[J.C. Jones [:jcj] (he/they)]

Resolving this. Feel free to reopen w/ a response to the questions above.

Comment 4

[Alicja Kario]

Unencrypted message means that the real content type is in the record header, not hidden by encryption.
IOW, the TLSCiphertext.opaque_type is wrong.

The expected alert is not spelled out in the RFC for Handshake messages specifically, but it is spelled out for an opposite situation with change_cipher_spec.
See RFC 8446, section 5:
An
implementation which receives any other change_cipher_spec value or
which receives a protected change_cipher_spec record MUST abort the
handshake with an "unexpected_message" alert.

Side note: given that the opaque_type and legacy_record_version in TLSCiphertext are defined as static values, one could argue that the correct alert would be decode_error. I'd argue that this would be inconsistent with other situations in which unexpected_alert is expected. At the very least it would require the same handling for legacy_record_version.

But I really don't see how bad_record_mac is the correct one – the server shouldn't even attempt to decrypt a message with invalid header.

Comment 5

[Eric Rescorla (:ekr)]

Marked this security till we know what's going on, because I agree we shouldn't be decrypting a message with an invalid header.

That said, I cannot reproduce this. When I try to run tlsfuzzer, I get:

07:00:12|~/dev/nss-dev$ PYTHONPATH=tlsfuzzer python tlsfuzzer/scripts/test-tls13-finished-plaintext.py
Traceback (most recent call last):
  File "tlsfuzzer/scripts/test-tls13-finished-plaintext.py", line 11, in <module>
    from tlsfuzzer.runner import Runner
  File "/Users/ekr/dev/nss-dev/tlsfuzzer/tlsfuzzer/runner.py", line 8, in <module>
    from tlslite.messages import Message, Certificate, RecordHeader2
  File "/Users/ekr/dev/nss-dev/tlsfuzzer/tlslite/__init__.py", line 27, in <module>
    from tlslite.api import *
  File "/Users/ekr/dev/nss-dev/tlsfuzzer/tlslite/api.py", line 5, in <module>
    from .constants import AlertLevel, AlertDescription, Fault
  File "/Users/ekr/dev/nss-dev/tlsfuzzer/tlslite/constants.py", line 11, in <module>
    from .utils.compat import a2b_hex
  File "/Users/ekr/dev/nss-dev/tlsfuzzer/tlslite/utils/compat.py", line 13, in <module>
    import ecdsa
ImportError: No module named ecdsa

I've written my own test and will attach it in Phabricator shortly. That test behaves as expected:

68266: TLS13[39171808]: spec=36528960 epoch=2 (handshake data) unprotect 0x0 len=40
68266: TLS13[39171808]: record has invalid exterior type=16
68266: SSL3[39171808]: send alert record, level=2 desc=10
68266: SSL3[39171808] SendRecord type: alert      (21) nIn=2
68266: TLS13[39171808]: spec=41853328 epoch=3 (application data) protect 0x0 len=2
server: Fatal alert sent: 10
server: Handshake failed with error SSL_ERROR_RX_UNEXPECTED_RECORD_TYPE: SSL received an unexpected record type.
server: Changing state from CONNECTING to ERROR
68266: SSL[39148464]: closing, rv=0 errno=-12109
68266: SSL[39171808]: closing, rv=0 errno=-12109

Comment 6

[Eric Rescorla (:ekr)]

Attachment: Test for bug 1512605

Test for bug 1512605.

Comment 7

[Alicja Kario]

ImportError: No module named ecdsa

the instructions are missing the following after pushd tlsfuzzer:

git clone https://github.com/warner/python-ecdsa.git .python-ecdsa
ln -s .python-ecdsa/ecdsa ecdsa

(or just pip install ecdsa)

I can't reproduce it with 3.43.0
It does fail with 3.40.0 though, same error as in comment #0.

Comment 8

[Martin Thomson [:mt:]]

This was fixed in Bug 1507179, which landed in 3.41, thanks to Daiki. We always rejected the record, but that bug created a specific error code for the condition and generated an alert.

We could just mark this as WORKSFORME, but do we want to keep ekr's test case?

Comment 9

[Alicja Kario]

I see no reason why not to merge ekr's test case...

Comment 10

[J.C. Jones [:jcj] (he/they)]

Agreed; let's unhide this and land the test. I'll do so tomorrow unless I hear reason to otherwise.

Comment 11

[Eric Rescorla (:ekr)]

Great. Glad that I am not totally out to lunch.

Comment 12

[Martin Thomson [:mt:]]

I've updated the test case. I tried doing this for <1.3, but then remembered that we don't have decryption capabilities in our test harnesses for that.

Interesting side note, calling SSL_ResetHandshake at the wrong time did not stop this test from passing thanks to bug 1303149.

Comment 13

[J.C. Jones [:jcj] (he/they)]

Unhiding (comment 10)

Comment 14

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/14203267aba1afa9f823ac486256211ac009a07c