Fix handling of padded TLS fragments in handshake #58
Conversation
|
esp8266/Arduino#3661 is the reason for this PR. I have no idea why it says CI failed, as far as I can see everything built just fine... |
earlephilhower
force-pushed
the
paddingfix
branch
6 times, most recently
from
4ff6062
to
c560109
Compare
When multiple handshake messages are sent in a single fragment, there may be some padding (i.e. pkt_size will be larger than the amount of bytes processed). When this happens, the old code would only advance the working pointer to the end of processed data, which would not be the start of the next packet per the sent pkt_size, causing handshake failure. Now simply advance the working pointer to the next packet irrespective of how many bytes in the current one were processed in the server_hello message. Also fix a CI problem introduced when the Arduino core common.sh started checking for a valid defined BUILD_TYPE.
earlephilhower
force-pushed
the
paddingfix
branch
from
c560109
to
1e70c67
Compare
|
The CI build was busted due to an update to test/common.sh in the Arduino repo. Including the fix for it here as it's pretty trivial. |
|
@igrr Thanks! Would you like to rebuild and commit the new library for the Arduino core? I can do it, if needed, but didn't want to step on your toes. Thx |
|
Thanks for the reminder @earlephilhower, created esp8266/Arduino#5125. |
| PARANOIA_CHECK(pkt_size, offset); | ||
| ssl->dc->bm_proc_index = pkt_size; | ||
| /* This check not always valid w/padding: */ | ||
| /* PARANOIA_CHECK(pkt_size, offset); */ |
There was a problem hiding this comment.
@earlephilhower That change does not seem correct to me. I read carefully the protocol and there it says that if there is a padding, then it will come in the form of an extension (0x0015: extension type=padding), which will have length and value. So in total the packet size must match the offset. If it does not happen then I would assume that the remote server is misbehaving. Can you paste here a hex dump of such a packet? How can I reproduce the issue ?
There was a problem hiding this comment.
Sorry, but I don't have a hex dump of a server that did this combining of units, but I think the websites referenced in esp8266/Arduino#3661 show that behavior in combining messages. I did look at the packets sent out to debug this issue, so I did see it coming across that way.
This is from memory, so apologies if it's a bit hazy:
IIRC the actual message size said one thing (which was I think 4-byte aligned length) whereas the message contents themselves were less than this. I think the unused trailing parts of the message length were filled with 0xff. Basically the server wanted to word-align messages and not byte-pack them one after the other. I read the spec and it seems like it's legal (or at least not specifically disallowed) as long as they are ignored and not parsed in any way to change the message interpretation.
FWIW, I believe BearSSL does the same "advance pointer to messagestart+messagelen" and not "advance pointer to amount of data I've processed in this message" bit. It didn't have an issue with the mentioned websites, and I do believe it is a much more paranoid TLS implementation.
When multiple handshake messages are sent in a single fragment, there
may be some padding (i.e. pkt_size will be larger than the amount of bytes
processed). When this happens, the old code would only advance the working
pointer to the end of processed data, which would not be the start of the
next packet per the sent pkt_size, causing handshake failure.
Now simply advance the working pointer to the next packet irrespective
of how many bytes in the current one were processed in the server_hello
message.