Skip to content

Handle partial messages in TLSProxy - #5412

Closed
bernd-edlinger wants to merge 2 commits into
openssl:masterfrom
bernd-edlinger:proxy_handle_partial_messages
Closed

Handle partial messages in TLSProxy#5412
bernd-edlinger wants to merge 2 commits into
openssl:masterfrom
bernd-edlinger:proxy_handle_partial_messages

Conversation

@bernd-edlinger

@bernd-edlinger bernd-edlinger commented Feb 19, 2018

Copy link
Copy Markdown
Member

Address a potential test failure when partial messages are received via Proxy socket.
(which might have happened with AppVeyor build #5370).

This is work in progress:
Some tests still fail of fragments are very short.

@bernd-edlinger
bernd-edlinger force-pushed the proxy_handle_partial_messages branch 2 times, most recently from 7f1697f to 037a554 Compare February 20, 2018 12:48
@bernd-edlinger

bernd-edlinger commented Feb 27, 2018

Copy link
Copy Markdown
Member Author

it happened again in an appveyor build:
https://ci.appveyor.com/project/openssl/openssl/build/master.15909

@bernd-edlinger

bernd-edlinger commented Feb 28, 2018

Copy link
Copy Markdown
Member Author

And here: https://ci.appveyor.com/project/openssl/openssl/build/master.15915

Received server packet
Packet length = 1460
Processing flight 1
 Record 1 (server -> client)
  Content type: HANDSHAKE
  Version: TLS1.2
  Length: 122
  Message type: ServerHello
  Message Length: 118
    Server Version:771
    Session ID Len:32
    Ciphersuite:4865
    Compression Method:0
    Extensions Len:46
 Record 2 (server -> client)
  Content type: CCS
  Version: TLS1.2
  Length: 1
 Record 3 (server -> client)
  Content type: APPLICATION DATA
  Version: TLS1.2
  Length: 23
  Inner content type: HANDSHAKE
  Message type: EncryptedExtensions
  Message Length: 2
    Extensions Len:0
 Record 4 (server -> client)
  Content type: APPLICATION DATA
  Version: TLS1.2
  Length: 1033
  Inner content type: HANDSHAKE
  Message type: Certificate
  Message Length: 1012
    Context:
    Certificate List Len:1008
    Certificate Len:1003
    Extensions Len:0
 Record 5 (server -> client)
  Content type: APPLICATION DATA
  Version: TLS1.2
  Length: 281 (expected), 256 (actual)
Use of uninitialized value within %record_type in concatenation (.) or string at C:\projects\openssl\util\perl/TLSProxy/Record.pm line 124.
  Inner content type: 
Forwarded packet length = 1460
Received server packet
Packet length = 83
Processing flight 2
 Record 1 (server -> client)
Use of uninitialized value within %record_type in concatenation (.) or string at C:\projects\openssl\util\perl/TLSProxy/Record.pm line 89.
  Content type: 
Use of uninitialized value within %tls_version in concatenation (.) or string at C:\projects\openssl\util\perl/TLSProxy/Record.pm line 90.
  Version: 
  Length: 47937 (expected), 78 (actual)
  Inner content type: HANDSHAKE
Use of uninitialized value within %message_type in concatenation (.) or string at C:\projects\openssl\util\perl/TLSProxy/Message.pm line 231.
  Message type: 
  Message Length: 14096662
Forwarded packet length = 83

It always happens in the AppVeyor x64 build.
Note the incomplete packet of 1460 bytes.

@bernd-edlinger

bernd-edlinger commented Feb 28, 2018

Copy link
Copy Markdown
Member Author

Here x86 was affected: https://ci.appveyor.com/project/openssl/openssl/build/master.15884

@bernd-edlinger

bernd-edlinger commented Feb 28, 2018

Copy link
Copy Markdown
Member Author

I think the difference between linux and windows is basically that
linux uses jumbo packets on the lo interface,
while windows splits the larger messages up into 1460 byte chunks.
This creates a race condition that makes ~5% of all CI builds fail.

@bernd-edlinger

bernd-edlinger commented Mar 4, 2018

Copy link
Copy Markdown
Member Author

Another failure here: https://ci.appveyor.com/project/openssl/openssl/build/master.15977

@bernd-edlinger

bernd-edlinger commented Mar 4, 2018

Copy link
Copy Markdown
Member Author

Currently this patch seems to work as long as fragments are not smaller than 30 bytes.
Windows seems to cut off TCP fragments at 1460 bytes.

The reason why test failures appear when the fragments are smaller than that,
is that in some tests the callback tries to filter/modify in-flight packets and it is no
longer transparent, that fragmentation has occurred, because I cannot tell for
sure if the current flight is over or there are still packets to come.

Well, I was able to get it a bit better, by looking at if there is a partial frame then
it is more likely that the current flight is not yet complete. But that is not
a 100% solution, as the packet could be split at exactly a message border.

@levitte, I would be interested in your opinion, is this good enough or
is there a better way how I could find out where the current flight ended.

@bernd-edlinger bernd-edlinger mentioned this pull request Mar 5, 2018
Closed
@bernd-edlinger
bernd-edlinger force-pushed the proxy_handle_partial_messages branch from 9697637 to 6c530bc Compare March 5, 2018 12:17
@bernd-edlinger bernd-edlinger changed the title WIP: Handle partial messages in TLSProxy Handle partial messages in TLSProxy Mar 5, 2018
@bernd-edlinger

bernd-edlinger commented Mar 5, 2018

Copy link
Copy Markdown
Member Author

It happened once again: https://ci.appveyor.com/project/openssl/openssl/build/master.16045

So rebased, and removed WIP for now.

@bernd-edlinger bernd-edlinger added branch: master Applies to master branch 1.1.0 labels Mar 5, 2018
@mattcaswell mattcaswell added this to the 1.1.1 milestone Mar 20, 2018
@mattcaswell mattcaswell mentioned this pull request Mar 20, 2018
Closed
@bernd-edlinger

bernd-edlinger commented Mar 21, 2018

Copy link
Copy Markdown
Member Author

another test failure here: https://ci.appveyor.com/project/openssl/openssl/build/master.16662

@bernd-edlinger
bernd-edlinger force-pushed the proxy_handle_partial_messages branch from 6c530bc to adb7aad Compare March 21, 2018 14:57
@bernd-edlinger

bernd-edlinger commented Mar 21, 2018

Copy link
Copy Markdown
Member Author

Rebased, because of merge conflicts.

mattcaswell
mattcaswell requested changes Mar 21, 2018
View reviewed changes
Comment thread util/perl/TLSProxy/Proxy.pm Outdated
print "Processing flight ".$self->flight."\n";

#Return contains the list of record found in the packet followed by the
#list of messages in those records

@mattcaswell mattcaswell Mar 21, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like this comment needs updating

@bernd-edlinger bernd-edlinger Mar 21, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ah, yes.

@bernd-edlinger
Update comment
b26b25a 
[to be squashed]
@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Mar 22, 2018
levitte pushed a commit that referenced this pull request Mar 22, 2018
@bernd-edlinger
Handle partial messages in TLSProxy
12636c1 
Reviewed-by: Matt Caswell <[email protected]>
(Merged from #5412)
@bernd-edlinger

bernd-edlinger commented Mar 22, 2018

Copy link
Copy Markdown
Member Author

Merged to master, 1.1.0 did unfortunately not cherry-pick.
Will send a new PR for 1.1.0 in a moment...

@bernd-edlinger bernd-edlinger mentioned this pull request Mar 22, 2018
Closed
@bernd-edlinger

bernd-edlinger commented Mar 22, 2018

Copy link
Copy Markdown
Member Author

backport to 1.1.0: see #5726

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattcaswell mattcaswell mattcaswell approved these changes

Assignees

No one assigned

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Milestone

1.1.1

Development

Successfully merging this pull request may close these issues.

2 participants

@bernd-edlinger @mattcaswell