[9front] [patch] git/pull: Be more careful about remote refs.

[9front] [patch] git/pull: Be more careful about remote refs.

[James Cook]

(This patch needs testing, and maybe more thought: maybe git/get.c
should be changed instead of git/pull. I haven't taken the time to
understand exactly what git/get does. But at minimum I hope this
is a useful bug report.)

I found 1-2 bugs in git/pull:

1. If the remote $origin has refs under refs/remotes/$origin,
these should be ignored, but instead, git/pull saves them
to .git/refs/remotes/$origin, possibly overwriting what it
found under refs/heads in the remote.

As a concrete example, the main branch for https://github.com/Perl/perl5.git
is called blead. If I run git/pull in a local checkout, I end up
with an outdated blead because for some reason the github repo has
its own refs/remotes/origin/blead, and git/get prints it after
refs/heads/blead.

2. If the remote path has special characters like '}', the remote
server could probably trick git/pull into executing arbitrary rc
commands. I haven't actually verified this is possible, but git/pull
is pasting the output of git/get unescaped into an rc command, and
I couldn't see any safety checking for that value in git/get.c.

The below patch fixes 1 by restricting to refs/(heads|tags)/ instead
of examining every remote ref, and fixes 2 by using an environment
variable instead of dynamically constructing an rc command. (I tried
writing to awk's special ENVIRON variable, but it didn't seem to
do anything, so I used /env.)

-- 
James



--- a/sys/src/cmd/git/pull
+++ b/sys/src/cmd/git/pull
@@ -10,7 +10,7 @@
 	if(! ~ $#debug 0)
 		dflag='-d'
 	{git/get $dflag -u $upstream $url >[2=3] || die $status} | awk '
-	/^remote/{
+	/^remote refs\/(heads|tags)\//{
 		if($2=="HEAD")
 			next
 		ref=$2
@@ -17,7 +17,9 @@
 		hash=$3
 		gsub("^refs/heads", "refs/remotes/'$upstream'", ref)
 		outfile = ".git/"ref
-		system("mkdir -p `{basename -d "outfile"}");
+		print outfile > "/env/outfile"
+		close("/env/outfile")
+		system("mkdir -p `{basename -d $outfile}");
 		print hash > outfile;
 		close(outfile);
 	}

Re: [9front] [patch] git/pull: Be more careful about remote refs.

[ori]

I think this makes sense; there's one small change I'd make:

	system("mkdir -p `{basename -d $outfile}");

should probably not use `{} directly, but quote it:

	system("mkdir -p `''''{basename -d $outfile}")

untested, of course: if '$outfile' is terminated with a
newline, that'd need to be stripped.

I think it may also make sense to sanitize the paths in
git/get and reject printing anything with frogs.

The split of work is that git/get is responsible for
sycing the git objects, and saying what branches were
touched, and git/pull is responsible for interpreting
that and updating the branches.

So, I think you're right in this change, mostly.

Thanks!

Quoth James Cook <falsifian@falsifian.org>:
> (This patch needs testing, and maybe more thought: maybe git/get.c
> should be changed instead of git/pull. I haven't taken the time to
> understand exactly what git/get does. But at minimum I hope this
> is a useful bug report.)
> 
> I found 1-2 bugs in git/pull:
> 
> 1. If the remote $origin has refs under refs/remotes/$origin,
> these should be ignored, but instead, git/pull saves them
> to .git/refs/remotes/$origin, possibly overwriting what it
> found under refs/heads in the remote.
> 
> As a concrete example, the main branch for https://github.com/Perl/perl5.git
> is called blead. If I run git/pull in a local checkout, I end up
> with an outdated blead because for some reason the github repo has
> its own refs/remotes/origin/blead, and git/get prints it after
> refs/heads/blead.
> 
> 2. If the remote path has special characters like '}', the remote
> server could probably trick git/pull into executing arbitrary rc
> commands. I haven't actually verified this is possible, but git/pull
> is pasting the output of git/get unescaped into an rc command, and
> I couldn't see any safety checking for that value in git/get.c.
> 
> The below patch fixes 1 by restricting to refs/(heads|tags)/ instead
> of examining every remote ref, and fixes 2 by using an environment
> variable instead of dynamically constructing an rc command. (I tried
> writing to awk's special ENVIRON variable, but it didn't seem to
> do anything, so I used /env.)
> 
> -- 
> James
> 
> 
> 
> --- a/sys/src/cmd/git/pull
> +++ b/sys/src/cmd/git/pull
> @@ -10,7 +10,7 @@
>  	if(! ~ $#debug 0)
>  		dflag='-d'
>  	{git/get $dflag -u $upstream $url >[2=3] || die $status} | awk '
> -	/^remote/{
> +	/^remote refs\/(heads|tags)\//{
>  		if($2=="HEAD")
>  			next
>  		ref=$2
> @@ -17,7 +17,9 @@
>  		hash=$3
>  		gsub("^refs/heads", "refs/remotes/'$upstream'", ref)
>  		outfile = ".git/"ref
> -		system("mkdir -p `{basename -d "outfile"}");
> +		print outfile > "/env/outfile"
> +		close("/env/outfile")
> +		system("mkdir -p `{basename -d $outfile}");
>  		print hash > outfile;
>  		close(outfile);
>  	}

Re: [9front] [patch] git/pull: Be more careful about remote refs.

[ori]

Quoth James Cook <falsifian@falsifian.org>:

> --- a/sys/src/cmd/git/pull
> +++ b/sys/src/cmd/git/pull
> @@ -10,7 +10,7 @@
>  	if(! ~ $#debug 0)
>  		dflag='-d'
>  	{git/get $dflag -u $upstream $url >[2=3] || die $status} | awk '
> -	/^remote/{
> +	/^remote refs\/(heads|tags)\//{
>  		if($2=="HEAD")
>  			next

also, this is dead code, if the pattern above is chagned

>  		ref=$2
> @@ -17,7 +17,9 @@
>  		hash=$3
>  		gsub("^refs/heads", "refs/remotes/'$upstream'", ref)
>  		outfile = ".git/"ref
> -		system("mkdir -p `{basename -d "outfile"}");
> +		print outfile > "/env/outfile"
> +		close("/env/outfile")
> +		system("mkdir -p `{basename -d $outfile}");
>  		print hash > outfile;
>  		close(outfile);
>  	}

Re: [9front] [patch] git/pull: Be more careful about remote refs.

[ori]

Quoth James Cook <falsifian@falsifian.org>:
> (This patch needs testing, and maybe more thought: maybe git/get.c
> should be changed instead of git/pull. I haven't taken the time to
> understand exactly what git/get does. But at minimum I hope this
> is a useful bug report.)
> 
> I found 1-2 bugs in git/pull:
> 
> 1. If the remote $origin has refs under refs/remotes/$origin,
> these should be ignored, but instead, git/pull saves them
> to .git/refs/remotes/$origin, possibly overwriting what it
> found under refs/heads in the remote.
> 
> As a concrete example, the main branch for https://github.com/Perl/perl5.git
> is called blead. If I run git/pull in a local checkout, I end up
> with an outdated blead because for some reason the github repo has
> its own refs/remotes/origin/blead, and git/get prints it after
> refs/heads/blead.
> 
> 2. If the remote path has special characters like '}', the remote
> server could probably trick git/pull into executing arbitrary rc
> commands. I haven't actually verified this is possible, but git/pull
> is pasting the output of git/get unescaped into an rc command, and
> I couldn't see any safety checking for that value in git/get.c.
> 
> The below patch fixes 1 by restricting to refs/(heads|tags)/ instead
> of examining every remote ref, and fixes 2 by using an environment
> variable instead of dynamically constructing an rc command. (I tried
> writing to awk's special ENVIRON variable, but it didn't seem to
> do anything, so I used /env.)
> 
> -- 
> James
> 
> 
> 
> --- a/sys/src/cmd/git/pull
> +++ b/sys/src/cmd/git/pull
> @@ -10,7 +10,7 @@
>  	if(! ~ $#debug 0)
>  		dflag='-d'
>  	{git/get $dflag -u $upstream $url >[2=3] || die $status} | awk '
> -	/^remote/{
> +	/^remote refs\/(heads|tags)\//{
>  		if($2=="HEAD")
>  			next
>  		ref=$2
> @@ -17,7 +17,9 @@
>  		hash=$3
>  		gsub("^refs/heads", "refs/remotes/'$upstream'", ref)
>  		outfile = ".git/"ref
> -		system("mkdir -p `{basename -d "outfile"}");
> +		print outfile > "/env/outfile"
> +		close("/env/outfile")
> +		system("mkdir -p `{basename -d $outfile}");
>  		print hash > outfile;
>  		close(outfile);
>  	}

How's this?

diff 490ebc54bb9b46a5719f059da6aef5b8b381eecf uncommitted
--- a/sys/src/cmd/git/pull
+++ b/sys/src/cmd/git/pull
@@ -10,14 +10,14 @@
 	if(! ~ $#debug 0)
 		dflag='-d'
 	{git/get $dflag -u $upstream $url >[2=3] || die $status} | awk '
-	/^remote/{
-		if($2=="HEAD")
-			next
+	/^remote refs\/(heads|tags)\//{
 		ref=$2
 		hash=$3
 		gsub("^refs/heads", "refs/remotes/'$upstream'", ref)
 		outfile = ".git/"ref
-		system("mkdir -p `{basename -d "outfile"}");
+		print outfile > "/env/outfile"
+		system("mkdir -p `$nl{basename -d $outfile}");\
+		close("/env/outfile")
 		print hash > outfile;
 		close(outfile);
 	}

Re: [9front] [patch] git/pull: Be more careful about remote refs.

[James Cook]

ori@eigenstate.org wrote:
> How's this?
> 
> diff 490ebc54bb9b46a5719f059da6aef5b8b381eecf uncommitted
> --- a/sys/src/cmd/git/pull
> +++ b/sys/src/cmd/git/pull
> @@ -10,14 +10,14 @@
>  	if(! ~ $#debug 0)
>  		dflag='-d'
>  	{git/get $dflag -u $upstream $url >[2=3] || die $status} | awk '
> -	/^remote/{
> -		if($2=="HEAD")
> -			next
> +	/^remote refs\/(heads|tags)\//{
>  		ref=$2
>  		hash=$3
>  		gsub("^refs/heads", "refs/remotes/'$upstream'", ref)
>  		outfile = ".git/"ref
> -		system("mkdir -p `{basename -d "outfile"}");
> +		print outfile > "/env/outfile"
> +		system("mkdir -p `$nl{basename -d $outfile}");\
> +		close("/env/outfile")
>  		print hash > outfile;
>  		close(outfile);
>  	}

That looks good and is working for me so far.

Below is an almost-complete patch to add some validation to git/get
as discussed on IRC.

Do not commit as is because I couldn't figure out a proper way to
have the file cmd/git/test/okref.c depend on object files in cmd/git.
To run the tests with my hacky patch, first mk the .$O files in
cmd/git, then cd to cmd/git/test and mk okref.test.

Also, I've barely tested it; just tried pulling one of my private
git repos.

Also also I haven't looked carefully at the 9front coding style.
E.g. I don't see use of "const" in other code so maybe that should
go.

I implemented okref then discovered (thanks ori) that the actual
rules were different, and then implemented those as okrefname. Kept
okref as a separate function because I already had those tests, and
maybe it'll come in handy later.

-- 
James


--- a/sys/src/cmd/git/get.c
+++ b/sys/src/cmd/git/get.c
@@ -247,6 +247,8 @@
 		getfields(buf, sp, nelem(sp), 1, " \t\n\r");
 		if(strstr(sp[1], "^{}"))
 			continue;
+		if (!okrefname(sp[1]))
+			sysfatal("remote side sent invalid ref: %s", sp[1]);
 		if(fetchbranch && !branchmatch(sp[1], fetchbranch))
 			continue;
 		if(refsz == nref + 1){
--- a/sys/src/cmd/git/git.h
+++ b/sys/src/cmd/git/git.h
@@ -312,6 +312,8 @@
 void	*earealloc(void *, ulong, ulong);
 void	*erealloc(void *, ulong);
 char	*estrdup(char *);
+int okref(char *);
+int okrefname(char *);
 int	slurpdir(char *, Dir **);
 int	hparse(Hash *, char *);
 int	hassuffix(char *, char *);
--- a/sys/src/cmd/git/test/mkfile
+++ b/sys/src/cmd/git/test/mkfile
@@ -9,7 +9,17 @@
 	lca\
 	merge\
 	noam\
+	okref\
 	range
+
+OFILES=\
+	../delta.$O\
+	../objset.$O\
+	../ols.$O\
+	../pack.$O\
+	../proto.$O\
+	../util.$O\
+	../ref.$O
 
 </sys/src/cmd/mktest
 
--- a/sys/src/cmd/git/util.c
+++ b/sys/src/cmd/git/util.c
@@ -164,6 +164,57 @@
 	return s;
 }
 
+/* Checks the rules at https://git-scm.com/docs/git-check-ref-format */
+int
+okref(char *s)
+{
+	char prev = 0;
+	int haveslash = 0;
+	char *p;
+
+	if (*s == 0) return 0;
+	if ((p = strstr(s, ".lock")) != nil) {
+		if (*(p+5) == 0) return 0;  /* ends in .lock */
+	}
+
+	for (; *s != 0; prev = *s, ++s) {
+		switch (*s) {
+		case '.':
+			if (prev == 0 || prev == '/' || prev == '.') return 0;
+			break;
+		case '/':
+			if (prev == 0 || prev == '/') return 0;
+			haveslash = 1;
+			break;
+		case '{':
+			if (prev == '@') return 0;
+			break;
+		case ' ':
+		case '~':
+		case '^':
+		case ':':
+		case '?':
+		case '*':
+		case '[':
+		case '\\':
+			return 0;
+		default:
+			if (*s <= 0x20) return 0;
+		}
+	}
+	return haveslash && *(s-1) != '/' && *(s-1) != '.';
+}
+
+/* Checks the rules for a refname at
+https://git-scm.com/docs/protocol-common/2.9.5 */
+int
+okrefname(char *s)
+{
+	if (strcmp(s, "HEAD") == 0) return 1;
+	if (strstr(s, "refs/") != s) return 0;
+	return okref(s+5);
+}
+
 int
 Hfmt(Fmt *fmt)
 {