security audit of Gitolite
Dan Carpenter
Summary: Gitolite is a very good program. The code is clean. The
documentation is excelent. It was easy to install. It was written
with a strong focus on security.
Attack Surface: Users call two perl scripts "gl-auth-command" and
the "update" git hook.
For gl-auth-command the users control the SSH_ORIGINAL_COMMAND
variable. The parsing on this string is very strict and I didn't
see any problems here.
For the update hook the users partially contol one variable which is
the name of the git branch. It's something like:
$ref = "refs/heads/<branch name>".
This variable is filtered by git so it can't be something like
"../../" or anything like that. Git ends up creating a file with the
branch name under ~/repositories/<repo>.git/refs/heads/<branch name>.
Gitolite handles weird filenames like "$(ls)" correctly, but it would
be better if it just banned those in the update hook.
Logging: Everthing gets logged into ~/.gitolite/logs/. It's pretty
easy to create a page full of fake logs:
logs=$(cat fake.txt)
ssh g...@127.0.1.100 "$logs"
But at the end of the fake logs you can tell that it was faked.
Still, it might be worth it to filter out newlines in log_it().
There isn't any place where newlines are needed.
Misc: I also looked at all the places that called system(), eval(),
exec(), open() or used back ticks. I looked for insecure tempfiles
as well. It all looks pretty good.
regards,
dan carpenter
Sitaram Chamarty
On Fri, Sep 30, 2011 at 06:11:52PM +0300, Dan Carpenter wrote:
> I was asked to do an audit of gitolite and here is my report.
>
> Summary: Gitolite is a very good program. The code is clean. The
> documentation is excelent. It was easy to install. It was written
> with a strong focus on security.
Thank you for the time you must have spent doing this.
Are you OK with giving a bit of background on why you
undertook this? I'm curious. I'm not sure if this is part
of what John Hawley said would would happen now that
kernel.org has decided to use it, or if it is a separate
effort, but either way I appreciate it.
> Attack Surface: Users call two perl scripts "gl-auth-command" and
> the "update" git hook.
>
> For gl-auth-command the users control the SSH_ORIGINAL_COMMAND
> variable. The parsing on this string is very strict and I didn't
> see any problems here.
>
> For the update hook the users partially contol one variable which is
> the name of the git branch. It's something like:
> $ref = "refs/heads/<branch name>".
> This variable is filtered by git so it can't be something like
> "../../" or anything like that. Git ends up creating a file with the
> branch name under ~/repositories/<repo>.git/refs/heads/<branch name>.
> Gitolite handles weird filenames like "$(ls)" correctly, but it would
> be better if it just banned those in the update hook.
While the incoming "ref" is easy enough to control, and
restricting it to $REPONAME_PATT does not sound too
draconian, when we check names of changed files in the
commit series, files *may* contain arbitrary names.
I see three ways around it. (1) Make sure these filenames
never get sent to a shell script where they may accidentally
get evaled (not that perl can't but it would not be
accidental). This is currently true; I just have to make
sure it continues to be so ;-)
(2) be draconian in terms of filenames in projects -- not a
good idea really.
(3) exhaustively list all the *really* dangerous
combinations (like '$(.*)') and ban only those.
For now I'm going with option 1, but I'd like opinions.
> Logging: Everthing gets logged into ~/.gitolite/logs/. It's pretty
> easy to create a page full of fake logs:
> logs=$(cat fake.txt)
> ssh g...@127.0.1.100 "$logs"
> But at the end of the fake logs you can tell that it was faked.
> Still, it might be worth it to filter out newlines in log_it().
> There isn't any place where newlines are needed.
Yup, the next line *does* show it as fake, but it could
still trip automatic parsers etc. Will fix... Thanks!
> Misc: I also looked at all the places that called system(), eval(),
> exec(), open() or used back ticks. I looked for insecure tempfiles
> as well. It all looks pretty good.
>
> regards,
> dan carpenter
Thanks once again for all this.
Regards
sitaram
Dan Carpenter
> Are you OK with giving a bit of background on why you
> undertook this? I'm curious. I'm not sure if this is part
> of what John Hawley said would would happen now that
> kernel.org has decided to use it, or if it is a separate
> effort, but either way I appreciate it.
>
Yes. I'm on the kernel team at Oracle and my manager asked me to
look it.
> > For the update hook the users partially contol one variable which is
> > the name of the git branch. It's something like:
> > $ref = "refs/heads/<branch name>".
> > This variable is filtered by git so it can't be something like
> > "../../" or anything like that. Git ends up creating a file with the
> > branch name under ~/repositories/<repo>.git/refs/heads/<branch name>.
> > Gitolite handles weird filenames like "$(ls)" correctly, but it would
> > be better if it just banned those in the update hook.
>
> While the incoming "ref" is easy enough to control, and
> restricting it to $REPONAME_PATT does not sound too
> draconian,
Could you also make sure that it doesn't end in .git in case
someone does a "find -name \*.git -prune" but without the -prune?
(Sorry, I feel weird for asking that. Feel free to ignore.).
> when we check names of changed files in the
> commit series, files *may* contain arbitrary names.
>
> I see three ways around it. (1) Make sure these filenames
> never get sent to a shell script where they may accidentally
> get evaled (not that perl can't but it would not be
> accidental). This is currently true; I just have to make
> sure it continues to be so ;-)
>
> (2) be draconian in terms of filenames in projects -- not a
> good idea really.
>
> (3) exhaustively list all the *really* dangerous
> combinations (like '$(.*)') and ban only those.
>
> For now I'm going with option 1, but I'd like opinions.
>
Yes. I agree with you that option 1 is best.
regards,
dan carpenter
Sitaram Chamarty
<dan.ca...@oracle.com> wrote:
> On Fri, Sep 30, 2011 at 10:14:19PM +0530, Sitaram Chamarty wrote:
>> Are you OK with giving a bit of background on why you
>> undertook this? I'm curious. I'm not sure if this is part
>> of what John Hawley said would would happen now that
>> kernel.org has decided to use it, or if it is a separate
>> effort, but either way I appreciate it.
>>
>
> Yes. I'm on the kernel team at Oracle and my manager asked me to
> look it.
>
>> > For the update hook the users partially contol one variable which is
>> > the name of the git branch. It's something like:
>> > $ref = "refs/heads/<branch name>".
>> > This variable is filtered by git so it can't be something like
>> > "../../" or anything like that. Git ends up creating a file with the
>> > branch name under ~/repositories/<repo>.git/refs/heads/<branch name>.
>> > Gitolite handles weird filenames like "$(ls)" correctly, but it would
>> > be better if it just banned those in the update hook.
>>
>> While the incoming "ref" is easy enough to control, and
>> restricting it to $REPONAME_PATT does not sound too
>> draconian,
>
> Could you also make sure that it doesn't end in .git in case
> someone does a "find -name \*.git -prune" but without the -prune?
> (Sorry, I feel weird for asking that. Feel free to ignore.).
:-)
I'll do it anyway...
regards
--
Sitaram
Sitaram Chamarty
On Fri, Sep 30, 2011 at 06:11:52PM +0300, Dan Carpenter wrote:
> Gitolite handles weird filenames like "$(ls)" correctly, but it would
> be better if it just banned those in the update hook.
Both the refname passed as arg-1 to the update hook as well
as filenames coming from `git diff --name-only` (when the
repo has NAME/ rules) are now checked against the following
pattern:
^[0-9a-zA-Z][0-9a-zA-Z._\@/+ :,-]*$
The pattern can be changed in the RC file if the admin wants
something more liberal.
> Logging: Everthing gets logged into ~/.gitolite/logs/. It's pretty
> easy to create a page full of fake logs:
> logs=$(cat fake.txt)
> ssh g...@127.0.1.100 "$logs"
> But at the end of the fake logs you can tell that it was faked.
> Still, it might be worth it to filter out newlines in log_it().
> There isn't any place where newlines are needed.
Also fixed.
These changes were tested on "pu", but I also made a
backport branch ("bp-v2.0.3") so you can do a quick
"incremental audit" if you wish, and John can then use that
to upgrade.
regards
sitaram
Sitaram Chamarty
...and in the end I didn't. Instead, I decided to take your
suggestion to "feel free to ignore" :-).
This is not a remotely triggered event, and protecting
against people with local shell access is not something I
want to get into.
regards
sitaram
Sitaram Chamarty
> poking whomever I need to get it mainstreamed. Dan, Sitaram, is it better
> at this point to run with mainline?
Well from my point of view, sure. The current "pu" also
contains fixes for the 2 audit comments discussed earlier in
this thread.
I guess it depends on this: what version was audited, and
are you allowed to run a version that is a wee bit up/down
from it? I assumed "v2.0.3", and "no". I am probably
wrong on both counts :)
regards
sitaram