openssl: Fragmented handshake messages with app data between them are not rejected

Using 1980ce45d6b

When OpenSSL receives handshake protocol messages that are fragmented over multiple records split by application data, the connection is not aborted.

This is a RFC 8446 section 5.1 violation:

   -  Handshake messages MUST NOT be interleaved with other record
      types.  That is, if a handshake message is split over two or more
      records, there MUST NOT be any other records between them.

Interestingly, when the message is fragmented in a 4 byte and 1 byte record, the server exhibits correct behaviour.

There’s also a minor issue with parsing the message: when it is too large (2 instead of 1 byte), the server replies with illegal_parameter rather than the expected decode_error.

reproducer

openssl req -x509 -newkey rsa -keyout localhost.key -out localhost.crt -subj /CN=localhost -nodes -batch
openssl s_server -key localhost.key -cert localhost.crt -www 2>server.err >server.out &
openssl_pid=$!
git clone https://github.com/tomato42/tlsfuzzer
pushd tlsfuzzer
# won't be needed after https://github.com/tomato42/tlsfuzzer/pull/501 is merged
git checkout key-update-handshake
git clone https://github.com/tomato42/tlslite-ng .tlslite-ng
pushd .tlslite-ng
# won't be needed after https://github.com/tomato42/tlslite-ng/pull/341 is merged
git checkout key-update-support
popd
ln -s .tlslite-ng/tlslite tlslite
git clone https://github.com/warner/python-ecdsa .python-ecdsa
ln -s .python-ecdsa/src/ecdsa ecdsa
PYTHONPATH=. python scripts/test-tls13-keyupdate.py -e 'app data split, conversation with KeyUpdate msg'
popd
kill $openssl_pid

OpenSSL output

Using default temp DH parameters
ACCEPT
...
140131671496512:error:14160098:SSL routines:read_state_machine:excessive message size:ssl/statem/statem.c:601:
...
140131671496512:error:1420607A:SSL routines:tls_process_key_update:bad key update:ssl/statem/statem_lib.c:648:
140131671496512:error:1420607A:SSL routines:tls_process_key_update:bad key update:ssl/statem/statem_lib.c:648:
140131671496512:error:1420607A:SSL routines:tls_process_key_update:bad key update:ssl/statem/statem_lib.c:648:
140131671496512:error:1420607A:SSL routines:tls_process_key_update:bad key update:ssl/statem/statem_lib.c:648:
140131671496512:error:1420607A:SSL routines:tls_process_key_update:bad key update:ssl/statem/statem_lib.c:648:
140131671496512:error:1420607A:SSL routines:tls_process_key_update:bad key update:ssl/statem/statem_lib.c:648:
140131671496512:error:1420607A:SSL routines:tls_process_key_update:bad key update:ssl/statem/statem_lib.c:648:

tlsfuzzer output

large KeyUpdate message ...
Error encountered while processing node <tlsfuzzer.expect.ExpectAlert object at 0x7f39b52a6150> (child: <tlsfuzzer.expect.ExpectClose object at 0x7f39b52a6210>) with last message being: <tlslite.messages.Message object at 0x7f39b5069c90>
Error while processing
Traceback (most recent call last):
  File "scripts/test-tls13-keyupdate.py", line 575, in main
    runner.run()
  File "/home/hkario/dev/tlsfuzzer/tlsfuzzer/runner.py", line 223, in run
    node.process(self.state, msg)
  File "/home/hkario/dev/tlsfuzzer/tlsfuzzer/expect.py", line 1366, in process
    raise AssertionError(problem_desc)
AssertionError: Expected alert description "decode_error" does not match received "illegal_parameter"

...

3/2 fragmented keyupdate msg, appdata between ...
Error encountered while processing node ExpectNewSessionTicket() (child: ExpectNewSessionTicket()) with last message being: <tlslite.messages.Message object at 0x7f39b4d76b90>
Error while processing
Traceback (most recent call last):
  File "scripts/test-tls13-keyupdate.py", line 575, in main
    runner.run()
  File "/home/hkario/dev/tlsfuzzer/tlsfuzzer/runner.py", line 221, in run
    RecordHeader2)))
AssertionError: Unexpected message from peer: ApplicationData(len=3863)

...

2/3 fragmented keyupdate msg, appdata between ...
Error encountered while processing node ExpectNewSessionTicket() (child: ExpectNewSessionTicket()) with last message being: <tlslite.messages.Message object at 0x7f39b4d7be10>
Error while processing
Traceback (most recent call last):
  File "scripts/test-tls13-keyupdate.py", line 575, in main
    runner.run()
  File "/home/hkario/dev/tlsfuzzer/tlsfuzzer/runner.py", line 221, in run
    RecordHeader2)))
AssertionError: Unexpected message from peer: ApplicationData(len=3863)

Test with KeyUpdate msg with different msg_type or fragmented msg.
Verify that server will correctly handle updating the keys
or refuse the connection with relevant Alert msg.
version: 1

Test end
successful: 262
failed: 4
  '1/4 fragmented keyupdate msg, appdata between'
  '2/3 fragmented keyupdate msg, appdata between'
  '3/2 fragmented keyupdate msg, appdata between'
  'large KeyUpdate message'

See also #8188 for ‘app data split, conversation with KeyUpdate msg’ test case

About this issue

Original URL
State: closed
Created 5 years ago
Comments: 22 (22 by maintainers)

Commits related to this issue

Don't interleave handshake and other record types in TLSv1.3 In TLSv1.3 it is illegal to interleave handshake records with non handshake records. Fixes #8189 — committed to mattcaswell/openssl by mattcaswell 5 years ago
Don't interleave handshake and other record types in TLSv1.3 In TLSv1.3 it is illegal to interleave handshake records with non handshake records. Fixes #8189 Reviewed-by: Ben Kaduk <kaduk@mit.edu> ... — committed to openssl/openssl by mattcaswell 5 years ago

Most upvoted comments

While the KeyUpdate size limit is derivable from the message format, the Certificate size limit is not. For DoS reasons, OpenSSL will and must reject large Certificate messages, even if they are syntactically valid. This is true of every TLS implementation as 2^24 bytes = 16MiB is too much to buffer per connection.

I believe a strict reading of the spec would point to illegal_parameter for this scenario, as nothing is out of bounds of the formal protocol syntax. It just happens that the KeyUpdate size limit is derivable from the formal protocol syntax, but the TLS library still hasn’t gotten far enough to notice it. (It cannot. The DoS prevention must kick in before allowing that much data to buffer up.)

This mess is a clear demonstration of why we should be folding those two alerts together. It is a waste of everyone’s time to keep debating the theological applicability of decode_error vs illegal_parameter in each case. unexpected_message can go too, as that will also vary by implementation strategy. If TLS 1.4 ever happens, hopefully we can correct this mess going forward. I would even support a document that just retroactively states all exists TLS versions can fold those alerts together going forward and we move on from this.

davidben on Feb 20, 2019

I disagree about the importance of fingerprinting which TLS stack is being used. (A simpler method is to look at the order of extensions, for example.) It won’t help attacks, people try all known attacks, and it doesn’t seem to me that it helps identify individual users. Can you explain why you think this is important, and is it more important than other things such as making the AES C code be constant time?

richsalz on Feb 14, 2019

I suspect there are a huge number of other areas where there are differences in implementation.

in the past year I see 11 bugs filed that mention tlsfuzzer, about half of which are complaining about wrong alert responses; currently we have about 25% of coverage done for TLS 1.3 so we are talking about something like 20 additional bugs about such issues for all of TLS 1.3. Even with older protocols included and additional extensions, I don’t expect it to more than triple. That’s a “huge number”?

I was referring to “differences in implementation”. I wasn’t specifically restricting that to alerts. If the objective is to avoid fingerprinting then, yes, I think there are huge number of ways to achieve that (not just alerts).

For something that as a “side effect” provides a comprehensive tests for error handling in the protocol?

There is no reason why you can’t still produce comprehensive tests. I just think you’re perception of what the “right” answer is for those tests is not correct. IMO the tests should accept a wider range of acceptable responses.

we’d have to just copy what some other implementation does.

no, we’d have to reach an agreement between multiple implementations what to do; sometimes it means OpenSSL needs to change, sometimes it means that other implementations need to change and sometimes it means that everybody needs to change (like in handling of 0-RTT Client Hello). But obviously I don’t file bugs against OpenSSL when it has the expected behaviour…

So what you’re asking for is (effectively) another standard to tighten these areas up. I can’t see that happening any time soon and I doubt there would be much support for it from other implementations.

mattcaswell on Feb 14, 2019

I consider the two checks to be checking slightly different things. One is checking the semantic feasibility of a particular value. The other is a syntax check. Hence different alert codes are used. We went around these exact same circles in #6631.

mattcaswell on Feb 11, 2019