From: Jay Satiro Date: Thu, 18 Sep 2025 06:07:17 +0000 (-0400) Subject: socks_sspi: Fix some memory cleanup calls X-Git-Tag: rc-8_17_0-1~354 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=f8175b1536610ec60a9d2dce5a055d59287af4d5;p=thirdparty%2Fcurl.git socks_sspi: Fix some memory cleanup calls - Ensure memory allocated by malloc() is freed by free(). Prior to this change SSPI's FreeContextBuffer() was sometimes used to free malloc'd memory. I can only assume the reason we have no crash reports about this is because the underlying heap free is probably the same for both. Reported-by: Joshua Rogers Fixes https://github.com/curl/curl/issues/18587 Closes https://github.com/curl/curl/pull/18594 --- diff --git a/lib/socks_sspi.c b/lib/socks_sspi.c index c106fec0c8..3ff3b723f9 100644 --- a/lib/socks_sspi.c +++ b/lib/socks_sspi.c @@ -90,6 +90,8 @@ 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"; + char *etbuf; + size_t etbuf_size; /* GSS-API request looks like * +----+------+-----+----------------+ @@ -131,11 +133,14 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, wrap_desc.pBuffers = sspi_w_token; wrap_desc.ulVersion = SECBUFFER_VERSION; - cred_handle.dwLower = 0; - cred_handle.dwUpper = 0; + memset(&cred_handle, 0, sizeof(cred_handle)); + memset(&sspi_context, 0, sizeof(sspi_context)); names.sUserName = NULL; + etbuf = NULL; + etbuf_size = 0; + status = Curl_pSecFn->AcquireCredentialsHandle(NULL, (TCHAR *)CURL_UNCONST(TEXT("Kerberos")), @@ -178,11 +183,8 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, curlx_unicodefree(sname); - if(sspi_recv_token.pvBuffer) { - Curl_pSecFn->FreeContextBuffer(sspi_recv_token.pvBuffer); - sspi_recv_token.pvBuffer = NULL; - sspi_recv_token.cbBuffer = 0; - } + Curl_safefree(sspi_recv_token.pvBuffer); + sspi_recv_token.cbBuffer = 0; if(check_sspi_err(data, status, "InitializeSecurityContext")) { failf(data, "Failed to initialise security context."); @@ -220,10 +222,7 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, } sspi_send_token.cbBuffer = 0; - if(sspi_recv_token.pvBuffer) { - Curl_pSecFn->FreeContextBuffer(sspi_recv_token.pvBuffer); - sspi_recv_token.pvBuffer = NULL; - } + Curl_safefree(sspi_recv_token.pvBuffer); sspi_recv_token.cbBuffer = 0; if(status != SEC_I_CONTINUE_NEEDED) @@ -289,7 +288,6 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, status = Curl_pSecFn->QueryCredentialsAttributes(&cred_handle, SECPKG_CRED_ATTR_NAMES, &names); - Curl_pSecFn->FreeCredentialsHandle(&cred_handle); if(check_sspi_err(data, status, "QueryCredentialAttributes")) { failf(data, "Failed to determine username."); result = CURLE_COULDNT_CONNECT; @@ -303,6 +301,7 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, curlx_unicodefree(user_utf8); #endif Curl_pSecFn->FreeContextBuffer(names.sUserName); + names.sUserName = NULL; } /* Do encryption */ @@ -399,35 +398,30 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, 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.pvBuffer = malloc(sspi_send_token.cbBuffer); - if(!sspi_send_token.pvBuffer) { + + etbuf_size = sspi_w_token[0].cbBuffer + + sspi_w_token[1].cbBuffer + + sspi_w_token[2].cbBuffer; + etbuf = malloc(etbuf_size); + if(!etbuf) { 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, + memcpy(etbuf, sspi_w_token[0].pvBuffer, sspi_w_token[0].cbBuffer); + memcpy(etbuf + 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(etbuf + 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); - sspi_w_token[0].pvBuffer = NULL; + Curl_safefree(sspi_w_token[0].pvBuffer); sspi_w_token[0].cbBuffer = 0; - Curl_pSecFn->FreeContextBuffer(sspi_w_token[1].pvBuffer); - sspi_w_token[1].pvBuffer = NULL; + Curl_safefree(sspi_w_token[1].pvBuffer); sspi_w_token[1].cbBuffer = 0; - Curl_pSecFn->FreeContextBuffer(sspi_w_token[2].pvBuffer); - sspi_w_token[2].pvBuffer = NULL; + Curl_safefree(sspi_w_token[2].pvBuffer); sspi_w_token[2].cbBuffer = 0; - us_length = htons((unsigned short)sspi_send_token.cbBuffer); + us_length = htons((unsigned short)etbuf_size); memcpy(socksreq + 2, &us_length, sizeof(short)); } @@ -450,16 +444,14 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, } } else { - code = Curl_conn_cf_send(cf->next, data, - (char *)sspi_send_token.pvBuffer, - sspi_send_token.cbBuffer, FALSE, &written); - if(code || (sspi_send_token.cbBuffer != (size_t)written)) { + code = Curl_conn_cf_send(cf->next, data, etbuf, etbuf_size, + FALSE, &written); + if(code || (etbuf_size != written)) { failf(data, "Failed to send SSPI encryption type."); result = CURLE_COULDNT_CONNECT; goto error; } - if(sspi_send_token.pvBuffer) - Curl_pSecFn->FreeContextBuffer(sspi_send_token.pvBuffer); + Curl_safefree(etbuf); } result = Curl_blockread_all(cf, data, (char *)socksreq, 4, &actualread); @@ -514,8 +506,16 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, status = Curl_pSecFn->DecryptMessage(&sspi_context, &wrap_desc, 0, &qop); + /* since sspi_w_token[1].pvBuffer is allocated by the SSPI in this case, it + must be freed in this block using FreeContextBuffer() instead of + potentially in error cleanup using free(). */ + if(check_sspi_err(data, status, "DecryptMessage")) { failf(data, "Failed to query security context attributes."); + if(sspi_w_token[1].pvBuffer) { + Curl_pSecFn->FreeContextBuffer(sspi_w_token[1].pvBuffer); + sspi_w_token[1].pvBuffer = NULL; + } result = CURLE_COULDNT_CONNECT; goto error; } @@ -523,13 +523,20 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, 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[1].pvBuffer) { + Curl_pSecFn->FreeContextBuffer(sspi_w_token[1].pvBuffer); + sspi_w_token[1].pvBuffer = NULL; + } result = CURLE_COULDNT_CONNECT; goto error; } memcpy(socksreq, sspi_w_token[1].pvBuffer, sspi_w_token[1].cbBuffer); - Curl_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer); + Curl_safefree(sspi_w_token[0].pvBuffer); + sspi_w_token[0].cbBuffer = 0; Curl_pSecFn->FreeContextBuffer(sspi_w_token[1].pvBuffer); + sspi_w_token[1].pvBuffer = NULL; + sspi_w_token[1].cbBuffer = 0; } else { if(sspi_w_token[0].cbBuffer != 1) { @@ -539,7 +546,8 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, goto error; } memcpy(socksreq, sspi_w_token[0].pvBuffer, sspi_w_token[0].cbBuffer); - Curl_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer); + Curl_safefree(sspi_w_token[0].pvBuffer); + sspi_w_token[0].cbBuffer = 0; } (void)curlx_nonblock(sock, TRUE); @@ -557,24 +565,24 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(struct Curl_cfilter *cf, conn->socks5_sspi_context = sspi_context; } */ + + Curl_pSecFn->DeleteSecurityContext(&sspi_context); + Curl_pSecFn->FreeCredentialsHandle(&cred_handle); return CURLE_OK; error: (void)curlx_nonblock(sock, TRUE); 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); + Curl_pSecFn->FreeCredentialsHandle(&cred_handle); + free(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); + free(sspi_w_token[0].pvBuffer); + free(sspi_w_token[1].pvBuffer); + free(sspi_w_token[2].pvBuffer); + free(etbuf); return result; } #endif