Handle partial messages in TLSProxy by bernd-edlinger · Pull Request #5412 · openssl/openssl

bernd-edlinger · 2018-02-19T22:17:15Z

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 · 2018-02-27T07:29:24Z

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

bernd-edlinger · 2018-02-28T10:30:31Z

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 · 2018-02-28T10:34:52Z

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

bernd-edlinger · 2018-02-28T10:57:06Z

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 · 2018-03-04T15:02:53Z

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

bernd-edlinger · 2018-03-04T15:40:26Z

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 · 2018-03-05T12:21:24Z

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

So rebased, and removed WIP for now.

bernd-edlinger · 2018-03-21T14:47:40Z

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

bernd-edlinger · 2018-03-21T14:57:52Z

Rebased, because of merge conflicts.

mattcaswell · 2018-03-21T16:45:30Z

util/perl/TLSProxy/Proxy.pm

    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


Looks like this comment needs updating

[to be squashed]

Reviewed-by: Matt Caswell <[email protected]> (Merged from #5412)

bernd-edlinger · 2018-03-22T16:32:34Z

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 · 2018-03-22T17:15:18Z

backport to 1.1.0: see #5726

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 mentioned this pull request Mar 5, 2018

bnrand_range: Always call bnrand() with the correct flag #5514

Closed

bernd-edlinger force-pushed the proxy_handle_partial_messages branch from 9697637 to 6c530bc Compare March 5, 2018 12:17

bernd-edlinger changed the title ~~WIP: Handle partial messages in TLSProxy~~ Handle partial messages in TLSProxy Mar 5, 2018

bernd-edlinger added branch: master Applies to master branch 1.1.0 labels Mar 5, 2018

mattcaswell added this to the 1.1.1 milestone Mar 20, 2018

mattcaswell mentioned this pull request Mar 20, 2018

Building openssl-1.1.1-pre3 reports errors in 70-test_sslmessages, 70-test_tls13messages #5693

Closed

Handle partial messages in TLSProxy

adb7aad

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

mattcaswell requested changes Mar 21, 2018

View reviewed changes

Update comment

b26b25a

[to be squashed]

mattcaswell approved these changes Mar 22, 2018

View reviewed changes

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

Handle partial messages in TLSProxy

12636c1

Reviewed-by: Matt Caswell <[email protected]> (Merged from #5412)

bernd-edlinger closed this Mar 22, 2018

bernd-edlinger mentioned this pull request Mar 22, 2018

Handle partial messages in TLSProxy #5726

Closed

Uh oh!

Comments

Conversation

bernd-edlinger commented Feb 19, 2018

Uh oh!

bernd-edlinger commented Feb 27, 2018

Uh oh!

bernd-edlinger commented Feb 28, 2018

Uh oh!

bernd-edlinger commented Feb 28, 2018

Uh oh!

bernd-edlinger commented Feb 28, 2018

Uh oh!

bernd-edlinger commented Mar 4, 2018

Uh oh!

bernd-edlinger commented Mar 4, 2018

Uh oh!

bernd-edlinger commented Mar 5, 2018

Uh oh!

bernd-edlinger commented Mar 21, 2018

Uh oh!

bernd-edlinger commented Mar 21, 2018

Uh oh!

mattcaswell Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

bernd-edlinger Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

bernd-edlinger commented Mar 22, 2018

Uh oh!

bernd-edlinger commented Mar 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants