threads / bug / 18802

bug: git-http-push fails to validate email address in LOCK request

Subject: bug: git-http-push fails to validate email address in LOCK request

## tl;dr

4 messages between Apr 9, 2009 and Apr 9, 2009.

replies: 3people: 2as markdown or json

Riku Voipio· Apr 9, 2009, 11:19 UTC · lore

Hi,

If for whatever case EMAIL env variable or gitconfig email= setting contains any special xml character <>%&, the lock request in push will fail, with a completly useless error message:

git push foo upstream-push:master
Error: cannot lock existing info/refs
error: failed to push some refs to 'https://git.foo.org/projects/foo'

using the undocumented GIT_CURL_VERBOSE=1 env variable:

-snip-

> LOCK /projects/foo/info/refs HTTP/1.1

Authorization: Basic nothisstringisnottheonefromthereallog
User-Agent: git/1.6.2.1
Host: git.foo.org
Accept: */*
Timeout: Second-600
Content-Type: text/xml
Content-Length: 225
Expect: 100-continue

< HTTP/1.1 100 Continue
* The requested URL returned error: 400
* Closing connection #0
Error: cannot lock existing info/refs
error: failed to push some refs to 'https://git.foo.org/projects/foo'
-snip-

Helpfully, the actual lock request is omitted.

After finding someone from git hoster to grep the logs:

[Tue Apr 07 14:13:24 2009] [error] [client 1.1.1.1] XML Parser
Error: XML parser error code: not well-formed (invalid token) (4)

from http-push.c, we see that the lock request is:

#define LOCK_REQUEST "<?xml version=\"1.0\" encoding=\"utf-8\" ?>\n<D:lockinfo xmlns:D=\"DAV:\">\n<D:lockscope><D:exclusive/></D:lockscope>\n<D:locktype><D:write/></D:locktype>\n<D:owner>\n<D:href>mailto:%s</D:href>\n</D:owner>\n</D:lockinfo>"

So there is exactly one variable being set. Turns out the email address in .gitconfig was set to '<riku.voipio@iki.fi>'. Yes, a user error. This was copied from $EMAIL env variable, where using strings such as 'foo bar <foo.bar@corp.com>' have been a norm forever.

-- 
"rm -rf" only sounds scary if you don't have backups

Mike Hommey· Apr 9, 2009, 12:02 UTC · re: Riku Voipio · lore

Re: bug: git-http-push fails to validate email address in LOCK request

On Thu, Apr 09, 2009 at 02:19:44PM +0300, Riku Voipio <riku.voipio@iki.fi> wrote:

Show 9 quoted lines

> from http-push.c, we see that the lock request is:
> 
> #define LOCK_REQUEST "<?xml version=\"1.0\" encoding=\"utf-8\" ?>\n<D:lockinfo xmlns:D=\"DAV:\">\n<D:lockscope><D:exclusive/></D:lockscope>\n<D:locktype><D:write/></D:locktype>\n<D:owner>\n<D:href>mailto:%s</D:href>\n</D:owner>\n</D:lockinfo>"
> 
> So there is exactly one variable being set. Turns out the email
> address in .gitconfig was set to '<riku.voipio@iki.fi>'. Yes,
> a user error. This was copied from $EMAIL env variable, where
> using strings such as 'foo bar <foo.bar@corp.com>' have been
> a norm forever.

That's the problem with generating XML by concatenating strings without checks :(

Mike

Mike Hommey· Apr 9, 2009, 22:23 UTC · re: Riku Voipio · lore

[PATCH] Replace ",<,>,& with their respective XML entities in DAV requests

If the repo url or the user email contain XML special characters, the remote DAV server is likely to reject the LOCK requests because the XML is then malformed.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 http-push.c |   36 ++++++++++++++++++++++++++++++++++--
 1 files changed, 34 insertions(+), 2 deletions(-)

Note I haven't tested more than with a quick run of t5540-http-push.sh. Also note that this doesn't solve possible problems with non UTF-8 characters in both strings we escape here. (a DAV server may rightfully reject XML containing non UTF-8 characters, since this XML is supposed to be UTF-8 according to its prolog, and would thus be malformed)

diff --git a/http-push.c b/http-push.c
index feeb340..29e6b6b 100644
--- a/http-push.c
+++ b/http-push.c
@@ -186,6 +186,32 @@ enum dav_header_flag {
 	DAV_HEADER_TIMEOUT = (1u << 2)
 };
 
+static char *xml_entities(char *s)
+{
+	struct strbuf buf = STRBUF_INIT;
+	while (*s) {
+		size_t len = strcspn(s, "\"<>&");
+		strbuf_add(&buf, s, len);
+		s += len;
+		switch (*s) {
+		case '"':
+			strbuf_addstr(&buf, "&quot;");
+			break;
+		case '<':
+			strbuf_addstr(&buf, "&lt;");
+			break;
+		case '>':
+			strbuf_addstr(&buf, "&gt;");
+			break;
+		case '&':
+			strbuf_addstr(&buf, "&amp;");
+			break;
+		}
+		s++;
+	}
+	return buf.buf;
+}
+
 static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -1225,6 +1251,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	struct remote_lock *lock = NULL;
 	struct curl_slist *dav_headers = NULL;
 	struct xml_ctx ctx;
+	char *escaped;
 
 	url = xmalloc(strlen(repo->url) + strlen(path) + 1);
 	sprintf(url, "%s%s", repo->url, path);
@@ -1259,7 +1286,9 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 		ep = strchr(ep + 1, '/');
 	}
 
-	strbuf_addf(&out_buffer.buf, LOCK_REQUEST, git_default_email);
+	escaped = xml_entities(git_default_email);
+	strbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped);
+	free(escaped);
 
 	sprintf(timeout_header, "Timeout: Second-%ld", timeout);
 	dav_headers = curl_slist_append(dav_headers, timeout_header);
@@ -1584,8 +1613,11 @@ static int locking_available(void)
 	struct curl_slist *dav_headers = NULL;
 	struct xml_ctx ctx;
 	int lock_flags = 0;
+	char *escaped;
 
-	strbuf_addf(&out_buffer.buf, PROPFIND_SUPPORTEDLOCK_REQUEST, repo->url);
+	escaped = xml_entities(repo->url);
+	strbuf_addf(&out_buffer.buf, PROPFIND_SUPPORTEDLOCK_REQUEST, escaped);
+	free(escaped);
 
 	dav_headers = curl_slist_append(dav_headers, "Depth: 0");
 	dav_headers = curl_slist_append(dav_headers, "Content-Type: text/xml");

-- 
1.6.2.1

Mike Hommey· Apr 9, 2009, 22:25 UTC · re: Riku Voipio · lore

[PATCH v2] Replace ",<,>,& with their respective XML entities in DAV requests

If the repo url or the user email contain XML special characters, the remote DAV server is likely to reject the LOCK requests because the XML is then malformed.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 http-push.c |   36 ++++++++++++++++++++++++++++++++++--
 1 files changed, 34 insertions(+), 2 deletions(-)

Gah, the previous one didn't have a fix squashed in...

diff --git a/http-push.c b/http-push.c
index feeb340..5138224 100644
--- a/http-push.c
+++ b/http-push.c
@@ -186,6 +186,32 @@ enum dav_header_flag {
 	DAV_HEADER_TIMEOUT = (1u << 2)
 };
 
+static char *xml_entities(char *s)
+{
+	struct strbuf buf = STRBUF_INIT;
+	while (*s) {
+		size_t len = strcspn(s, "\"<>&");
+		strbuf_add(&buf, s, len);
+		s += len;
+		switch (*s) {
+		case '"':
+			strbuf_addstr(&buf, "&quot;");
+			break;
+		case '<':
+			strbuf_addstr(&buf, "&lt;");
+			break;
+		case '>':
+			strbuf_addstr(&buf, "&gt;");
+			break;
+		case '&':
+			strbuf_addstr(&buf, "&amp;");
+			break;
+		}
+		s++;
+	}
+	return strbuf_detach(&buf, NULL);
+}
+
 static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -1225,6 +1251,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	struct remote_lock *lock = NULL;
 	struct curl_slist *dav_headers = NULL;
 	struct xml_ctx ctx;
+	char *escaped;
 
 	url = xmalloc(strlen(repo->url) + strlen(path) + 1);
 	sprintf(url, "%s%s", repo->url, path);
@@ -1259,7 +1286,9 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 		ep = strchr(ep + 1, '/');
 	}
 
-	strbuf_addf(&out_buffer.buf, LOCK_REQUEST, git_default_email);
+	escaped = xml_entities(git_default_email);
+	strbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped);
+	free(escaped);
 
 	sprintf(timeout_header, "Timeout: Second-%ld", timeout);
 	dav_headers = curl_slist_append(dav_headers, timeout_header);
@@ -1584,8 +1613,11 @@ static int locking_available(void)
 	struct curl_slist *dav_headers = NULL;
 	struct xml_ctx ctx;
 	int lock_flags = 0;
+	char *escaped;
 
-	strbuf_addf(&out_buffer.buf, PROPFIND_SUPPORTEDLOCK_REQUEST, repo->url);
+	escaped = xml_entities(repo->url);
+	strbuf_addf(&out_buffer.buf, PROPFIND_SUPPORTEDLOCK_REQUEST, escaped);
+	free(escaped);
 
 	dav_headers = curl_slist_append(dav_headers, "Depth: 0");
 	dav_headers = curl_slist_append(dav_headers, "Content-Type: text/xml");

-- 
1.6.2.1