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 c106fec0c8fb1df92857f49e00a6c021b5dcf46e..3ff3b723f9a2060258b176ee225666d1352f4f67 100644 (file)
--- 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