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

Jump to bottom

Regression in SecureTransport #10227

Closed
hjmallon opened this issue Jan 4, 2023 · 1 comment
Closed

Regression in SecureTransport #10227

hjmallon opened this issue Jan 4, 2023 · 1 comment
Assignees
@icing
Labels
regression TLS

Comments

@hjmallon
Copy link
Contributor

hjmallon commented Jan 4, 2023

I did this

This is reproduceable with curl and libcurl. I originally saw it in libcurl but I bisected it in curl.

curl https://google.co.uk/

# Sometimes you will get (with 7.87)
curl: (35) Internal SSL engine error encountered during the SSL handshake

# Sometimes you will get the correct response (with 7.87)
...301 Moved...

# However with curl 7.47 it works every time.

I expected the following

It to work every time like 7.47 does.

curl/libcurl version

% ./src/curl -V
curl 7.87.0-DEV (x86_64-apple-darwin22.2.0) libcurl/7.87.0-DEV SecureTransport zlib/1.2.11 brotli/1.0.9 zstd/1.5.2 libidn2/2.3.4 nghttp2/1.51.0 librtmp/2.3
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps mqtt pop3 pop3s rtmp rtsp smb smbs smtp smtps telnet tftp
Features: alt-svc AsynchDNS brotli HSTS HTTP2 HTTPS-proxy IDN IPv6 Largefile libz NTLM NTLM_WB SSL threadsafe UnixSockets zstd

operating system

Darwin pixmac570 22.2.0 Darwin Kernel Version 22.2.0: Fri Nov 11 02:08:47 PST 2022; root:xnu-8792.61.2~4/RELEASE_X86_64 x86_64

git bisect

I have done a git bisect with

autoreconf -fi
./configure --with-secure-transport --without-openssl
make -j10
./src/curl https://google.co.uk/

and it says the first bad commit it

commit 55807e6c056f27846d70cec70ee6ac3f0e5b3bbe
Author: Stefan Eissing <stefan@eissing.org>
Date:   Fri Nov 25 14:06:43 2022 +0100

    tls: backends use connection filters for IO, enabling HTTPS-proxy
    
     - OpenSSL (and compatible)
     - BearSSL
     - gnutls
     - mbedtls
     - rustls
     - schannel
     - secure-transport
     - wolfSSL (v5.0.0 and newer)
    
     This leaves only the following without HTTPS-proxy support:
     - gskit
     - nss
     - wolfSSL (versions earlier than v5.0.0)
    
    Closes #9962

 CMakeLists.txt       |   5 +-
 configure.ac         |  13 ++-
 lib/cfilters.c       |  31 +++++-
 lib/cfilters.h       | 115 ++++++++++++----------
 lib/connect.c        |  10 --
 lib/ftp.c            |   2 +-
 lib/http_proxy.c     |   8 --
 lib/imap.c           |   2 +-
 lib/pop3.c           |   2 +-
 lib/smtp.c           |   2 +-
 lib/vtls/bearssl.c   |  26 ++---
 lib/vtls/gtls.c      |  97 ++++++++++--------
 lib/vtls/mbedtls.c   |  50 ++++++++--
 lib/vtls/openssl.c   | 270 +++++++++++++++++++++++++++++++++++++++++----------
 lib/vtls/rustls.c    |  76 ++++++++++-----
 lib/vtls/schannel.c  |  44 ++++-----
 lib/vtls/sectransp.c | 151 +++++++++-------------------
 lib/vtls/vtls.c      | 104 +++++++++-----------
 lib/vtls/vtls.h      |  12 ---
 lib/vtls/vtls_int.h  |  13 ++-
 lib/vtls/wolfssl.c   | 179 +++++++++++++++++++++++++++++++++-
 m4/curl-wolfssl.m4   |   9 ++
 22 files changed, 796 insertions(+), 425 deletions(-)
@dfandrich dfandrich mentioned this issue Jan 4, 2023
Closed
icing added a commit to icing/curl that referenced this issue Jan 5, 2023
@icing
Fixing secure-transport IO for incomplete read/writes. Refs curl#10227.
ddd3d6f 
SecureTransport expects result code errSSLWouldBlock when the requested length
could not be sent/recieved in full. The previous code returned noErr, which let
SecureTransport to believe that the IO had terminated prematurely.
@icing
Copy link
Contributor

icing commented Jan 5, 2023

@hjmallon would you be able to check the change in #10235? In my local tests, this solves the problem.

@bagder bagder linked a pull request Jan 5, 2023 that will close this issue
Fixing secure-transport IO for incomplete read/writes. Refs #10227. #10235
Closed
@bagder bagder closed this as completed in 16bb32e Jan 5, 2023
@jay jay mentioned this issue Jan 27, 2023
Closed
bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
@icing @bch
sectransp: fix for incomplete read/writes
a1a799d 
SecureTransport expects result code errSSLWouldBlock when the requested
length could not be sent/recieved in full. The previous code returned
noErr, which let SecureTransport to believe that the IO had terminated
prematurely.

Fixes curl#10227
Closes curl#10235
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@icing icing

Labels
regression TLS
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixing secure-transport IO for incomplete read/writes. Refs #10227. icing/curl
3 participants
@icing @dfandrich @hjmallon