From 401e4961ee600bdb8af3443d2dbfbf7d06bb2852 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Mon, 18 Aug 2025 17:10:35 +0200 Subject: [PATCH] socks_sspi: simplify, clean up Curl_SOCKS5_gssapi_negotiate This function returned error on MANY places, each with its own cleanup sequence and by the look of it almost all of them were incomplete, making them leak resources on errors. This take now gotos to the error label where it cleans everything up before returning error. This also simplifies the function a lot. Closes #18315 --- lib/socks_sspi.c | 286 ++++++++++++++++++++--------------------------- 1 file changed, 120 insertions(+), 166 deletions(-) diff --git a/lib/socks_sspi.c b/lib/socks_sspi.c index 1e80ee18d9..49210585b0 100644 --- a/lib/socks_sspi.c +++ b/lib/socks_sspi.c @@ -90,7 +90,6 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, unsigned char socksreq[4]; /* room for GSS-API exchange header only */ const char *service = data->set.str[STRING_PROXY_SERVICE_NAME] ? data->set.str[STRING_PROXY_SERVICE_NAME] : "rcmd"; - const size_t service_length = strlen(service); /* GSS-API request looks like * +----+------+-----+----------------+ @@ -101,20 +100,12 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, */ /* prepare service name */ - if(strchr(service, '/')) { + if(strchr(service, '/')) service_name = strdup(service); - if(!service_name) - return CURLE_OUT_OF_MEMORY; - } - else { - service_name = malloc(service_length + - strlen(conn->socks_proxy.host.name) + 2); - if(!service_name) - return CURLE_OUT_OF_MEMORY; - msnprintf(service_name, service_length + - strlen(conn->socks_proxy.host.name) + 2, "%s/%s", - service, conn->socks_proxy.host.name); - } + else + service_name = aprintf("%s/%s", service, conn->socks_proxy.host.name); + if(!service_name) + return CURLE_OUT_OF_MEMORY; input_desc.cBuffers = 1; input_desc.pBuffers = &sspi_recv_token; @@ -132,6 +123,10 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, sspi_send_token.cbBuffer = 0; sspi_send_token.pvBuffer = NULL; + sspi_w_token[0].pvBuffer = + sspi_w_token[1].pvBuffer = + sspi_w_token[2].pvBuffer = NULL; + wrap_desc.cBuffers = 3; wrap_desc.pBuffers = sspi_w_token; wrap_desc.ulVersion = SECBUFFER_VERSION; @@ -139,21 +134,19 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, cred_handle.dwLower = 0; cred_handle.dwUpper = 0; - status = Curl_pSecFn->AcquireCredentialsHandle(NULL, + names.sUserName = NULL; + + status = + Curl_pSecFn->AcquireCredentialsHandle(NULL, (TCHAR *)CURL_UNCONST(TEXT("Kerberos")), - SECPKG_CRED_OUTBOUND, - NULL, - NULL, - NULL, - NULL, - &cred_handle, - &expiry); + SECPKG_CRED_OUTBOUND, + NULL, NULL, NULL, NULL, + &cred_handle, &expiry); if(check_sspi_err(data, status, "AcquireCredentialsHandle")) { failf(data, "Failed to acquire credentials."); - free(service_name); - Curl_pSecFn->FreeCredentialsHandle(&cred_handle); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } (void)curlx_nonblock(sock, FALSE); @@ -164,24 +157,24 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, TCHAR *sname; sname = curlx_convert_UTF8_to_tchar(service_name); - if(!sname) - return CURLE_OUT_OF_MEMORY; - - status = Curl_pSecFn->InitializeSecurityContext(&cred_handle, - context_handle, - sname, - ISC_REQ_MUTUAL_AUTH | - ISC_REQ_ALLOCATE_MEMORY | - ISC_REQ_CONFIDENTIALITY | - ISC_REQ_REPLAY_DETECT, - 0, - SECURITY_NATIVE_DREP, - &input_desc, - 0, - &sspi_context, - &output_desc, - &sspi_ret_flags, - &expiry); + if(!sname) { + result = CURLE_OUT_OF_MEMORY; + goto error; + } + + status = + Curl_pSecFn->InitializeSecurityContext(&cred_handle, context_handle, + sname, + ISC_REQ_MUTUAL_AUTH | + ISC_REQ_ALLOCATE_MEMORY | + ISC_REQ_CONFIDENTIALITY | + ISC_REQ_REPLAY_DETECT, + 0, SECURITY_NATIVE_DREP, + &input_desc, 0, + &sspi_context, + &output_desc, + &sspi_ret_flags, + &expiry); curlx_unicodefree(sname); @@ -192,13 +185,9 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, } if(check_sspi_err(data, status, "InitializeSecurityContext")) { - free(service_name); - Curl_pSecFn->FreeCredentialsHandle(&cred_handle); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - if(sspi_recv_token.pvBuffer) - Curl_pSecFn->FreeContextBuffer(sspi_recv_token.pvBuffer); failf(data, "Failed to initialise security context."); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } if(sspi_send_token.cbBuffer) { @@ -211,14 +200,8 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, &written); if(code || (written != 4)) { failf(data, "Failed to send SSPI authentication request."); - free(service_name); - if(sspi_send_token.pvBuffer) - Curl_pSecFn->FreeContextBuffer(sspi_send_token.pvBuffer); - if(sspi_recv_token.pvBuffer) - Curl_pSecFn->FreeContextBuffer(sspi_recv_token.pvBuffer); - Curl_pSecFn->FreeCredentialsHandle(&cred_handle); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } code = Curl_conn_cf_send(cf->next, data, @@ -226,16 +209,9 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, sspi_send_token.cbBuffer, FALSE, &written); if(code || (sspi_send_token.cbBuffer != written)) { failf(data, "Failed to send SSPI authentication token."); - free(service_name); - if(sspi_send_token.pvBuffer) - Curl_pSecFn->FreeContextBuffer(sspi_send_token.pvBuffer); - if(sspi_recv_token.pvBuffer) - Curl_pSecFn->FreeContextBuffer(sspi_recv_token.pvBuffer); - Curl_pSecFn->FreeCredentialsHandle(&cred_handle); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } - } if(sspi_send_token.pvBuffer) { @@ -266,29 +242,23 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, result = Curl_blockread_all(cf, data, (char *)socksreq, 4, &actualread); if(result || (actualread != 4)) { failf(data, "Failed to receive SSPI authentication response."); - free(service_name); - Curl_pSecFn->FreeCredentialsHandle(&cred_handle); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } /* ignore the first (VER) byte */ if(socksreq[1] == 255) { /* status / message type */ failf(data, "User was rejected by the SOCKS5 server (%u %u).", (unsigned int)socksreq[0], (unsigned int)socksreq[1]); - free(service_name); - Curl_pSecFn->FreeCredentialsHandle(&cred_handle); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } if(socksreq[1] != 1) { /* status / message type */ failf(data, "Invalid SSPI authentication response type (%u %u).", (unsigned int)socksreq[0], (unsigned int)socksreq[1]); - free(service_name); - Curl_pSecFn->FreeCredentialsHandle(&cred_handle); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } memcpy(&us_length, socksreq + 2, sizeof(short)); @@ -298,39 +268,32 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, sspi_recv_token.pvBuffer = malloc(us_length); if(!sspi_recv_token.pvBuffer) { - free(service_name); - Curl_pSecFn->FreeCredentialsHandle(&cred_handle); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_OUT_OF_MEMORY; + result = CURLE_OUT_OF_MEMORY; + goto error; } result = Curl_blockread_all(cf, data, (char *)sspi_recv_token.pvBuffer, sspi_recv_token.cbBuffer, &actualread); if(result || (actualread != us_length)) { failf(data, "Failed to receive SSPI authentication token."); - free(service_name); - if(sspi_recv_token.pvBuffer) - Curl_pSecFn->FreeContextBuffer(sspi_recv_token.pvBuffer); - Curl_pSecFn->FreeCredentialsHandle(&cred_handle); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } context_handle = &sspi_context; } - free(service_name); + Curl_safefree(service_name); /* Everything is good so far, user was authenticated! */ status = Curl_pSecFn->QueryCredentialsAttributes(&cred_handle, - SECPKG_CRED_ATTR_NAMES, - &names); + SECPKG_CRED_ATTR_NAMES, + &names); Curl_pSecFn->FreeCredentialsHandle(&cred_handle); if(check_sspi_err(data, status, "QueryCredentialAttributes")) { - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - Curl_pSecFn->FreeContextBuffer(names.sUserName); failf(data, "Failed to determine username."); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } else { #ifndef CURL_DISABLE_VERBOSE_STRINGS @@ -396,12 +359,12 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, } else { status = Curl_pSecFn->QueryContextAttributes(&sspi_context, - SECPKG_ATTR_SIZES, - &sspi_sizes); + SECPKG_ATTR_SIZES, + &sspi_sizes); if(check_sspi_err(data, status, "QueryContextAttributes")) { - Curl_pSecFn->DeleteSecurityContext(&sspi_context); failf(data, "Failed to query security context attributes."); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } sspi_w_token[0].cbBuffer = sspi_sizes.cbSecurityTrailer; @@ -409,16 +372,15 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, sspi_w_token[0].pvBuffer = malloc(sspi_sizes.cbSecurityTrailer); if(!sspi_w_token[0].pvBuffer) { - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_OUT_OF_MEMORY; + result = CURLE_OUT_OF_MEMORY; + goto error; } sspi_w_token[1].cbBuffer = 1; sspi_w_token[1].pvBuffer = malloc(1); if(!sspi_w_token[1].pvBuffer) { - Curl_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_OUT_OF_MEMORY; + result = CURLE_OUT_OF_MEMORY; + goto error; } memcpy(sspi_w_token[1].pvBuffer, &gss_enc, 1); @@ -426,42 +388,33 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, sspi_w_token[2].cbBuffer = sspi_sizes.cbBlockSize; sspi_w_token[2].pvBuffer = malloc(sspi_sizes.cbBlockSize); if(!sspi_w_token[2].pvBuffer) { - Curl_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer); - Curl_pSecFn->FreeContextBuffer(sspi_w_token[1].pvBuffer); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_OUT_OF_MEMORY; + result = CURLE_OUT_OF_MEMORY; + goto error; } status = Curl_pSecFn->EncryptMessage(&sspi_context, - KERB_WRAP_NO_ENCRYPT, - &wrap_desc, - 0); + KERB_WRAP_NO_ENCRYPT, + &wrap_desc, 0); if(check_sspi_err(data, status, "EncryptMessage")) { - Curl_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer); - Curl_pSecFn->FreeContextBuffer(sspi_w_token[1].pvBuffer); - Curl_pSecFn->FreeContextBuffer(sspi_w_token[2].pvBuffer); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); failf(data, "Failed to query security context attributes."); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } - sspi_send_token.cbBuffer = sspi_w_token[0].cbBuffer - + sspi_w_token[1].cbBuffer - + sspi_w_token[2].cbBuffer; + sspi_send_token.cbBuffer = sspi_w_token[0].cbBuffer + + sspi_w_token[1].cbBuffer + + sspi_w_token[2].cbBuffer; sspi_send_token.pvBuffer = malloc(sspi_send_token.cbBuffer); if(!sspi_send_token.pvBuffer) { - Curl_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer); - Curl_pSecFn->FreeContextBuffer(sspi_w_token[1].pvBuffer); - Curl_pSecFn->FreeContextBuffer(sspi_w_token[2].pvBuffer); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_OUT_OF_MEMORY; + result = CURLE_OUT_OF_MEMORY; + goto error; } memcpy(sspi_send_token.pvBuffer, sspi_w_token[0].pvBuffer, sspi_w_token[0].cbBuffer); memcpy((PUCHAR) sspi_send_token.pvBuffer +(int)sspi_w_token[0].cbBuffer, sspi_w_token[1].pvBuffer, sspi_w_token[1].cbBuffer); - memcpy((PUCHAR) sspi_send_token.pvBuffer - + sspi_w_token[0].cbBuffer - + sspi_w_token[1].cbBuffer, + memcpy((PUCHAR) sspi_send_token.pvBuffer + + sspi_w_token[0].cbBuffer + + sspi_w_token[1].cbBuffer, sspi_w_token[2].pvBuffer, sspi_w_token[2].cbBuffer); Curl_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer); @@ -482,10 +435,8 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, &written); if(code || (written != 4)) { failf(data, "Failed to send SSPI encryption request."); - if(sspi_send_token.pvBuffer) - Curl_pSecFn->FreeContextBuffer(sspi_send_token.pvBuffer); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } if(data->set.socks5_gssapi_nec) { @@ -494,8 +445,8 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, &written); if(code || (written != 1)) { failf(data, "Failed to send SSPI encryption type."); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } } else { @@ -504,10 +455,8 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, sspi_send_token.cbBuffer, FALSE, &written); if(code || (sspi_send_token.cbBuffer != (size_t)written)) { failf(data, "Failed to send SSPI encryption type."); - if(sspi_send_token.pvBuffer) - Curl_pSecFn->FreeContextBuffer(sspi_send_token.pvBuffer); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } if(sspi_send_token.pvBuffer) Curl_pSecFn->FreeContextBuffer(sspi_send_token.pvBuffer); @@ -516,23 +465,23 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, result = Curl_blockread_all(cf, data, (char *)socksreq, 4, &actualread); if(result || (actualread != 4)) { failf(data, "Failed to receive SSPI encryption response."); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } /* ignore the first (VER) byte */ if(socksreq[1] == 255) { /* status / message type */ failf(data, "User was rejected by the SOCKS5 server (%u %u).", (unsigned int)socksreq[0], (unsigned int)socksreq[1]); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } if(socksreq[1] != 2) { /* status / message type */ failf(data, "Invalid SSPI encryption response type (%u %u).", (unsigned int)socksreq[0], (unsigned int)socksreq[1]); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } memcpy(&us_length, socksreq + 2, sizeof(short)); @@ -541,8 +490,8 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, sspi_w_token[0].cbBuffer = us_length; sspi_w_token[0].pvBuffer = malloc(us_length); if(!sspi_w_token[0].pvBuffer) { - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_OUT_OF_MEMORY; + result = CURLE_OUT_OF_MEMORY; + goto error; } result = Curl_blockread_all(cf, data, (char *)sspi_w_token[0].pvBuffer, @@ -550,9 +499,8 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, if(result || (actualread != us_length)) { failf(data, "Failed to receive SSPI encryption type."); - Curl_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } @@ -563,30 +511,20 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, sspi_w_token[1].cbBuffer = 0; sspi_w_token[1].pvBuffer = NULL; - status = Curl_pSecFn->DecryptMessage(&sspi_context, - &wrap_desc, - 0, - &qop); + status = Curl_pSecFn->DecryptMessage(&sspi_context, &wrap_desc, + 0, &qop); if(check_sspi_err(data, status, "DecryptMessage")) { - if(sspi_w_token[0].pvBuffer) - Curl_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer); - if(sspi_w_token[1].pvBuffer) - Curl_pSecFn->FreeContextBuffer(sspi_w_token[1].pvBuffer); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); failf(data, "Failed to query security context attributes."); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } if(sspi_w_token[1].cbBuffer != 1) { failf(data, "Invalid SSPI encryption response length (%lu).", (unsigned long)sspi_w_token[1].cbBuffer); - if(sspi_w_token[0].pvBuffer) - Curl_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer); - if(sspi_w_token[1].pvBuffer) - Curl_pSecFn->FreeContextBuffer(sspi_w_token[1].pvBuffer); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } memcpy(socksreq, sspi_w_token[1].pvBuffer, sspi_w_token[1].cbBuffer); @@ -597,9 +535,8 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, if(sspi_w_token[0].cbBuffer != 1) { failf(data, "Invalid SSPI encryption response length (%lu).", (unsigned long)sspi_w_token[0].cbBuffer); - Curl_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer); - Curl_pSecFn->DeleteSecurityContext(&sspi_context); - return CURLE_COULDNT_CONNECT; + result = CURLE_COULDNT_CONNECT; + goto error; } memcpy(socksreq, sspi_w_token[0].pvBuffer, sspi_w_token[0].cbBuffer); Curl_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer); @@ -621,5 +558,22 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, } */ return CURLE_OK; +error: + free(service_name); + Curl_pSecFn->FreeCredentialsHandle(&cred_handle); + Curl_pSecFn->DeleteSecurityContext(&sspi_context); + if(sspi_recv_token.pvBuffer) + Curl_pSecFn->FreeContextBuffer(sspi_recv_token.pvBuffer); + if(sspi_send_token.pvBuffer) + Curl_pSecFn->FreeContextBuffer(sspi_send_token.pvBuffer); + if(names.sUserName) + Curl_pSecFn->FreeContextBuffer(names.sUserName); + if(sspi_w_token[0].pvBuffer) + Curl_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer); + if(sspi_w_token[1].pvBuffer) + Curl_pSecFn->FreeContextBuffer(sspi_w_token[1].pvBuffer); + if(sspi_w_token[2].pvBuffer) + Curl_pSecFn->FreeContextBuffer(sspi_w_token[2].pvBuffer); + return result; } #endif -- 2.47.2