git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Rubén Justo" <rjusto@gmail.com>
Subject: [PATCH 08/11] http: use git_config_string_dup()
Date: Sat, 6 Apr 2024 21:03:54 -0400	[thread overview]
Message-ID: <20240407010354.GH868358@coredump.intra.peff.net> (raw)
In-Reply-To: <20240407005656.GA436890@coredump.intra.peff.net>

There are many calls to git_config_string() in the HTTP code, leading to
possible leaks if a config variable is seen multiple times. I split
these out from the other "easy" cases because the change to non-const
pointers has some follow-on effects:

  1. When these variables are passed to var_override(), that should now
     take a non-const pointer (and consequently can drop the cast it
     passes to free).

  2. That file also has the somewhat redundant set_from_env() helper,
     which did _not_ free the existing value, and so has basically the
     same leak as git_config_string(). Rather than adjust it to take a
     non-const pointer, we can just swap it out for var_override(),
     fixing the leak and reducing the number of helpers at the same
     time.

This makes for a fairly big patch, but we have to do it all at once to
keep the compiler happy (or alternatively, insert a bunch of temporary
casts into http.c).

Note that some of these are git_config_pathname_dup(); it seemed better
to convert them all together, since the root of the problem (and the
solution) are essentially the same.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 105 +++++++++++++++++++++++++++------------------------------
 1 file changed, 49 insertions(+), 56 deletions(-)

diff --git a/http.c b/http.c
index e73b136e58..f2bcc9083e 100644
--- a/http.c
+++ b/http.c
@@ -38,11 +38,11 @@ char curl_errorstr[CURL_ERROR_SIZE];
 
 static int curl_ssl_verify = -1;
 static int curl_ssl_try;
-static const char *curl_http_version = NULL;
-static const char *ssl_cert;
-static const char *ssl_cert_type;
-static const char *ssl_cipherlist;
-static const char *ssl_version;
+static char *curl_http_version;
+static char *ssl_cert;
+static char *ssl_cert_type;
+static char *ssl_cipherlist;
+static char *ssl_version;
 static struct {
 	const char *name;
 	long ssl_version;
@@ -59,23 +59,23 @@ static struct {
 	{ "tlsv1.3", CURL_SSLVERSION_TLSv1_3 },
 #endif
 };
-static const char *ssl_key;
-static const char *ssl_key_type;
-static const char *ssl_capath;
-static const char *curl_no_proxy;
+static char *ssl_key;
+static char *ssl_key_type;
+static char *ssl_capath;
+static char *curl_no_proxy;
 #ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY
-static const char *ssl_pinnedkey;
+static char *ssl_pinnedkey;
 #endif
-static const char *ssl_cainfo;
+static char *ssl_cainfo;
 static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
-static const char *curl_http_proxy;
-static const char *http_proxy_authmethod;
+static char *curl_http_proxy;
+static char *http_proxy_authmethod;
 
-static const char *http_proxy_ssl_cert;
-static const char *http_proxy_ssl_key;
-static const char *http_proxy_ssl_ca_info;
+static char *http_proxy_ssl_cert;
+static char *http_proxy_ssl_key;
+static char *http_proxy_ssl_ca_info;
 static struct credential proxy_cert_auth = CREDENTIAL_INIT;
 static int proxy_ssl_cert_password_required;
 
@@ -95,7 +95,7 @@ static struct {
 	 */
 };
 #ifdef CURLGSSAPI_DELEGATION_FLAG
-static const char *curl_deleg;
+static char *curl_deleg;
 static struct {
 	const char *name;
 	long curl_deleg_param;
@@ -108,11 +108,11 @@ static struct {
 
 static struct credential proxy_auth = CREDENTIAL_INIT;
 static const char *curl_proxyuserpwd;
-static const char *curl_cookie_file;
+static char *curl_cookie_file;
 static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
-static const char *user_agent;
+static char *user_agent;
 static int curl_empty_auth = -1;
 
 enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
@@ -366,28 +366,28 @@ static int http_options(const char *var, const char *value,
 			const struct config_context *ctx, void *data)
 {
 	if (!strcmp("http.version", var)) {
-		return git_config_string(&curl_http_version, var, value);
+		return git_config_string_dup(&curl_http_version, var, value);
 	}
 	if (!strcmp("http.sslverify", var)) {
 		curl_ssl_verify = git_config_bool(var, value);
 		return 0;
 	}
 	if (!strcmp("http.sslcipherlist", var))
-		return git_config_string(&ssl_cipherlist, var, value);
+		return git_config_string_dup(&ssl_cipherlist, var, value);
 	if (!strcmp("http.sslversion", var))
-		return git_config_string(&ssl_version, var, value);
+		return git_config_string_dup(&ssl_version, var, value);
 	if (!strcmp("http.sslcert", var))
-		return git_config_pathname(&ssl_cert, var, value);
+		return git_config_pathname_dup(&ssl_cert, var, value);
 	if (!strcmp("http.sslcerttype", var))
-		return git_config_string(&ssl_cert_type, var, value);
+		return git_config_string_dup(&ssl_cert_type, var, value);
 	if (!strcmp("http.sslkey", var))
-		return git_config_pathname(&ssl_key, var, value);
+		return git_config_pathname_dup(&ssl_key, var, value);
 	if (!strcmp("http.sslkeytype", var))
-		return git_config_string(&ssl_key_type, var, value);
+		return git_config_string_dup(&ssl_key_type, var, value);
 	if (!strcmp("http.sslcapath", var))
-		return git_config_pathname(&ssl_capath, var, value);
+		return git_config_pathname_dup(&ssl_capath, var, value);
 	if (!strcmp("http.sslcainfo", var))
-		return git_config_pathname(&ssl_cainfo, var, value);
+		return git_config_pathname_dup(&ssl_cainfo, var, value);
 	if (!strcmp("http.sslcertpasswordprotected", var)) {
 		ssl_cert_password_required = git_config_bool(var, value);
 		return 0;
@@ -436,27 +436,27 @@ static int http_options(const char *var, const char *value,
 		return 0;
 	}
 	if (!strcmp("http.proxy", var))
-		return git_config_string(&curl_http_proxy, var, value);
+		return git_config_string_dup(&curl_http_proxy, var, value);
 
 	if (!strcmp("http.proxyauthmethod", var))
-		return git_config_string(&http_proxy_authmethod, var, value);
+		return git_config_string_dup(&http_proxy_authmethod, var, value);
 
 	if (!strcmp("http.proxysslcert", var))
-		return git_config_string(&http_proxy_ssl_cert, var, value);
+		return git_config_string_dup(&http_proxy_ssl_cert, var, value);
 
 	if (!strcmp("http.proxysslkey", var))
-		return git_config_string(&http_proxy_ssl_key, var, value);
+		return git_config_string_dup(&http_proxy_ssl_key, var, value);
 
 	if (!strcmp("http.proxysslcainfo", var))
-		return git_config_string(&http_proxy_ssl_ca_info, var, value);
+		return git_config_string_dup(&http_proxy_ssl_ca_info, var, value);
 
 	if (!strcmp("http.proxysslcertpasswordprotected", var)) {
 		proxy_ssl_cert_password_required = git_config_bool(var, value);
 		return 0;
 	}
 
 	if (!strcmp("http.cookiefile", var))
-		return git_config_pathname(&curl_cookie_file, var, value);
+		return git_config_pathname_dup(&curl_cookie_file, var, value);
 	if (!strcmp("http.savecookies", var)) {
 		curl_save_cookies = git_config_bool(var, value);
 		return 0;
@@ -472,7 +472,7 @@ static int http_options(const char *var, const char *value,
 	}
 
 	if (!strcmp("http.useragent", var))
-		return git_config_string(&user_agent, var, value);
+		return git_config_string_dup(&user_agent, var, value);
 
 	if (!strcmp("http.emptyauth", var)) {
 		if (value && !strcmp("auto", value))
@@ -484,7 +484,7 @@ static int http_options(const char *var, const char *value,
 
 	if (!strcmp("http.delegation", var)) {
 #ifdef CURLGSSAPI_DELEGATION_FLAG
-		return git_config_string(&curl_deleg, var, value);
+		return git_config_string_dup(&curl_deleg, var, value);
 #else
 		warning(_("Delegation control is not supported with cURL < 7.22.0"));
 		return 0;
@@ -493,7 +493,7 @@ static int http_options(const char *var, const char *value,
 
 	if (!strcmp("http.pinnedpubkey", var)) {
 #ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY
-		return git_config_pathname(&ssl_pinnedkey, var, value);
+		return git_config_pathname_dup(&ssl_pinnedkey, var, value);
 #else
 		warning(_("Public key pinning not supported with cURL < 7.39.0"));
 		return 0;
@@ -572,10 +572,10 @@ static void init_curl_http_auth(CURL *result)
 }
 
 /* *var must be free-able */
-static void var_override(const char **var, char *value)
+static void var_override(char **var, char *value)
 {
 	if (value) {
-		free((void *)*var);
+		free(*var);
 		*var = xstrdup(value);
 	}
 }
@@ -1208,13 +1208,6 @@ static CURL *get_curl_handle(void)
 	return result;
 }
 
-static void set_from_env(const char **var, const char *envname)
-{
-	const char *val = getenv(envname);
-	if (val)
-		*var = val;
-}
-
 void http_init(struct remote *remote, const char *url, int proactive_auth)
 {
 	char *low_speed_limit;
@@ -1291,14 +1284,14 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	if (getenv("GIT_SSL_NO_VERIFY"))
 		curl_ssl_verify = 0;
 
-	set_from_env(&ssl_cert, "GIT_SSL_CERT");
-	set_from_env(&ssl_cert_type, "GIT_SSL_CERT_TYPE");
-	set_from_env(&ssl_key, "GIT_SSL_KEY");
-	set_from_env(&ssl_key_type, "GIT_SSL_KEY_TYPE");
-	set_from_env(&ssl_capath, "GIT_SSL_CAPATH");
-	set_from_env(&ssl_cainfo, "GIT_SSL_CAINFO");
+	var_override(&ssl_cert, getenv("GIT_SSL_CERT"));
+	var_override(&ssl_cert_type, getenv("GIT_SSL_CERT_TYPE"));
+	var_override(&ssl_key, getenv("GIT_SSL_KEY"));
+	var_override(&ssl_key_type, getenv("GIT_SSL_KEY_TYPE"));
+	var_override(&ssl_capath, getenv("GIT_SSL_CAPATH"));
+	var_override(&ssl_cainfo, getenv("GIT_SSL_CAINFO"));
 
-	set_from_env(&user_agent, "GIT_HTTP_USER_AGENT");
+	var_override(&user_agent, getenv("GIT_HTTP_USER_AGENT"));
 
 	low_speed_limit = getenv("GIT_HTTP_LOW_SPEED_LIMIT");
 	if (low_speed_limit)
@@ -1314,9 +1307,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	if (max_requests < 1)
 		max_requests = DEFAULT_MAX_REQUESTS;
 
-	set_from_env(&http_proxy_ssl_cert, "GIT_PROXY_SSL_CERT");
-	set_from_env(&http_proxy_ssl_key, "GIT_PROXY_SSL_KEY");
-	set_from_env(&http_proxy_ssl_ca_info, "GIT_PROXY_SSL_CAINFO");
+	var_override(&http_proxy_ssl_cert, getenv("GIT_PROXY_SSL_CERT"));
+	var_override(&http_proxy_ssl_key, getenv("GIT_PROXY_SSL_KEY"));
+	var_override(&http_proxy_ssl_ca_info, getenv("GIT_PROXY_SSL_CAINFO"));
 
 	if (getenv("GIT_PROXY_SSL_CERT_PASSWORD_PROTECTED"))
 		proxy_ssl_cert_password_required = 1;
-- 
2.44.0.872.g288abe5b5b



  parent reply	other threads:[~2024-04-07  1:04 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-06 18:11 [PATCH] config: do not leak excludes_file Junio C Hamano
2024-04-06 19:17 ` [WIP] git_config_pathname() leakfix Junio C Hamano
2024-04-07  0:56 ` [PATCH 0/12] git_config_string() considered harmful Jeff King
2024-04-07  0:58   ` [PATCH 01/11] config: make sequencer.c's git_config_string_dup() public Jeff King
2024-04-07  1:00   ` [PATCH 02/11] config: add git_config_pathname_dup() Jeff King
2024-04-07  1:00   ` [PATCH 03/11] config: prefer git_config_string_dup() for temp variables Jeff King
2024-04-07  1:50     ` Eric Sunshine
2024-04-07  2:45       ` Jeff King
2024-04-07  1:01   ` [PATCH 04/11] config: use git_config_string_dup() for open-coded equivalents Jeff King
2024-04-07  1:01   ` [PATCH 05/11] config: use git_config_string_dup() to fix leaky open coding Jeff King
2024-04-07  1:02   ` [PATCH 06/11] config: use git_config_string() in easy cases Jeff King
2024-04-07  2:05     ` Eric Sunshine
2024-04-07  2:47       ` Jeff King
2024-04-07  1:03   ` [PATCH 07/11] config: use git_config_pathname_dup() " Jeff King
2024-04-07  1:03   ` Jeff King [this message]
2024-04-07  1:04   ` [PATCH 09/11] merge: use git_config_string_dup() for pull strategies Jeff King
2024-04-07  1:04   ` [PATCH 10/11] userdiff: use git_config_string_dup() when we can Jeff King
2024-04-07  1:07   ` [PATCH 11/11] blame: use "dup" string_list for ignore-revs files Jeff King
2024-04-07  2:42     ` Eric Sunshine
2024-04-07  1:08   ` [PATCH 0/12] git_config_string() considered harmful Jeff King
2024-04-07 17:58   ` Rubén Justo
2024-04-08 20:55     ` Jeff King
2024-04-11 23:36       ` Rubén Justo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240407010354.GH868358@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=rjusto@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).