From a07e0d6b5ca4064e8c80625fd64c96807d6d01b6 Mon Sep 17 00:00:00 2001 From: Sitaram Chamarty Date: Sat, 1 Oct 2011 07:32:29 +0530 Subject: [PATCH] tighten up ref/file names (warning: possible backward compat breakage) The backward compat breakage is for people who already have all kinds of arbitrary characters in filenames *and* use `NAME/` rules. See the doc change in this commit for details and mitigation. See this link for background: http://groups.google.com/group/gitolite/browse_thread/thread/8dc5242052b16d0f Thanks to Dan Carpenter for the audit. --- conf/example.gitolite.rc | 1 + doc/gitolite.rc.mkd | 15 ++++++ src/gitolite.pm | 4 ++ src/gitolite_rc.pm | 4 +- t/t69-bad-ref-file-names | 101 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 t/t69-bad-ref-file-names diff --git a/conf/example.gitolite.rc b/conf/example.gitolite.rc index dbde473..6dadb70 100644 --- a/conf/example.gitolite.rc +++ b/conf/example.gitolite.rc @@ -56,6 +56,7 @@ $SVNSERVE = ""; # $GL_ADC_PATH = ""; # $GL_GET_MEMBERSHIPS_PGM = "/usr/local/bin/expand-ldap-user-to-groups" # $GL_HTTP_ANON_USER = "mob"; +# $GL_REF_OR_FILENAME_PATT=qr(^[0-9a-zA-Z][0-9a-zA-Z._\@/+ :,-]*$); # ------------------------------------------------------------------------------ # less used/changed variables diff --git a/doc/gitolite.rc.mkd b/doc/gitolite.rc.mkd index fb90d53..de6b96f 100644 --- a/doc/gitolite.rc.mkd +++ b/doc/gitolite.rc.mkd @@ -260,6 +260,21 @@ on feedback from my users to find or fix issues. gitolite that unauthenticated HTTP users are actually authenticated as this user. + * `$GL_REF_OR_FILENAME_PATT`, string + + Set of allowed characters in refnames (and, if you have `NAME/` rules, in + filenames as well). The default pattern is almost the same as + `$REPONAME_PATT` with some additions. + + Although the current code is not at risk in any way even if we let in + names containing strings like `$(command)`, and although I intend to make + sure things stay that way, it's probably a good idea to trap weird + filenames early. Just to be safe. + + You ought to be able to loosen the pattern by adding other characters to + it, if you really need to. If you do, at least avoid backquotes and the + dollar sign! + ### less used/changed variables diff --git a/src/gitolite.pm b/src/gitolite.pm index 85ee660..e39f310 100644 --- a/src/gitolite.pm +++ b/src/gitolite.pm @@ -213,6 +213,10 @@ sub check_ref { # NOTE: the function DIES when access is denied, unless arg 5 is true my ($allowed_refs, $repo, $ref, $perm, $dry_run) = @_; + + # sanity check the ref + die "invalid characters in ref or filename: $ref\n" unless $ref =~ $GL_REF_OR_FILENAME_PATT; + my @allowed_refs = sort { $a->[0] <=> $b->[0] } @{$allowed_refs}; for my $ar (@allowed_refs) { my $refex = $ar->[1]; diff --git a/src/gitolite_rc.pm b/src/gitolite_rc.pm index 01bcba2..9a31221 100644 --- a/src/gitolite_rc.pm +++ b/src/gitolite_rc.pm @@ -9,7 +9,7 @@ use Exporter 'import'; @EXPORT = qw( $ABRT $WARN $R_COMMANDS $W_COMMANDS - $REPONAME_PATT $USERNAME_PATT $REPOPATT_PATT + $REPONAME_PATT $USERNAME_PATT $REPOPATT_PATT $GL_REF_OR_FILENAME_PATT $ADC_CMD_ARGS_PATT $BIG_INFO_CAP $current_data_version @@ -48,6 +48,8 @@ $REPONAME_PATT=qr(^\@?[0-9a-zA-Z][0-9a-zA-Z._\@/+-]*$); $USERNAME_PATT=qr(^\@?[0-9a-zA-Z][0-9a-zA-Z._\@+-]*$); # same as REPONAME, but used for wildcard repos, allows some common regex metas $REPOPATT_PATT=qr(^\@?[0-9a-zA-Z[][\\^.$|()[\]*+?{}0-9a-zA-Z._\@/,-]*$); +# pattern for refnames pushed or names of files changed +$GL_REF_OR_FILENAME_PATT=qr(^[0-9a-zA-Z][0-9a-zA-Z._\@/+ :,-]*$); # ADC commands and arguments must match this pattern $ADC_CMD_ARGS_PATT=qr(^[0-9a-zA-Z._\@/+:-]*$); diff --git a/t/t69-bad-ref-file-names b/t/t69-bad-ref-file-names new file mode 100644 index 0000000..96732e0 --- /dev/null +++ b/t/t69-bad-ref-file-names @@ -0,0 +1,101 @@ +cd $TESTDIR +$TESTDIR/rollback || die "rollback failed" +# ---------- + +# vim: syn=sh: +# ---------- +name "setup" +echo " + repo aa + RW+ = @all +" | ugc + +# reasonably complex setup; we'll do everything from one repo though +cd ~/td +rm -rf aa +runlocal git clone u1:aa +cd ~/td/aa +mdc file-1 +name "INTERNAL" +runlocal git push origin HEAD +expect "To u1:aa" +expect_push_ok "\* \[new branch\] HEAD -> master" + +name "push file aa,bb ok" +mdc aa,bb +runlocal git push origin HEAD +expect "To u1:aa" +expect_push_ok "HEAD -> master" + +name "push file aa=bb ok" +mdc aa=bb +runlocal git push origin HEAD +expect "To u1:aa" +expect_push_ok "HEAD -> master" + +name "push to branch dd,ee ok" +runlocal git push origin master:dd,ee +expect "To u1:aa" +expect_push_ok "\* \[new branch\] master -> dd,ee" + +name "push to branch dd=ee fail" +runlocal git push origin master:dd=ee +expect "remote: invalid characters in ref or filename: refs/heads/dd=ee" +expect "remote: error: hook declined to update refs/heads/dd=ee" +expect "\[remote rejected\] master -> dd=ee (hook declined)" +expect "error: failed to push some refs to 'u1:aa'" + +name "INTERNAL" + +cd $TESTDIR +$TESTDIR/rollback || die "rollback failed" +# ---------- + +name "setup" +echo " + repo aa + RW+ = @all + RW+ NAME/ = @all +" | ugc -r + +# reasonably complex setup; we'll do everything from one repo though +cd ~/td +rm -rf aa +runlocal git clone u1:aa +cd ~/td/aa +mdc file-1 +name "INTERNAL" +runlocal git push origin HEAD +expect "To u1:aa" +expect_push_ok "\* \[new branch\] HEAD -> master" + +name "push file aa,bb ok" +mdc aa,bb +runlocal git push origin HEAD +expect "To u1:aa" +expect_push_ok "HEAD -> master" + +name "push file aa=bb fail" +mdc aa=bb +runlocal git push origin HEAD +expect "To u1:aa" +expect "remote: invalid characters in ref or filename: NAME/aa=bb" +expect "remote: error: hook declined to update refs/heads/master" +expect "\[remote rejected\] HEAD -> master (hook declined)" +expect "error: failed to push some refs to 'u1:aa'" + +name "push to branch dd,ee ok" +runlocal git reset --hard HEAD^ +mdc some-file +runlocal git push origin master:dd,ee +expect "To u1:aa" +expect_push_ok "\* \[new branch\] master -> dd,ee" + +name "push to branch dd=ee fail" +runlocal git push origin master:dd=ee +expect "remote: invalid characters in ref or filename: refs/heads/dd=ee" +expect "remote: error: hook declined to update refs/heads/dd=ee" +expect "\[remote rejected\] master -> dd=ee (hook declined)" +expect "error: failed to push some refs to 'u1:aa'" + +name "INTERNAL"