Skip to content

Detect EOF while reading in libssl (master)#10907

Closed
mattcaswell wants to merge 2 commits intoopenssl:masterfrom
mattcaswell:detect-eof
Closed

Detect EOF while reading in libssl (master)#10907
mattcaswell wants to merge 2 commits intoopenssl:masterfrom
mattcaswell:detect-eof

Conversation

@mattcaswell
Copy link
Copy Markdown
Member

@mattcaswell mattcaswell commented Jan 20, 2020

If we hit an EOF while reading in libssl then we will report an error
back to the application (SSL_ERROR_SYSCALL) but errno will be 0. We add
an error to the stack (which means we instead return SSL_ERROR_SSL) and
therefore give a hint as to what went wrong.

Contains a partial fix for #10880

This is the master version of #10882. I've left #10882 in WIP for now, but this one is ready for review.

I have a feeling that we are likely to see a number of reports as a result of this change going in. It turns out to be rather easy to hit the new "unexpected eof while reading" error that this change introduces.

For example if you start s_server:

$ openssl s_server

And then start s_client:

$ openssl s_client

Simply pressing ctrl-C on the s_server side results in this error on the s_client side:

40:C2:55:46:1B:7F:00:00:error:SSL routines::unexpected eof while reading:ssl/record/rec_layer_s3.c:306:

Previously we got this rather enigmatic and unhelpful message:

read:errno=0

Similarly doing it the other way around (pressing ctrl-c on the s_client side) we see the same "unexpected eof while reading" displayed by s_server. Previously it just said "ERROR" with no additional information.

I think this is an improvement and more accurately helps people to diagnose the problem. However I suspect that lots of people encounter this at the moment and it is logged one way in their logs - and this change will give a different error message - which may be a surprise to lots of people.

With all of that in mind we should consider whether we want to backport this to 1.1.1, or whether this bug fix is a step too far there.

@mattcaswell
Detect EOF while reading in libssl
8133813 
If we hit an EOF while reading in libssl then we will report an error
back to the application (SSL_ERROR_SYSCALL) but errno will be 0. We add
an error to the stack (which means we instead return SSL_ERROR_SSL) and
therefore give a hint as to what went wrong.

Contains a partial fix for openssl#10880
@mattcaswell mattcaswell added the branch: master Applies to master branch label Jan 20, 2020
@slontis
Copy link
Copy Markdown
Member

slontis commented Jan 22, 2020

Are there any other bss_ files that should be handling BIO_FLAGS_IN_EOF?

@mattcaswell
Copy link
Copy Markdown
Member Author

mattcaswell commented Jan 24, 2020

Are there any other bss_ files that should be handling BIO_FLAGS_IN_EOF?

I added an additional commit to teach more BIOs how to respond to BIO_CTRL_EOF

@mattcaswell
Copy link
Copy Markdown
Member Author

mattcaswell commented Feb 3, 2020

Ping

@beldmit beldmit added the approval: done This pull request has the required number of approvals label Feb 3, 2020
@iamamoose iamamoose added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Feb 4, 2020
openssl-machine pushed a commit that referenced this pull request Feb 4, 2020
@mattcaswell
Detect EOF while reading in libssl
d924dbf 
If we hit an EOF while reading in libssl then we will report an error
back to the application (SSL_ERROR_SYSCALL) but errno will be 0. We add
an error to the stack (which means we instead return SSL_ERROR_SSL) and
therefore give a hint as to what went wrong.

Contains a partial fix for #10880

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #10907)
openssl-machine pushed a commit that referenced this pull request Feb 4, 2020
@mattcaswell
Teach more BIOs how to handle BIO_CTRL_EOF
6d53ad6 
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #10907)
@mattcaswell
Copy link
Copy Markdown
Member Author

mattcaswell commented Feb 4, 2020

Pushed. Thanks.

@mattcaswell mattcaswell closed this Feb 4, 2020
@mattcaswell mattcaswell mentioned this pull request Feb 4, 2020
Closed
@bsdjhb
Copy link
Copy Markdown
Contributor

bsdjhb commented Mar 5, 2020

So I now get the new error even without pressing Ctrl-C in openssl s_client. That is, a normal s_client request that should end normally now fails with the error when previously with 1.1.1 it didn't report an error:

OpenSSL 1.1.1d-freebsd 10 Sep 2019:

# openssl s_client -host oe1 -port 443 -msg -tls1_2
CONNECTED(00000003)
...
---
GET /
>>> ??? [length 0005]
    17 03 03 00 1e
<<< ??? [length 0005]
    17 03 03 13 b5
HTTP/1.0 200 ok
Content-type: text/html

...
   0 callback cache hits
   0 cache full overflows (128 allowed)
---
no client certificate available
</pre></BODY></HTML>

read:errno=0
>>> ??? [length 0005]
    15 03 03 00 1a
>>> TLS 1.2, Alert [length 0002], warning close_notify
    01 00

This is what I get with 'master' after this commit:

# openssl s_client -host oe1 -port 443 -msg -tls1_2
CONNECTED(00000003)
...
---
GET /
>>> ??? [length 0005]
    17 03 03 00 1e
<<< ??? [length 0005]
    17 03 03 13 b5
HTTP/1.0 200 ok
Content-type: text/html

...
   0 callback cache hits
   0 cache full overflows (128 allowed)
---
no client certificate available
</pre></BODY></HTML>

00:10:B7:00:08:00:00:00:error:SSL routines::unexpected eof while reading:../ssl/record/rec_layer_s3.c:307:

@bsdjhb
Copy link
Copy Markdown
Contributor

bsdjhb commented Mar 24, 2020

I'm getting the same new error on 1.1.1e since this commit was merged there.

@szemere szemere mentioned this pull request Mar 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@beldmit beldmit beldmit approved these changes

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mattcaswell @slontis @bsdjhb @beldmit @iamamoose