Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix handling of padded TLS fragments in handshake #58

Merged
merged 1 commit into from
Sep 11, 2018
Merged

Fix handling of padded TLS fragments in handshake #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,12 @@ script:
- export PATH="$HOME/arduino_ide:$PATH"
- which arduino
- cd $ESP8266_ARDUINO_DIR
# 2.4.2 common.sh errors out if there is no valid build_type set, so ignore error
- sed -i 's/exit 1//' tests/common.sh
- source tests/common.sh
- arduino --board esp8266com:esp8266:generic --save-prefs
- arduino --get-pref sketchbook.path
- build_sketches $HOME/arduino_ide $ESP8266_ARDUINO_DIR/libraries/ESP8266WiFi/examples/HTTPSRequest
- build_sketches $HOME/arduino_ide $ESP8266_ARDUINO_DIR/libraries/ESP8266WiFi/examples/HTTPSRequest "-l $ESP8266_ARDUINO_DIR/libraries" 1 0
# Feel free to add more test cases (for other environments) here

notifications:
Expand Down
5 changes: 3 additions & 2 deletions ssl/tls1_clnt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,8 +392,9 @@ static int process_server_hello(SSL *ssl)
}
}

ssl->dc->bm_proc_index = offset;
PARANOIA_CHECK(pkt_size, offset);
ssl->dc->bm_proc_index = pkt_size;
/* This check not always valid w/padding: */
/* PARANOIA_CHECK(pkt_size, offset); */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


error:
return ret;
Expand Down