Skip to content
Permalink
Browse files Browse the repository at this point in the history
(security) fix bug in pattern to detect path traversal
while we're about it, add the same check to some of the internal
routines, so that commands can also be protected.

finally, just to make sure we don't lose it again in some other fashion,
add a few tests for path traversal...
  • Loading branch information
sitaramc committed Oct 5, 2012
1 parent 0d371ac commit f636ce3
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 3 deletions.
2 changes: 1 addition & 1 deletion src/gitolite-shell
Expand Up @@ -168,7 +168,7 @@ sub sanity {
my $repo = shift;
_die "'$repo' contains bad characters" if $repo !~ $REPONAME_PATT;
_die "'$repo' ends with a '/'" if $repo =~ m(/$);
_die "'$repo' contains '..'" if $repo =~ m(\.\.$);
_die "'$repo' contains '..'" if $repo =~ m(\.\.);
}
# ----------------------------------------------------------------------
Expand Down
15 changes: 14 additions & 1 deletion src/lib/Gitolite/Conf/Load.pm
Expand Up @@ -67,8 +67,9 @@ my $last_repo = '';

sub access {
my ( $repo, $user, $aa, $ref ) = @_;
_die "invalid repo '$repo'" if not( $repo and $repo =~ $REPOPATT_PATT );
_die "invalid user '$user'" if not( $user and $user =~ $USERNAME_PATT );
sanity($repo);

my $deny_rules = option( $repo, 'deny-rules' );
load($repo);

Expand Down Expand Up @@ -175,8 +176,18 @@ sub option {
return $ret->{$option};
}

sub sanity {
my $repo = shift;

_die "invalid repo '$repo'" if not( $repo and $repo =~ $REPOPATT_PATT );
_die "'$repo' ends with a '/'" if $repo =~ m(/$);
_die "'$repo' contains '..'" if $repo =~ $REPONAME_PATT and $repo =~ m(\.\.);
}
sub repo_missing {
my $repo = shift;
sanity($repo);
return not -d "$rc{GL_REPO_BASE}/$repo.git";
}
Expand Down Expand Up @@ -400,6 +411,8 @@ sub generic_name {
sub creator {
my $repo = shift;
sanity($repo);
return ( $ENV{GL_USER} || '' ) if repo_missing($repo);
my $f = "$rc{GL_REPO_BASE}/$repo.git/gl-creator";
my $creator = '';
Expand Down
19 changes: 18 additions & 1 deletion t/0-me-first.t
Expand Up @@ -6,10 +6,12 @@ use warnings;
use lib "src/lib";
use Gitolite::Test;

my $rb = `gitolite query-rc -n GL_REPO_BASE`;

# initial smoke tests
# ----------------------------------------------------------------------

try "plan 65";
try "plan 73";

# basic push admin repo
confreset;confadd '
Expand Down Expand Up @@ -75,4 +77,19 @@ try "
glt ls-remote u5 file:///cc/1; ok; perl s/TRACE.*//g; !/\\S/
glt ls-remote u5 file:///cc/2; !ok; /DENIED by fallthru/
glt ls-remote u6 file:///cc/2; !ok; /DENIED by fallthru/
# command
glt perms u4 -c cc/bar/baz/frob + READERS u2;
ok; /Initialized empty .*cc/bar/baz/frob.git/
# path traversal
glt ls-remote u4 file:///cc/dd/../ee
!ok; /FATAL: 'cc/dd/\\.\\./ee' contains '\\.\\.'/
glt ls-remote u5 file:///cc/../../../../../..$rb/gitolite-admin
!ok; /FATAL: 'cc/../../../../../..$rb/gitolite-admin' contains '\\.\\.'/
glt perms u4 -c cc/bar/baz/../frob + READERS u2
!ok; /FATAL: 'cc/bar/baz/\\.\\./frob' contains '\\.\\.'/
";

0 comments on commit f636ce3

Please sign in to comment.