Re: [PATCH 2/6] http: always update the base URL for redirects
On 02/12/16 00:18, Jeff King wrote: > On Fri, Dec 02, 2016 at 12:07:50AM +, Ramsay Jones wrote: > In a British context "Mallory and Irvine" were two (male) climbers who died on Everest in 1924 (tales of daring...), so it's easy to expect (from this side of the pond) that 'Mallory' would be male. However he was really George Mallory. Meanwhile that search engine's images shows far more female Mallorys, so I've learnt something. >>> >>> "baby name Mallory" in search engine gave me several sites, most of >>> them telling me that is a girl's name except for one. >>> >>> Didn't think of doing image search, but that's a good way ;-) >> >> Heh, I didn't think about any of this. I was remembering the >> description of 'Man-in-the-middle Attack' from Applied Cryptography >> (Bruce Schneier) which implies that Mallory is male. > > I admit that I always assumed Applied Cryptography (and other papers) > were always talking about a female. But that's probably because I > started with an assumption about the name in the first place. That > probably came from watching the Family Ties sitcom as a kid; the older > daughter is named Mallory (and if you google it, you can see some > amazing 80's haircuts and clothes). > > We can call her Marsha if you want, instead evoking Brady Bunch memories > of 60's clothing and haircuts. I'm not sure it matters too much, but if you are going to change the name then Eve is also used in the description of Man-in-the-middle (see "Practical Cryptography", Ferguson and Schneier). ATB, Ramsay Jones
Re: [PATCH 2/6] http: always update the base URL for redirects
On Fri, Dec 02, 2016 at 12:07:50AM +, Ramsay Jones wrote: > >> In a British context "Mallory and Irvine" were two (male) climbers who > >> died on Everest in 1924 (tales of daring...), so it's easy to expect > >> (from this side of the pond) that 'Mallory' would be male. However he > >> was really George Mallory. > >> > >> Meanwhile that search engine's images shows far more female Mallorys, > >> so I've learnt something. > > > > "baby name Mallory" in search engine gave me several sites, most of > > them telling me that is a girl's name except for one. > > > > Didn't think of doing image search, but that's a good way ;-) > > Heh, I didn't think about any of this. I was remembering the > description of 'Man-in-the-middle Attack' from Applied Cryptography > (Bruce Schneier) which implies that Mallory is male. I admit that I always assumed Applied Cryptography (and other papers) were always talking about a female. But that's probably because I started with an assumption about the name in the first place. That probably came from watching the Family Ties sitcom as a kid; the older daughter is named Mallory (and if you google it, you can see some amazing 80's haircuts and clothes). We can call her Marsha if you want, instead evoking Brady Bunch memories of 60's clothing and haircuts. -Peff
Re: [PATCH 2/6] http: always update the base URL for redirects
On 01/12/16 23:43, Junio C Hamano wrote: > "Philip Oakley" writes: > >>> Depends, I only know Mallorys who are women so her seems appropriate. >>> >> In a British context "Mallory and Irvine" were two (male) climbers who >> died on Everest in 1924 (tales of daring...), so it's easy to expect >> (from this side of the pond) that 'Mallory' would be male. However he >> was really George Mallory. >> >> Meanwhile that search engine's images shows far more female Mallorys, >> so I've learnt something. > > "baby name Mallory" in search engine gave me several sites, most of > them telling me that is a girl's name except for one. > > Didn't think of doing image search, but that's a good way ;-) Heh, I didn't think about any of this. I was remembering the description of 'Man-in-the-middle Attack' from Applied Cryptography (Bruce Schneier) which implies that Mallory is male. ATB, Ramsay Jones
Re: [PATCH 2/6] http: always update the base URL for redirects
"Philip Oakley" writes: >> Depends, I only know Mallorys who are women so her seems appropriate. >> > In a British context "Mallory and Irvine" were two (male) climbers who > died on Everest in 1924 (tales of daring...), so it's easy to expect > (from this side of the pond) that 'Mallory' would be male. However he > was really George Mallory. > > Meanwhile that search engine's images shows far more female Mallorys, > so I've learnt something. "baby name Mallory" in search engine gave me several sites, most of them telling me that is a girl's name except for one. Didn't think of doing image search, but that's a good way ;-)
Re: [PATCH 2/6] http: always update the base URL for redirects
From: "Brandon Williams" On 12/01, Ramsay Jones wrote: On 01/12/16 09:04, Jeff King wrote: > If a malicious server redirects the initial ref > advertisement, it may be able to leak sha1s from other, > unrelated servers that the client has access to. For > example, imagine that Alice is a git user, she has access to > a private repository on a server hosted by Bob, and Mallory > runs a malicious server and wants to find out about Bob's > private repository. > > Mallory asks Alice to clone an unrelated repository from her ---^^^ ... from _him_ ? (ie Mallory) > over HTTP. When Alice's client contacts Mallory's server for > the initial ref advertisement, the server issues an HTTP > redirect for Bob's server. Alice contacts Bob's server and > gets the ref advertisement for the private repository. If > there is anything to fetch, she then follows up by asking > the server for one or more sha1 objects. But who is the > server? > > If it is still Mallory's server, then Alice will leak the > existence of those sha1s to her. --^^^ ... to _him_ ? (again Mallory) ATB, Ramsay Jones Depends, I only know Mallorys who are women so her seems appropriate. -- Brandon Williams In a British context "Mallory and Irvine" were two (male) climbers who died on Everest in 1924 (tales of daring...), so it's easy to expect (from this side of the pond) that 'Mallory' would be male. However he was really George Mallory. Meanwhile that search engine's images shows far more female Mallorys, so I've learnt something. -- Philip
Re: [PATCH 2/6] http: always update the base URL for redirects
On 12/01, Ramsay Jones wrote: > > > On 01/12/16 09:04, Jeff King wrote: > > If a malicious server redirects the initial ref > > advertisement, it may be able to leak sha1s from other, > > unrelated servers that the client has access to. For > > example, imagine that Alice is a git user, she has access to > > a private repository on a server hosted by Bob, and Mallory > > runs a malicious server and wants to find out about Bob's > > private repository. > > > > Mallory asks Alice to clone an unrelated repository from her > ---^^^ > ... from _him_ ? (ie Mallory) > > > over HTTP. When Alice's client contacts Mallory's server for > > the initial ref advertisement, the server issues an HTTP > > redirect for Bob's server. Alice contacts Bob's server and > > gets the ref advertisement for the private repository. If > > there is anything to fetch, she then follows up by asking > > the server for one or more sha1 objects. But who is the > > server? > > > > If it is still Mallory's server, then Alice will leak the > > existence of those sha1s to her. > --^^^ > ... to _him_ ? (again Mallory) > > ATB, > Ramsay Jones Depends, I only know Mallorys who are women so her seems appropriate. -- Brandon Williams
Re: [PATCH 2/6] http: always update the base URL for redirects
On 01/12/16 09:04, Jeff King wrote: > If a malicious server redirects the initial ref > advertisement, it may be able to leak sha1s from other, > unrelated servers that the client has access to. For > example, imagine that Alice is a git user, she has access to > a private repository on a server hosted by Bob, and Mallory > runs a malicious server and wants to find out about Bob's > private repository. > > Mallory asks Alice to clone an unrelated repository from her ---^^^ ... from _him_ ? (ie Mallory) > over HTTP. When Alice's client contacts Mallory's server for > the initial ref advertisement, the server issues an HTTP > redirect for Bob's server. Alice contacts Bob's server and > gets the ref advertisement for the private repository. If > there is anything to fetch, she then follows up by asking > the server for one or more sha1 objects. But who is the > server? > > If it is still Mallory's server, then Alice will leak the > existence of those sha1s to her. --^^^ ... to _him_ ? (again Mallory) ATB, Ramsay Jones
[PATCH 2/6] http: always update the base URL for redirects
If a malicious server redirects the initial ref
advertisement, it may be able to leak sha1s from other,
unrelated servers that the client has access to. For
example, imagine that Alice is a git user, she has access to
a private repository on a server hosted by Bob, and Mallory
runs a malicious server and wants to find out about Bob's
private repository.
Mallory asks Alice to clone an unrelated repository from her
over HTTP. When Alice's client contacts Mallory's server for
the initial ref advertisement, the server issues an HTTP
redirect for Bob's server. Alice contacts Bob's server and
gets the ref advertisement for the private repository. If
there is anything to fetch, she then follows up by asking
the server for one or more sha1 objects. But who is the
server?
If it is still Mallory's server, then Alice will leak the
existence of those sha1s to her.
Since commit c93c92f30 (http: update base URLs when we see
redirects, 2013-09-28), the client usually rewrites the base
URL such that all further requests will go to Bob's server.
But this is done by textually matching the URL. If we were
originally looking for "http://mallory/repo.git/info/refs";,
and we got pointed at "http://bob/other.git/info/refs";, then
we know that the right root is "http://bob/other.git";.
If the redirect appears to change more than just the root,
we punt and continue to use the original server. E.g.,
imagine the redirect adds a URL component that Bob's server
will ignore, like "http://bob/other.git/info/refs?dummy=1";.
We can solve this by aborting in this case rather than
silently continuing to use Mallory's server. In addition to
protecting from sha1 leakage, it's arguably safer and more
sane to refuse a confusing redirect like that in general.
For example, part of the motivation in c93c92f30 is
avoiding accidentally sending credentials over clear http,
just to get a response that says "try again over https". So
even in a non-malicious case, we'd prefer to err on the side
of caution.
The downside is that it's possible this will break a
legitimate but complicated server-side redirection scheme.
The setup given in the newly added test does work, but it's
convoluted enough that we don't need to care about it. A
more plausible case would be a server which redirects a
request for "info/refs?service=git-upload-pack" to just
"info/refs" (because it does not do smart HTTP, and for some
reason really dislikes query parameters). Right now we
would transparently downgrade to dumb-http, but with this
patch, we'd complain (and the user would have to set
GIT_SMART_HTTP=0 to fetch).
Reported-by: Jann Horn
Signed-off-by: Jeff King
---
http.c| 12
t/lib-httpd/apache.conf | 8
t/t5551-http-fetch-smart.sh | 4
t/t5812-proto-disable-http.sh | 1 +
4 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/http.c b/http.c
index 840dbd1c7..ba03beead 100644
--- a/http.c
+++ b/http.c
@@ -1655,9 +1655,9 @@ static int http_request(const char *url,
*
* Note that this assumes a sane redirect scheme. It's entirely possible
* in the example above to end up at a URL that does not even end in
- * "info/refs". In such a case we simply punt, as there is not much we can
- * do (and such a scheme is unlikely to represent a real git repository,
- * which means we are likely about to abort anyway).
+ * "info/refs". In such a case we die. There's not much we can do, such a
+ * scheme is unlikely to represent a real git repository, and failing to
+ * rewrite the base opens options for malicious redirects to do funny things.
*/
static int update_url_from_redirect(struct strbuf *base,
const char *asked,
@@ -1675,10 +1675,14 @@ static int update_url_from_redirect(struct strbuf *base,
new_len = got->len;
if (!strip_suffix_mem(got->buf, &new_len, tail))
- return 0; /* insane redirect scheme */
+ die(_("unable to update url base from redirection:\n"
+ " asked for: %s\n"
+ " redirect: %s"),
+ asked, got->buf);
strbuf_reset(base);
strbuf_add(base, got->buf, new_len);
+
return 1;
}
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index c3e631394..f98b23a3c 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -132,6 +132,14 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1
[R=302]
RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1
[R=302]
RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
+# The first rule issues a client-side redirect to something
+# that _doesn't_ look like a git repo. The second rule is a
+# server-side rewrite, so that it turns out the odd-looking
+# thing _is_ a git repo. The "[PT]" tells Apache to match
+# the usual ScriptAlias rules for /smart.
+RewriteRule ^/insane-redir/(.*)$ /intern-redir/$1/foo [R=301]
+RewriteRule ^/intern-r
