Closed Bug 1471967 Opened 2 years ago Closed 2 years ago

selfserv aborts the connection when the PSK identity is unrecognised (0-RTT intolerance)

Categories

(NSS :: Libraries, defect)

3.38
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: hkario, Assigned: ueno)

Details

Attachments

(1 file)

When the PSK identity of the client is unrecognised, the selfserv aborts the connection with illegal_parameter error.

Using c84a61acb17d
and openssl 358ffa05cd3a0 (few commits after 1.1.1-pre8)

Setup:
1. Compile openssl
2. openssl req -x509 -newkey rsa -keyout /tmp/localhost.key -out \
   /tmp/localhost.crt -subj /CN=localhost -nodes -batch
3. mkdir /tmp/nssdb
4. certutil -N -d sql:/tmp/nssdb --empty-password
5. openssl pkcs12 -export -passout pass: -out /tmp/localhost.p12 \
   -inkey /tmp/localhost.key -in /tmp/localhost.crt -name localhost
6. pk12util -i /tmp/localhost.p12 -d sql:/tmp/nssdb -W ''

Create session data so that openssl will send a PSK:
1. openssl s_server -key /tmp/localhost.key -cert /tmp/localhost.crt\
   -www > /tmp/server.log 2> server.err &
2. ossl_pid=$!
3. openssl s_client -connect localhost:4433 -CAfile /tmp/localhost.crt\
   -sess_out /tmp/sess.pem < /dev/null
4. kill $ossl_pid

Reproducer:
1. selfserv" -d sql:/tmp/nssdb -p 4433 -V tls1.0: -H 1 -n localhost -u
2. openssl s_client -connect localhost:4433 -CAfile /tmp/localhost.crt\
   -sess_in /tmp/sess.pem < /dev/null

OpenSSL error:
140538105800512:error:14094417:SSL routines:ssl3_read_bytes:sslv3 alert illegal parameter:ssl/record/rec_layer_s3.c:1523:SSL alert number 47

NSS error:
selfserv: HDX PR_Read returned error -12260:
SSL received a malformed Client Hello handshake message.
I suspect that this is because we decide it's a malformed ticket and because this is a "can't happen" we abort. 

Hubert, can you say where the error is happening?
Flags: needinfo?(hkario)
but malformed identity is supposed to be ignored, malformed binder is the one that _may_ cause connection abort

given that the identity comes from openssl, I don't see how NSS can say that that identity is valid, so it should ignore it

> can you say where the error is happening?

you mean in code? no, not easily, never traced NSS before
Flags: needinfo?(hkario)
This also impacts 0-RTT tolerance (https://tools.ietf.org/html/draft-ietf-tls-tls13-28#page-58 ), meaning that a server is unable to correctly process messages that came from a client that previously received ticket that allowed early data.

Reproducer setup is the same, selfserv is started the same way.

tlsfuzzer reproducer:
git clone https://github.com/tomato42/tlsfuzzer
pushd tlsfuzzer
# won't be needed after tlsfuzzer#423 is merged
git checkout 0rtt-garbage-resumption
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
PYTHONPATH=. python scripts/test-tls13-0rtt-garbage.py


tlsfuzzer output:
sanity ...
OK

handshake with invalid PSK ...
Error encountered while processing node <tlsfuzzer.expect.ExpectServerHello object at 0x7fe0ce338d50> (child: <tlsfuzzer.expect.ExpectChangeCipherSpec object at 0x7fe0ce338c50>) with last message being: <tlslite.messages.Message object at 0x7fe0ce2c7ed0>
Error while processing
Traceback (most recent call last):
  File "scripts/test-tls13-0rtt-garbage.py", line 262, in main
    runner.run()
  File "/home/hkario/dev/tlsfuzzer/tlsfuzzer/runner.py", line 210, in run
    RecordHeader2)))
AssertionError: Unexpected message from peer: Alert(fatal, illegal_parameter)

handshake with invalid 0-RTT ...
Error encountered while processing node <tlsfuzzer.expect.ExpectServerHello object at 0x7fe0ce2c7350> (child: <tlsfuzzer.expect.ExpectChangeCipherSpec object at 0x7fe0ce2c7250>) with last message being: <tlslite.messages.Message object at 0x7fe0ce284410>
Error while processing
Traceback (most recent call last):
  File "scripts/test-tls13-0rtt-garbage.py", line 262, in main
    runner.run()
  File "/home/hkario/dev/tlsfuzzer/tlsfuzzer/runner.py", line 210, in run
    RecordHeader2)))
AssertionError: Unexpected message from peer: Alert(fatal, illegal_parameter)

sanity ...
OK

Basic check if TLS 1.3 server can handle 0-RTT handshake
Verify that the server can handle a 0-RTT handshake from client
even if (or rather, especially if) it doesn't support 0-RTT.

version: 1

Test end
successful: 2
failed: 2
  'handshake with invalid 0-RTT'
  'handshake with invalid PSK'


NSS output:
selfserv: HDX PR_Read returned error -12260:
SSL received a malformed Client Hello handshake message.
selfserv: HDX PR_Read returned error -12260:
SSL received a malformed Client Hello handshake message.
Summary: selfserv aborts the connection when the PSK identity is unrecognised → selfserv aborts the connection when the PSK identity is unrecognised (0-RTT intolerance)
It's indeed failing in decrypting a malformed ticket, while the comment says it shouldn't abort:
https://searchfox.org/mozilla-central/source/security/nss/lib/ssl/tls13exthandle.c#503

I think this happens because ticket's key name check is done a bit too late.  I will file a patch soon.
In TLS 1.3, upon receiving a malformed ticket, server doesn't immediately abort the connection, but rejects client's resumption attempt.
After applying the change, most of the tlsfuzzer tests succeed, except the following:

successful: 8
failed: 3
  'handshake with 0-RTT, HRR and early data after 2nd Client Hello'
  'handshake with invalid 0-RTT and HRR'
  'undecryptable record later in handshake together with early_data'
It turned out that these 2 are false-positive (I forgot to add --cookie option to the test script):
  'handshake with 0-RTT, HRR and early data after 2nd Client Hello'
  'handshake with invalid 0-RTT and HRR'

I am not sure about this:
  'undecryptable record later in handshake together with early_data'
which interleaves client Finished message with an early data.
(In reply to Daiki Ueno [:ueno] from comment #7)
> It turned out that these 2 are false-positive (I forgot to add --cookie
> option to the test script):
>   'handshake with 0-RTT, HRR and early data after 2nd Client Hello'

this one sends early_data after the 2nd Client Hello, that's a protocol violation, and if the failure is on anything but wrong alert description, then it's a problem

>   'handshake with invalid 0-RTT and HRR'

that's a big problem, as that will cause connection aborts (it's a positive test case)
 
> I am not sure about this:
>   'undecryptable record later in handshake together with early_data'
> which interleaves client Finished message with an early data.

it's testing if the server will accept early data records (will perform trial decryption) only right after first Client Hello (the first test above checks for the case of early data between 2nd CH and Finished), while this one tests for it in a fragmented Finished
I meant the former 2 tests are now passing after adding --cookie.
The third one is failing with a timeout.
(In reply to Daiki Ueno [:ueno] from comment #9)
> I meant the former 2 tests are now passing after adding --cookie.
> The third one is failing with a timeout.

that would indicate that the server isn't handling it correctly – is not noticing the unexpected message (with bad MAC/tag) after receiving a properly encrypted record
Hubert, Daiki, I'm a little unclear on where
Sorry, saved to soon

1. Marking security because Hubert's c10 sounds kind of bad, and so let's sort it out.
2. I'm a little unclear on where we stand.
2.1. Per c8, is --cookie the right way to run these, so these tests are now fine?
2.2. Per c10, what is the test case here? It sounds like it is:

  1. Finished [partial]
  2. <Undecryptable packet>
  3. Finished [remainder]

Is that correct? Based on reading the code, I would actually expect that handshake to complete (which I agree is a defect). Or perhaps you only send packets #1 and #2? That would cause a hang, I think.
Group: crypto-core-security
MT, the issue with point 2.2. that I see is that we only disable the trial decryption discard when we have a full handshake message.
Yes, I agree that this is a problem, but not a serious one.  The junk record can't affect our state.  In some ways it might be better to allow it, rather than to have a partial Finished affecting state in this fashion.  On the other hand, the Finished is protected with handshake keys, so the odds that that message is inauthentic are low enough for this to be largely an academic concern.

We can drop trial decryption sooner, but it's only going to add more warts to the code.  That is, when a handshake record is successfully decrypted, we can disable trial decryption, rather than waiting for the full message to arrive.
(In reply to Eric Rescorla (:ekr) from comment #12)
> 2.1. Per c8, is --cookie the right way to run these, so these tests are now fine?

yes, --cookie is needed as the tests expect a very specific set of extensions in the messages from the server, and server is free to send cookie even if we don't advertise it, so the test needs to be told to expect the cookie extension (and reply with it)

> 2.2. Per c10, what is the test case here? It sounds like it is:
> 
>   1. Finished [partial]
>   2. <Undecryptable packet>
>   3. Finished [remainder]
> 
> Is that correct? Based on reading the code, I would actually expect that
> handshake to complete (which I agree is a defect). Or perhaps you only send
> packets #1 and #2? That would cause a hang, I think.

it's the second situation, the test sends just #1 and #2, you can change the test case to the former by adding

    from tlsfuzzer.messages import FlushMessageList
    node = node.add_child(FlushMessageList(finished_fragments))

after line 476 (after this node):

    # early data spliced into the Finished message
    node = node.add_child(PlaintextMessageGenerator(
        ContentType.application_data,
        getRandomBytes(64)))


(In reply to Martin Thomson [:mt:] from comment #14)
> Yes, I agree that this is a problem, but not a serious one.  The junk record
> can't affect our state.  In some ways it might be better to allow it, rather
> than to have a partial Finished affecting state in this fashion.  On the
> other hand, the Finished is protected with handshake keys, so the odds that
> that message is inauthentic are low enough for this to be largely an
> academic concern.
> 
> We can drop trial decryption sooner, but it's only going to add more warts
> to the code.  That is, when a handshake record is successfully decrypted, we
> can disable trial decryption, rather than waiting for the full message to
> arrive.

I worry if the trial decryption is not disabled right after the first decryptable record, that the attacker will be able to do truncation on real 0-RTT data (or in general, mess with 0-RTT data)
> I worry if the trial decryption is not disabled right after the first decryptable record, that the attacker will be able to do truncation on real 0-RTT data (or in general, mess with 0-RTT data)

We only do trial decryption when 0-RTT is rejected.  So any amount of messing with that data is possible, but harmless.
If we ever want to fix it, I guess it would be sufficient to move this in tls13_HandlePostHelloHandshakeMessage():
https://searchfox.org/mozilla-central/source/security/nss/lib/ssl/tls13con.c#817

to the place where the first fragment of non-ClientHello handshake message is decrypted, which is maybe around:
https://searchfox.org/mozilla-central/source/security/nss/lib/ssl/tls13con.c#5049

Would it be acceptable? If so I can amend the patch.
Comment on attachment 9002005 [details]
Bug 1471967, skip unrecognized session tickets in TLS 1.3

Eric Rescorla (:ekr) has approved the revision.
Attachment #9002005 - Flags: review+
Thank you for the review; I have pushed it as:
https://hg.mozilla.org/projects/nss/rev/5bc69334e84f

For the Finished fragmentation issue, as it's not really related to this it would be better tracked as a separate bug.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.39
Assignee: nobody → dueno
Group: crypto-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.