authentication support: work has begun!

authentication support: work has begun!

[Jason]

Hi folks,

While still a horrendous mess, I've begun work adding authentication
support, using our nice new lua filter system.

A sample script looks like this [at the moment]:

http://git.zx2c4.com/cgit/tree/filters/simple-authentication.lua?h=jd/authentication

The full commit of this attrocity looks like this:
   http://git.zx2c4.com/cgit/commit/?h=jd/authentication

It's far from finished or polished, but I thought I'd give everyone a
preview of it. It's running on:
    http://git.zx2c4.com/glouglou/log

Currently just enter anything you want as username and password. It will
set a cookie. Check out the HTTP headers and response and everything to see
what's happening.

Preliminary comments?

Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20140115/c9c025fa/attachment.html>

authentication support: work has begun!

[lekensteyn]

Hi,

On Wednesday 15 January 2014 02:02:13 Jason A. Donenfeld wrote:
> While still a horrendous mess, I've begun work adding authentication
> support, using our nice new lua filter system.
> 
> A sample script looks like this [at the moment]:
> 
> http://git.zx2c4.com/cgit/tree/filters/simple-authentication.lua?h=jd/authen
> tication
> 
> The full commit of this attrocity looks like this:
>    http://git.zx2c4.com/cgit/commit/?h=jd/authentication
> 
> It's far from finished or polished, but I thought I'd give everyone a
> preview of it. It's running on:
>     http://git.zx2c4.com/glouglou/log
> 
> Currently just enter anything you want as username and password. It will
> set a cookie. Check out the HTTP headers and response and everything to see
> what's happening.
> 
> Preliminary comments?

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.

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.

An important consideration is caching. Adding the Set-Cookie header disables
caching for nginx at least, but other authenticated requests can still be
cached.

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.

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.

Regards,
Peter

authentication support: work has begun!

[Jason]

On Wed, Jan 15, 2014 at 10:28 AM, Peter Wu <lekensteyn at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20140115/5dcd4a62/attachment.html>

authentication support: work has begun!

[Jason]

Username: jason
Password: secretpassword
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20140115/ceb180e9/attachment.html>

authentication support: work has begun!

[lekensteyn]

On Wednesday 15 January 2014 14:42:12 Jason A. Donenfeld wrote:
> > 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?

Consider the use of fastcgi_cache as shown in this config:
http://lists.zx2c4.com/pipermail/cgit/2013-April/001307.html

The current login page is cachable, you should add "Cache-Control: private" to 
prevent that. Otherwise, with the nginx fastcgi cache enabled, you will still
see the login page. Perhaps a redirect to /login?url=%2Fglouglou%2Flog is more 
obvious? (302)

A related issue exists when
(1) an authenticated user requests the resource. nginx caches it.
(2) an unauthenticated user requests the resource. nginx cache returns the 
supposedly password-protected resource.

Again, use of the Cache-Control header would help here, but then no protected 
resource will be cached by nginx (bug/feature).

> 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.

Yes, I understood that auth=1 is a temporary hack for demonstration purposes, 
your use of a HMAC looks safe. :)

Aside from storing passwords in plaintext, I see no other obvious issues.

Thanks,
Peter

authentication support: work has begun!

[Jason]

On Wed, Jan 15, 2014 at 7:17 PM, Peter Wu <lekensteyn at gmail.com> wrote:
> Aside from storing passwords in plaintext, I see no other obvious issues.

I'm not too keen on this either. Care to submit a patch against
jd/authentication that does a crypt() / mkpasswd salted hash
situation? Does luacrypto support this? Investigate it?

> The current login page is cachable, you should add "Cache-Control: private" to
> prevent that.

Excellent idea.

authentication support: work has begun!

[Jason]

On Wed, Jan 15, 2014 at 7:29 PM, Jason A. Donenfeld <Jason at zx2c4.com> wrote:
> On Wed, Jan 15, 2014 at 7:17 PM, Peter Wu <lekensteyn at gmail.com> wrote:
>> The current login page is cachable, you should add "Cache-Control: private" to
>> prevent that.
>
> Excellent idea.

I've added no-cache, no-store to the login page and the redirection.

I've also merged this to master. Please test, and send any fixes you find!

authentication support: work has begun!

[Jason]

On Wed, Jan 15, 2014 at 10:28 AM, Peter Wu <lekensteyn at gmail.com> wrote:
> While the referrer part may not be that easily spoofable

Note that as of b826537 we no longer rely on the referer and instead
use a hidden html form with a secured value. This also doubles as CSRF
protection.