* [PATCH] git-daemon extra paranoia
@ 2005-10-18 20:54 H. Peter Anvin
2005-10-18 21:19 ` Junio C Hamano
2005-10-18 22:25 ` [PATCH] " Linus Torvalds
0 siblings, 2 replies; 12+ messages in thread
From: H. Peter Anvin @ 2005-10-18 20:54 UTC (permalink / raw)
To: Git Mailing List
[-- Attachment #1: Type: text/plain, Size: 271 bytes --]
This patch adds some extra paranoia to the git-daemon filename test. In
particular, it now rejects pathnames containing // or ending with /; it
also adds a redundant test for pathname absoluteness (belts and suspenders.)
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1185 bytes --]
Extra paranoia about non-canonical pathnames
---
commit a22f643931e48a319a70af7e91f809648160ecbf
tree 9d6934089c2628253d0690efde3fa7f36a1a8861
parent 4aaa702794447d9b281dd22fe532fd61e02434e1
author Peter Anvin <hpa@tazenda.sc.orionmulti.com> Tue, 18 Oct 2005 13:51:45 -0700
committer Peter Anvin <hpa@tazenda.sc.orionmulti.com> Tue, 18 Oct 2005 13:51:45 -0700
daemon.c | 16 ++++++++++++----
1 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/daemon.c b/daemon.c
--- a/daemon.c
+++ b/daemon.c
@@ -80,17 +80,25 @@ static int path_ok(const char *dir)
{
const char *p = dir;
char **pp;
- int sl = 1, ndot = 0;
+ int sl, ndot;
+
+ /* The pathname here should be an absolute path. */
+ if ( *p++ != '/' )
+ return 0;
+
+ sl = 1; ndot = 0;
for (;;) {
if ( *p == '.' ) {
ndot++;
} else if ( *p == '/' || *p == '\0' ) {
- if ( sl && ndot > 0 && ndot < 3 )
- return 0; /* . or .. in path */
+ if ( sl && ndot < 3 ) /* Refuse "", "." or ".." */
+ return 0;
sl = 1;
+
+ /* If this was end of string, we passed all tests */
if ( *p == '\0' )
- break; /* End of string and all is good */
+ break;
} else {
sl = ndot = 0;
}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-daemon extra paranoia
2005-10-18 20:54 [PATCH] git-daemon extra paranoia H. Peter Anvin
@ 2005-10-18 21:19 ` Junio C Hamano
2005-10-18 21:29 ` H. Peter Anvin
2005-10-18 22:25 ` [PATCH] " Linus Torvalds
1 sibling, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2005-10-18 21:19 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: git
"H. Peter Anvin" <hpa@zytor.com> writes:
> This patch adds some extra paranoia to the git-daemon filename test. In
> particular, it now rejects pathnames containing // or ending with /; it
> also adds a redundant test for pathname absoluteness (belts and suspenders.)
>
> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> Extra paranoia about non-canonical pathnames
I would understand rejecting /../, and perhaps /./, but why
reject // in between or / at the end?
Especially, I think this part in daemon.c::upload():
if (!path_ok(dir)) {
logerror("Forbidden directory: %s\n", dir);
return -1;
}
if (chdir(dir) < 0) {
logerror("Cannot chdir('%s'): %s", dir, strerror(errno));
return -1;
}
chdir(".git");
relies on the fact that you can say "/home/junio/git/" for me to
publish "/home/junio/git/.git/" repository, so I would suspect
that it is necessary to allow "ending with /" at least.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-daemon extra paranoia
2005-10-18 21:19 ` Junio C Hamano
@ 2005-10-18 21:29 ` H. Peter Anvin
2005-10-18 22:08 ` H. Peter Anvin
0 siblings, 1 reply; 12+ messages in thread
From: H. Peter Anvin @ 2005-10-18 21:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Junio C Hamano wrote:
>
> I would understand rejecting /../, and perhaps /./, but why
> reject // in between or / at the end?
>
For security, avoiding aliases is highly desirable, and if they're
useless the easiest way to do that is to reject. If aliases are
required, which it sounds like it might be, then canonicalization needs
to be applied.
This may sound redundant, but a lot of avoiding security holes involves
applying good practices up front, instead of reactively.
Allowing a terminal slash should be reasonably easy, though.
-hpa
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-daemon extra paranoia
2005-10-18 21:29 ` H. Peter Anvin
@ 2005-10-18 22:08 ` H. Peter Anvin
2005-10-18 22:13 ` [PATCH] Revised - " H. Peter Anvin
0 siblings, 1 reply; 12+ messages in thread
From: H. Peter Anvin @ 2005-10-18 22:08 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: Junio C Hamano, git
H. Peter Anvin wrote:
>
> For security, avoiding aliases is highly desirable, and if they're
> useless the easiest way to do that is to reject. If aliases are
> required, which it sounds like it might be, then canonicalization needs
> to be applied.
>
> This may sound redundant, but a lot of avoiding security holes involves
> applying good practices up front, instead of reactively.
>
I thought I might want to add a bit of an explanation, just for the
purpose of illustration.
Right now, we use a whitelist for access control. Aliases are not a
problem, because they fail shut.
A year from now, someone decides that they want a "all but" feature, and
thus adds a blacklist on top of the whitelist. If aliases are
permitted, unless the blacklist logic is written very carefully, one
would then be able to get around the blacklist by using one of the
aliased paths.
Improper handling of aliases is probably second only to buffer overflows
and large-string DoS attacks when it comes to security vulnerabilities.
-hpa
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] Revised - git-daemon extra paranoia
2005-10-18 22:08 ` H. Peter Anvin
@ 2005-10-18 22:13 ` H. Peter Anvin
0 siblings, 0 replies; 12+ messages in thread
From: H. Peter Anvin @ 2005-10-18 22:13 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: Junio C Hamano, git
[-- Attachment #1: Type: text/plain, Size: 318 bytes --]
This patch adds some extra paranoia to the git-daemon filename test. In
particular, it now rejects pathnames containing //; it also adds a
redundant test for pathname absoluteness (belts and suspenders.)
A single / at the end of the path is still permitted, however.
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 833 bytes --]
diff --git a/daemon.c b/daemon.c
--- a/daemon.c
+++ b/daemon.c
@@ -80,17 +80,29 @@ static int path_ok(const char *dir)
{
const char *p = dir;
char **pp;
- int sl = 1, ndot = 0;
+ int sl, ndot;
+
+ /* The pathname here should be an absolute path. */
+ if ( *p++ != '/' )
+ return 0;
+
+ sl = 1; ndot = 0;
for (;;) {
if ( *p == '.' ) {
ndot++;
- } else if ( *p == '/' || *p == '\0' ) {
+ } else if ( *p == '\0' ) {
+ /* Reject "." and ".." at the end of the path */
if ( sl && ndot > 0 && ndot < 3 )
- return 0; /* . or .. in path */
+ return 0;
+
+ /* Otherwise OK */
+ break;
+ } else if ( *p == '/' ) {
+ /* Refuse "", "." or ".." */
+ if ( sl && ndot < 3 )
+ return 0;
sl = 1;
- if ( *p == '\0' )
- break; /* End of string and all is good */
} else {
sl = ndot = 0;
}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-daemon extra paranoia
2005-10-18 20:54 [PATCH] git-daemon extra paranoia H. Peter Anvin
2005-10-18 21:19 ` Junio C Hamano
@ 2005-10-18 22:25 ` Linus Torvalds
2005-10-18 22:47 ` Junio C Hamano
2005-10-19 0:21 ` H. Peter Anvin
1 sibling, 2 replies; 12+ messages in thread
From: Linus Torvalds @ 2005-10-18 22:25 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: Git Mailing List
On Tue, 18 Oct 2005, H. Peter Anvin wrote:
>
> This patch adds some extra paranoia to the git-daemon filename test. In
> particular, it now rejects pathnames containing // or ending with /; it also
> adds a redundant test for pathname absoluteness (belts and suspenders.)
Hmm. The "not ending in /" is a bad test.
Especially in light of the fact that the git-pack protocol quite by design
tends to add a ".git" to the end as a fallback, so that a user that wants
to specify a particular directory _without_ that fallback needs to have
the slash at the end.
Now, git-daemon hasn't implemented that, but I think that was just a
mistake that grew out of it not getting a lot of testing, since it wasn't
used much. I personally use the "without the final .git" version quite
often, because it just looks so much nicer for the user.
In fact, here's a patch that makes git-daemon allow it, and thus match the
behaviour of the ssh transport.
The logic is simple: if the original "chdir()" fails, try another one with
".git" appended. This is in _addition_ to doing the 'chdir(".git")' later,
so that if you have a checked-out git repository in /home/linux-2.6.git,
then doing a
git pull git://host/home/linux-2.6
will on the remote end do:
chmod("/home/linux-2.6") // fails with ENOENT
chmod("/home/linux-2.6.git") // works
chmod(".git") // works
resulting in it ending up in /home/linux-2.6.git/.git, which is exactly
correct, and where it wants to be.
I personally find it a nice bit of usability enhancement. You can name
your git repositories with a ".git" suffix (which can help all kinds of
automated tasks - like autopacking), but you don't force your users to
care.
Linus
---
diff --git a/daemon.c b/daemon.c
index 11fa3ed..a488512 100644
--- a/daemon.c
+++ b/daemon.c
@@ -128,8 +128,13 @@ static int upload(char *dir, int dirlen)
}
if (chdir(dir) < 0) {
- logerror("Cannot chdir('%s'): %s", dir, strerror(errno));
- return -1;
+ int err = errno;
+ strcpy(dir + dirlen, ".git");
+ if (err != ENOENT || chdir(dir) < 0) {
+ dir[dirlen] = 0;
+ logerror("Cannot chdir('%s'): %s", dir, strerror(err));
+ return -1;
+ }
}
chdir(".git");
@@ -164,7 +169,12 @@ static int execute(void)
static char line[1000];
int len;
- len = packet_read_line(0, line, sizeof(line));
+ /*
+ * Make sure that we leave room for an extra ".git" at
+ * the end of the line. Note that the packet interfaces
+ * already guarantee that there is an ending '\0'.
+ */
+ len = packet_read_line(0, line, sizeof(line)-4);
if (len && line[len-1] == '\n')
line[--len] = 0;
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] git-daemon extra paranoia
2005-10-18 22:25 ` [PATCH] " Linus Torvalds
@ 2005-10-18 22:47 ` Junio C Hamano
2005-10-18 23:21 ` Linus Torvalds
2005-10-19 0:21 ` H. Peter Anvin
1 sibling, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2005-10-18 22:47 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git
Linus Torvalds <torvalds@osdl.org> writes:
> git pull git://host/home/linux-2.6
>
> will on the remote end do:
>
> chdir("/home/linux-2.6") // fails with ENOENT
> chdir("/home/linux-2.6.git") // works
> chdir(".git") // works
>
> resulting in it ending up in /home/linux-2.6.git/.git, which is exactly
> correct, and where it wants to be.
>
> I personally find it a nice bit of usability enhancement. You can name
> your git repositories with a ".git" suffix (which can help all kinds of
> automated tasks - like autopacking), but you don't force your users to
> care.
Wouldn't having /home/linux-2.6/.git/ repository with
/home/linux-2.6/ working tree be good enough for that? Instead
of doing "find / -type d -name '*.git'" you could do "find /
-type d -name .git" for automated tasks.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-daemon extra paranoia
2005-10-18 22:47 ` Junio C Hamano
@ 2005-10-18 23:21 ` Linus Torvalds
0 siblings, 0 replies; 12+ messages in thread
From: Linus Torvalds @ 2005-10-18 23:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Tue, 18 Oct 2005, Junio C Hamano wrote:
>
> Wouldn't having /home/linux-2.6/.git/ repository with
> /home/linux-2.6/ working tree be good enough for that? Instead
> of doing "find / -type d -name '*.git'" you could do "find /
> -type d -name .git" for automated tasks.
In this case, yes.
But In a mixed environment where you might have "bare" repositories, you
want to have "reponame.git" as the repository name.
So with the rule that (a) try to first append ".git" and (b) then, after a
successful chdir, try to go in one more level, you can handle both types,
without ever having to care whether it's checked-out or not.
And for secondary projects (where git isn't necessarily the primary source
control method), I actually use the "project.git" naming just to make it
obvious that this is the "gitified" version of the project.
For example, I keep both my private uemacs and pine source trees as git
repositories these days, and I have them under "~/src/uemacs.git/" and
"~/src/pine.git/" even though they are checked out and thus actually have
another ".git" inside of them.
Linus
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-daemon extra paranoia
2005-10-18 22:25 ` [PATCH] " Linus Torvalds
2005-10-18 22:47 ` Junio C Hamano
@ 2005-10-19 0:21 ` H. Peter Anvin
2005-10-19 0:41 ` Linus Torvalds
1 sibling, 1 reply; 12+ messages in thread
From: H. Peter Anvin @ 2005-10-19 0:21 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List
Linus Torvalds wrote:
>
> Hmm. The "not ending in /" is a bad test.
>
> Especially in light of the fact that the git-pack protocol quite by design
> tends to add a ".git" to the end as a fallback, so that a user that wants
> to specify a particular directory _without_ that fallback needs to have
> the slash at the end.
>
> Now, git-daemon hasn't implemented that, but I think that was just a
> mistake that grew out of it not getting a lot of testing, since it wasn't
> used much. I personally use the "without the final .git" version quite
> often, because it just looks so much nicer for the user.
>
> In fact, here's a patch that makes git-daemon allow it, and thus match the
> behaviour of the ssh transport.
>
> The logic is simple: if the original "chdir()" fails, try another one with
> ".git" appended. This is in _addition_ to doing the 'chdir(".git")' later,
> so that if you have a checked-out git repository in /home/linux-2.6.git,
> then doing a
>
This is also exactly the kind of DWIM that tends to result in the kind
of security holes I described earlier.
The DWIM aspect is fine, of course, but it has to be done up front:
instead of doing just chdir(), each path should be validated through
path_ok() before even being considered for chdir(). Perhaps the right
thing to do is to combine the two functions.
-hpa
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-daemon extra paranoia
2005-10-19 0:21 ` H. Peter Anvin
@ 2005-10-19 0:41 ` Linus Torvalds
2005-10-19 0:43 ` H. Peter Anvin
0 siblings, 1 reply; 12+ messages in thread
From: Linus Torvalds @ 2005-10-19 0:41 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: Git Mailing List
On Tue, 18 Oct 2005, H. Peter Anvin wrote:
>
> This is also exactly the kind of DWIM that tends to result in the kind of
> security holes I described earlier.
I don't agree.
DWIM isn't automatically a security hole. DWIM _can_ be a security hole,
but so can anything else that is badly designed or specified.
And just appending ".git" is _not_ badly designed/specified. I did think
about the boundary cases, and it's entirely safe:
- it can't result in "surprises": if the original pathname doesn't exist,
then even if there is a race and it got created in between the two
chdir's as a directory and the name had a slash at the end, adding
".git" is actually safe even if it succeeds: it won't take us anywhere
surprising. At worst it will take us to the ".git" directory of a newly
added git archive, but that's what we wanted anyway, so..
- you can't create ".." with it - even if the passed-in filename ended
with "xyz/.", you'll end up with a perfectly safe "xyz/..git", so any
safety checks that were done on the original pathname are still valid
when appending ".git" to it.
- and exactly because we don't append slashes or anything like that, the
end result won't even have anything ambiguous like "//" in it.
So it really doesn't have any downsides that I can see.
> The DWIM aspect is fine, of course, but it has to be done up front: instead of
> doing just chdir(), each path should be validated through path_ok() before
> even being considered for chdir(). Perhaps the right thing to do is to
> combine the two functions.
Sure, you could do that, and just replace path_ok + chdir with a
"safe_chdir()". I don't really see the point, unless you want to walk the
path one component at a time, though (which is really quite expensive).
If you want to verify that it's still on the same filesystem and didn't
traverse any dubious symlinks (the only reason to do the component walking
afaik), it's actually much cheaper to just do the chdir() and then do a
"getcwd()" to verify that the result matches. At least under Linux.
(That, btw, is likely the right way to do "valid directory checking"
anyway: if you have a white-list of acceptable directories, just do a
chdir() blindly without any checking, then do "getcwd()" and check the
result of that against the whitelist - then you can even allow ".." etc,
and never even care)
Linus
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-daemon extra paranoia
2005-10-19 0:41 ` Linus Torvalds
@ 2005-10-19 0:43 ` H. Peter Anvin
2005-10-19 1:18 ` Junio C Hamano
0 siblings, 1 reply; 12+ messages in thread
From: H. Peter Anvin @ 2005-10-19 0:43 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List
Linus Torvalds wrote:
> And just appending ".git" is _not_ badly designed/specified. I did think
> about the boundary cases, and it's entirely safe:
>
> - it can't result in "surprises": if the original pathname doesn't exist,
> then even if there is a race and it got created in between the two
> chdir's as a directory and the name had a slash at the end, adding
> ".git" is actually safe even if it succeeds: it won't take us anywhere
> surprising. At worst it will take us to the ".git" directory of a newly
> added git archive, but that's what we wanted anyway, so..
>
> - you can't create ".." with it - even if the passed-in filename ended
> with "xyz/.", you'll end up with a perfectly safe "xyz/..git", so any
> safety checks that were done on the original pathname are still valid
> when appending ".git" to it.
>
> - and exactly because we don't append slashes or anything like that, the
> end result won't even have anything ambiguous like "//" in it.
>
> So it really doesn't have any downsides that I can see.
>
Consider the whitelist/blacklist scenario I described in the previous
email. You have:
whitelist: /pub/scm
blacklist: /pub/scm/foo/bar.git
If you can bypass the blacklist by using the pathname /pub/scm/foo/bar,
that's bad.
>
>>The DWIM aspect is fine, of course, but it has to be done up front: instead of
>>doing just chdir(), each path should be validated through path_ok() before
>>even being considered for chdir(). Perhaps the right thing to do is to
>>combine the two functions.
>
> Sure, you could do that, and just replace path_ok + chdir with a
> "safe_chdir()". I don't really see the point, unless you want to walk the
> path one component at a time, though (which is really quite expensive).
>
The only reason to do that is to make it less likely that a future
programmer would screw it up.
-hpa
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] git-daemon extra paranoia
2005-10-19 0:43 ` H. Peter Anvin
@ 2005-10-19 1:18 ` Junio C Hamano
0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2005-10-19 1:18 UTC (permalink / raw)
To: git
"H. Peter Anvin" <hpa@zytor.com> writes:
> Consider the whitelist/blacklist scenario I described in the previous
> email. You have:
>
> whitelist: /pub/scm
> blacklist: /pub/scm/foo/bar.git
>
> If you can bypass the blacklist by using the pathname /pub/scm/foo/bar,
> that's bad.
I like the simplicity of the check Linus suggested. Given
/pub/scm/fora/../foo/bar/, you would end up chdir() to
/pub/scm/foo/bar.git and getcwd() would hit the blacklist
entry. Which almost means that you do not even need path_ok()
;-).
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2005-10-19 1:18 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-10-18 20:54 [PATCH] git-daemon extra paranoia H. Peter Anvin
2005-10-18 21:19 ` Junio C Hamano
2005-10-18 21:29 ` H. Peter Anvin
2005-10-18 22:08 ` H. Peter Anvin
2005-10-18 22:13 ` [PATCH] Revised - " H. Peter Anvin
2005-10-18 22:25 ` [PATCH] " Linus Torvalds
2005-10-18 22:47 ` Junio C Hamano
2005-10-18 23:21 ` Linus Torvalds
2005-10-19 0:21 ` H. Peter Anvin
2005-10-19 0:41 ` Linus Torvalds
2005-10-19 0:43 ` H. Peter Anvin
2005-10-19 1:18 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).