<br><br><div class="gmail_quote">On Wed, Jan 15, 2014 at 10:28 AM, Peter Wu <span dir="ltr"><<a href="mailto:lekensteyn@gmail.com" target="_blank">lekensteyn@gmail.com</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The script is vulnerable to header injection:<br>
<br>
$ curl -i <a href="http://git.zx2c4.com/login" target="_blank">http://git.zx2c4.com/login</a> -H 'Referer: x%0d\nX: 1' \<br>
  -d 'username=1; path%3d/&password=%0aY: 2'<br>
HTTP/1.1 302 Redirect<br>
Server: ZX2C4 Web Server<br>
Date: Wed, 15 Jan 2014 08:54:00 GMT<br>
Transfer-Encoding: chunked<br>
Connection: keep-alive<br>
Keep-Alive: timeout=20<br>
Location: x%0d\nX: 1<br>
Set-Cookie: auth=1<br>
Set-Cookie: username=1; path=/<br>
Set-Cookie: password=<br>
Y: 2<br>
<br>
While the referrer part may not be that easily spoofable, the post fields are<br>
a different matter. Speaking of the referrer header, that field might not be<br>
set at all. What about making the URL available in a post field  "return-url"?<br>
It still has to be validated though.<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
For the cookie, I suggest to add "; HttpOnly" to Set-Cookie to prevent cookie<br>
theft through XSS. If possible, also add "; Secure" to prevent leakage through<br>
HTTP when HTTPS is used.<br></blockquote><div><br></div><div>I had planned on this too. A good suggestion. None of the cookie generation is complete right now.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
An important consideration is caching. Adding the Set-Cookie header disables<br>
caching for nginx at least, but other authenticated requests can still be<br>
cached.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
This authentication mechanism is unsafe if the transport is not encrypted<br>
(i.e. use HTTPS!), passwords are then leaked in the air. You mentioned<br>
using a HMAC, but what data do you want to protect? For best results,<br>
some state has to be retained. The authentication does not have to rely on<br>
cookies either, it can use client SSL certificates too.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
What if the script fails to load (syntax error)? Is access then granted to<br>
everyone, silently ignoring the error? That would be bad, it should then<br>
return an 500 Internal server error.</blockquote><div><br></div><div>If the script fails to load due to a syntax error, cgit will bail out. It "fails safe" in this regard. </div><div><br></div><div><br></div><div>
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.</div></div>