On Wed, Jan 15, 2014 at 10:28 AM, Peter Wu lekenst...@gmail.com wrote:
The script is vulnerable to header injection:
$ curl -i http://git.zx2c4.com/login -H 'Referer: x%0d\nX: 1' \
-d 'username=1; path%3d/password=%0aY: 2'
HTTP/1.1 302 Redirect
Server: ZX2C4 Web Server
Date: Wed, 15 Jan 2014 08:54:00 GMT
Transfer-Encoding: chunked
Connection: keep-alive
Keep-Alive: timeout=20
Location: x%0d\nX: 1
Set-Cookie: auth=1
Set-Cookie: username=1; path=/
Set-Cookie: password=
Y: 2
While the referrer part may not be that easily spoofable, the post fields
are
a different matter. Speaking of the referrer header, that field might not
be
set at all. What about making the URL available in a post field
return-url?
It still has to be validated though.
Yes, of course. I was aware of this too. Clearly the current spitting out
of headers is unfinished. In the first place, I wouldn't be spitting the
password and username post directly out in the cookies (-- We wouldn't
actually want to set these cookies... Just for testing. in the comments),
let alone leaving everything completely unvalidated. Rest assured, this
won't make it to the master branch.
For the cookie, I suggest to add ; HttpOnly to Set-Cookie to prevent
cookie
theft through XSS. If possible, also add ; Secure to prevent leakage
through
HTTP when HTTPS is used.
I had planned on this too. A good suggestion. None of the cookie generation
is complete right now.
An important consideration is caching. Adding the Set-Cookie header
disables
caching for nginx at least, but other authenticated requests can still be
cached.
Not completely though. I've taken careful precaution to ensure that caching
from the cgit end stays in tact. If the cookie is authenticated, then cgit
is able to serve up the cached pages from its own cache. If the cookie is
unauthenticated, then, yes, it displays an uncached version of the please
authenticate page.
I did not check the ramification this has on nginx's handling of HTTP
caching of resources, however. Can you elaborate on this, if it's a
problem, and how to mitigate it?
This authentication mechanism is unsafe if the transport is not encrypted
(i.e. use HTTPS!), passwords are then leaked in the air. You mentioned
using a HMAC, but what data do you want to protect? For best results,
some state has to be retained. The authentication does not have to rely on
cookies either, it can use client SSL certificates too.
Obviously. All authentication mechanisms in the browser that do not go over
HTTPS are vulnerable. If you have HTTPS, then excellent. If you don't, then
you should be aware of how vulnerable you will be without it. There will be
a note in the documentation about this naturally.
The HMAC mention doesn't have to do with cleartext vs non-cleartext. It's
for this reason -- I'm not going to be relying in an auth=1 cookie for
authentication passing. This is trivially spoofable. Instead, there's going
to be something like ${username}|${expiration-unix-time}|${salt}|${hmac},
so that such state is stored in the cookie itself, but then validated
server side using a secret.
What if the script fails to load (syntax error)? Is access then granted to
everyone, silently ignoring the error? That would be bad, it should then
return an 500 Internal server error.
If the script fails to load due to a syntax error, cgit will bail out. It
fails safe in this regard.
Thanks for your feedback! I'll ping you when I've finished the web side of
things and you can let me know if it's satisfactory.
___
CGit mailing list
CGit@lists.zx2c4.com
http://lists.zx2c4.com/mailman/listinfo/cgit
