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.

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

openssl: Revert to less sensitivity for SYSCALL errors (releases only) #4623

Closed
wants to merge 1 commit into from

Conversation

@jay
Copy link
Member

@jay jay commented Nov 20, 2019

Prior to this change 0ab38f5 (precedes 7.67.0) increased the sensitivity
of OpenSSL's SSL_ERROR_SYSCALL error so that abrupt server closures were
also considered errors. For example, a server that does not send a known
protocol termination point (eg HTTP content length or chunked encoding)
and does not send a TLS termination point (close_notify alert) would
cause an error if it closed the connection.

To be clear that behavior made it into release build 7.67.0
unintentionally. So far there is just one user report due to it.

Ultimately the idea is a good one, and other SSL backends may already
behave similarly (such as Windows native OS SSL Schannel). However much
more of our user base is using OpenSSL and there is a mass of legacy
users in that space, so I think that behavior should be partially
reverted and then rolled out slowly.

This commit changes the behavior so that the increased sensitivity is
disabled in curl release builds but remains enabled in curl development
builds. If after a period of time there are no major issues then it can
be enabled for OpenSSL 1.1.1+ in curl release builds.

Bug: #4409 (comment)
Reported-by: Bjoern Franke

Closes #xxxx


/cc @bjo81

@jay
Copy link
Member Author

@jay jay commented Nov 20, 2019

Fixes #4624

LIBCURL_DEV_BUILD defined:

> curld https://www.presseportal.de 1>NUL
curl: (56) OpenSSL SSL_read: Connection closed abruptly, errno 0 (Fatal because this is a curl developer's build)

LIBCURL_DEV_BUILD undefined:

> curld https://www.presseportal.de 1>NUL
[No error or error message shown, exit code 0]
@bagder
Copy link
Member

@bagder bagder commented Nov 21, 2019

I'm for reverting that behavior change.

But I don't like the LIBCURL_DEV_BUILD thing. I think we should use the CURLDEBUG instead so that only the ones of us who really want to debug curl gets that to occur - so many users build directly from source/git that I don't think we should affect them that directly.

@jay
Copy link
Member Author

@jay jay commented Nov 21, 2019

I considered DEBUGBUILD but I'd guess there are very few people running that in practice. If users are building from the repo I would assume they're almost always building a development build. To me it seems reasonable, as any development build may have issues that would not be in a release build.

@bagder
Copy link
Member

@bagder bagder commented Nov 21, 2019

If users are building from the repo I would assume they're almost always building a development build

I think we would want that to be true but I fear it is not. There are even cases of operating systems building curl like that and shipping it to billions of users. We have rather large amount of users who build straight from git and deploy to production.

- Disable the extra sensitivity except in debug builds (--enable-debug).

- Improve SYSCALL error message logic in ossl_send and ossl_recv so that
  "No error" / "Success" socket error text isn't shown on SYSCALL error.

Prior to this change 0ab38f5 (precedes 7.67.0) increased the sensitivity
of OpenSSL's SSL_ERROR_SYSCALL error so that abrupt server closures were
also considered errors. For example, a server that does not send a known
protocol termination point (eg HTTP content length or chunked encoding)
_and_ does not send a TLS termination point (close_notify alert) would
cause an error if it closed the connection.

To be clear that behavior made it into release build 7.67.0
unintentionally. So far there is just one user report due to it.

Ultimately the idea is a good one, and other SSL backends may already
behave similarly (such as Windows native OS SSL Schannel). However much
more of our user base is using OpenSSL and there is a mass of legacy
users in that space, so I think that behavior should be partially
reverted and then rolled out slowly.

This commit changes the behavior so that the increased sensitivity is
disabled in all curl builds except curl debug builds (DEBUGBUILD). If
after a period of time there are no major issues then it can be enabled
in dev and release builds with the newest OpenSSL (1.1.1+).

Bug: #4409 (comment)
Reported-by: Bjoern Franke

Fixes #4624
Closes #4623
@jay jay force-pushed the jay:ossl_rollback_syscall_on_read branch from 3eeac3f to d741f50 Nov 22, 2019
@jay
Copy link
Member Author

@jay jay commented Nov 22, 2019

Ok I changed it to DEBUGBUILD. Also I improved on the SYSCALL error message logic for ossl_send and ossl_recv.

@bagder
bagder approved these changes Nov 22, 2019
@jay jay closed this in 78cef06 Nov 23, 2019
@jay jay deleted the jay:ossl_rollback_syscall_on_read branch Nov 23, 2019
@archon810
Copy link

@archon810 archon810 commented Dec 6, 2019

This change breaks Facebook oembeds from WordPress. Took us a while to track it down to the exact curl version.

Is the curl version that fixes this known at this point?

@bagder
Copy link
Member

@bagder bagder commented Dec 6, 2019

This regression is (only) present in 7.67.0. The fix is in git and will be part of the pending 7.68.0 release.

@anthonypeyton
Copy link

@anthonypeyton anthonypeyton commented Dec 7, 2019

@bagder Do you have an ETA on the release? This broke some things for my client too and if the release is soon I can hold off updating the AMI rather than rolling back.

@bagder
Copy link
Member

@bagder bagder commented Dec 7, 2019

Next release date is always present here. 7.68.0 is set to ship on January 8th 2020.

@archon810
Copy link

@archon810 archon810 commented Jan 11, 2020

curl 7.68 is out and fixes this issue.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.