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

Crash with WinSSL #3412

Closed
MarcelRaad opened this issue Dec 27, 2018 · 12 comments

Comments

Projects
None yet
5 participants
@MarcelRaad
Copy link
Member

commented Dec 27, 2018

I did this

See the WinSSL autobuilds since December 23 on https://curl.haxx.se/dev/builds.html:

test 2043...[Disable certificate revocation checks]
../src/curl.exe --output log/curl2043.out --include --trace-ascii log/trace2043 --trace-time --ssl-no-revoke -I https://revoked.grc.com/ >log/stdout2043 2>log/stderr2043
sh: line 1: 6648 Segmentation fault

I expected the following

No segmentation fault

curl/libcurl version

Daily tarball since December 23

operating system

Windows 10, MSYS2 shell, MinGW-w64

I'll try to have a look when I'm back from my holidays tomorrow.

@gvanem

This comment has been minimized.

Copy link
Member

commented Dec 27, 2018

Verified from the cmd-line using a MSVC-built curl.exe.
Here:

void Curl_ssl_sessionid_lock(struct connectdata *conn)
{
  if(SSLSESSION_SHARED(conn->data))

conn->data == NULL!

@danielgustafsson

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

Could fb445a1 and/or f3ce387 be related to this?

@MarcelRaad

This comment has been minimized.

Copy link
Member Author

commented Dec 28, 2018

Full call stack:

Curl_ssl_sessionid_lock(connectdata * conn=0x000001d48f950560) Line 286
Curl_schannel_shutdown(connectdata * conn=0x000001d48f950560, int sockindex=0) Line 2017
Curl_ssl_shutdown(connectdata * conn=0x000001d48f950560, int sockindex=0) Line 560
Curl_schannel_close(connectdata * conn=0x000001d48f950560, int sockindex=0) Line 1922
Curl_ssl_close(connectdata * conn=0x000001d48f950560, int sockindex=0) Line 556
Curl_disconnect(Curl_easy * data=0x000001d48f91eef0, connectdata * conn=0x000001d48f950560, bool dead_connection=false) Line 804
Curl_conncache_close_all_connections(conncache * connc=0x000001d48a21d9b0) Line 580
curl_multi_cleanup(Curl_multi * multi=0x000001d48a21d8c0) Line 2287
@MarcelRaad

This comment has been minimized.

Copy link
Member Author

commented Jan 1, 2019

Bisected to fb445a1.

@bagder

This comment has been minimized.

Copy link
Member

commented Jan 1, 2019

Thanks. I'll look into it asap...

@bagder

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

I would've expected f3ce387 to have addressed that issue since it brings back the conn->data assignment in Curl_disconnect:

curl/lib/url.c

Line 786 in 251cabf

conn->data = data;

@MarcelRaad can you see what exactly is wrong at vtls/vtls.c:286 ?

@MarcelRaad

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

What happens is that conn->data gets set to NULL at

curl/lib/url.c

Line 793 in ba266b3

Curl_conncache_remove_conn(data, conn, TRUE);
and the crash because of that happens at

curl/lib/url.c

Line 802 in ba266b3

Curl_ssl_close(conn, FIRSTSOCKET);
then with the call stack posted above.

According to the comment, the assignment in your snippet is for conn->handler->disconnect, which is not even called.

@bagder

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

I think it looks like a mistake that Curl_ssl_shutdown gets called when a single connection is closed (from within Curl_schannel_close). Looking at vtls/openssl:Curl_ossl_close() as a comparison it doesn't do that (and neither does any other TLS backend). There's a separate call to the shutdown function.

The shutdown function needs a 'data' pointer to find the session_id cache (and lock/unlock it).

I'm not sure what the best fix for this is now, but it might be to allow the shutdown to just skip the lock/unlock if there's no transfer associated with the connection anymore. Maybe like this:

diff --git a/lib/vtls/schannel.c b/lib/vtls/schannel.c
index 56fd93e1e..ef9b068fa 100644
--- a/lib/vtls/schannel.c
+++ b/lib/vtls/schannel.c
@@ -5,11 +5,11 @@
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
  * Copyright (C) 2012 - 2016, Marc Hoersken, <info@marc-hoersken.de>
  * Copyright (C) 2012, Mark Salisbury, <mark.salisbury@hp.com>
- * Copyright (C) 2012 - 2018, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 2012 - 2019, Daniel Stenberg, <daniel@haxx.se>, et al.
  *
  * This software is licensed as described in the file COPYING, which
  * you should have received as part of this distribution. The terms
  * are also available at https://curl.haxx.se/docs/copyright.html.
  *
@@ -2011,13 +2011,15 @@ static int Curl_schannel_shutdown(struct connectdata *conn, int sockindex)
     Curl_safefree(BACKEND->ctxt);
   }
 
   /* free SSPI Schannel API credential handle */
   if(BACKEND->cred) {
-    Curl_ssl_sessionid_lock(conn);
+    if(conn->data)
+      Curl_ssl_sessionid_lock(conn);
     Curl_schannel_session_free(BACKEND->cred);
-    Curl_ssl_sessionid_unlock(conn);
+    if(conn->data)
+      Curl_ssl_sessionid_unlock(conn);
     BACKEND->cred = NULL;
   }
 
   /* free internal buffer for received encrypted data */
   if(BACKEND->encdata_buffer != NULL) {
@MarcelRaad

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2019

@bagder Can you push that please? It's necessary for testing #3321.

bagder added a commit that referenced this issue Jan 18, 2019

@bagder bagder closed this in 6ee6729 Jan 18, 2019

@chris-araman

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

I'm still seeing something similar, even after 6ee6729.

I think we may need to check that data is not NULL here:

if(data->set.verbose || data->set.errorbuffer) {

...because Curl_schannel_shutdown also passes data to failf/Curl_failf. The infof/Curl_infof call already makes this check here:

if(data && data->set.verbose) {

However, it seems like anywhere we might call failf, it would be useful to have some context to be able to provide debugging information to the caller. So, maybe @bagder's hunch is correct, that we might need a more complete fix.
#3412 (comment)

I don't have a proposed fix, but I think for now I'm just going to try checking for data here and seeing how far I get.

if(sspi_status != SEC_E_OK)

@gvanem

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

@chris-araman

I think we may need to check that data is not NULL here:
[curl/lib/sendf.c]
(

if(data->set.verbose || data->set.errorbuffer) {
)

That is close to where issue #3505 crashes too (at line 162).
Different function, but the maybe the assumptions WRT to Schannel are perhaps related?

@chris-araman

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

@gvanem, thanks. I agree it looks like the same invariant is at issue in #3505. I will move my discussion there.

jay added a commit that referenced this issue Feb 6, 2019

url: close TLS before removing conn from cache
- Fix potential crashes in schannel shutdown.

Ensure any TLS shutdown messages are sent before removing the
association between the connection and the easy handle. Reverts
@bagder's previous partial fix for #3412.

Fixes #3412
Fixes #3505
Closes #3531

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.