List for cgit developers and users
 help / color / mirror / Atom feed
* [PATCH] Encode value and field before calculating cookie digest, the same way secure_value() does
@ 2018-04-12 17:54 thevlad
  2018-06-16 16:28 ` john
  0 siblings, 1 reply; 3+ messages in thread
From: thevlad @ 2018-04-12 17:54 UTC (permalink / raw)


From: Vlad Safronov <thevlad at gmail.com>

Bugfix: Encode value and field before calculating cookie digest, the same way as secure_value() does
so validating will work correctly on encoded values.
---
 filters/simple-authentication.lua | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/filters/simple-authentication.lua b/filters/simple-authentication.lua
index de34d09..b40a819 100644
--- a/filters/simple-authentication.lua
+++ b/filters/simple-authentication.lua
@@ -230,6 +230,8 @@ function validate_value(expected_field, cookie)
 		return nil
 	end
 
+	value = url_encode(value)
+	field = url_encode(field)
 	-- Lua hashes strings, so these comparisons are time invariant.
 	if hmac ~= crypto.hmac.digest("sha1", field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt, secret) then
 		return nil
-- 
2.17.0



^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH] Encode value and field before calculating cookie digest,  the same way secure_value() does
  2018-04-12 17:54 [PATCH] Encode value and field before calculating cookie digest, the same way secure_value() does thevlad
@ 2018-06-16 16:28 ` john
       [not found]   ` <CALR6X0_NbTSKE0DPDoni7H6M6d1HoXco=PBCJQRQOeVZGaGwZw@mail.gmail.com>
  0 siblings, 1 reply; 3+ messages in thread
From: john @ 2018-06-16 16:28 UTC (permalink / raw)


On Thu, Apr 12, 2018 at 08:54:31PM +0300, thevlad at gmail.com wrote:
> From: Vlad Safronov <thevlad at gmail.com>
> 
> Bugfix: Encode value and field before calculating cookie digest, the same way as secure_value() does
> so validating will work correctly on encoded values.

Missing sign-off (see [1] for what this means).

[1] https://elinux.org/Developer_Certificate_Of_Origin

However, I don't think this change is correct.  secure_value() places
the encoded strings into the authstr, which is what validate_value() is
reading, so field and value are already URL encoded here.

This is why field is url_decode()'d later in this function before
comparing with the expected value.

> ---
>  filters/simple-authentication.lua | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/filters/simple-authentication.lua b/filters/simple-authentication.lua
> index de34d09..b40a819 100644
> --- a/filters/simple-authentication.lua
> +++ b/filters/simple-authentication.lua
> @@ -230,6 +230,8 @@ function validate_value(expected_field, cookie)
>  		return nil
>  	end
>  
> +	value = url_encode(value)
> +	field = url_encode(field)
>  	-- Lua hashes strings, so these comparisons are time invariant.
>  	if hmac ~= crypto.hmac.digest("sha1", field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt, secret) then
>  		return nil
> -- 
> 2.17.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH] Encode value and field before calculating cookie digest,  the same way secure_value() does
       [not found]     ` <20180617175023.GM1922@john.keeping.me.uk>
@ 2018-06-17 19:03       ` thevlad
  0 siblings, 0 replies; 3+ messages in thread
From: thevlad @ 2018-06-17 19:03 UTC (permalink / raw)


Adding list address removed unintentionally.

> Hmm... but the other caller of validate_value() does pass in an encoded
> value from a POSTed form field.

local redirect = validate_value("redirect", post["redirect"])

post hash is populated with decoded values by parse_qs() function.

>
> I think the real bug is that get_cookie() and set_cookie() are
> asymmetric, so we should just remove the url_decode() in get_cookie().


Well, that was my initial thought but secure_value() and validate_value()
were those asymmetric.
get_cookie() and set_cookie() has nothing to do with it.

secure_value() shall encode values before for digesting by design to avoid
clash
with user value "|" and one used by function as separator.

/v

On Sun, Jun 17, 2018 at 8:50 PM, John Keeping <john at keeping.me.uk> wrote:

> Did you mean to drop the list in this reply?
>
> I'm not very familiar with this Lua code, so someone else might have
> better insight.
>
> On Sun, Jun 17, 2018 at 08:11:32PM +0300, Vlad wrote:
> > Unfortunatelly validate_value() is called with _decoded_ cookie:
> >
> > local username = validate_value("username", get_cookie(http["cookie"],
> > "cgitauth"))
> > ..
> >
> > because get_cookie() returns decoded values:
> > ..
> > function get_cookie (cookies, name)
> >   cookies = string.gsub(";" .. cookies .. ";", "%s*;%s*", ";")
> >   return url_decode(string.match(cookies, ";" .. name .. "=(.-);"))
> > end
> >
> >
> > thus validate_value() will never validate for values/fields with e.g.
> > non-ascii characters.
>
> Hmm... but the other caller of validate_value() does pass in an encoded
> value from a POSTed form field.
>
> I think the real bug is that get_cookie() and set_cookie() are
> asymmetric, so we should just remove the url_decode() in get_cookie().
>
> > On Sat, Jun 16, 2018 at 7:28 PM, John Keeping <john at keeping.me.uk>
> wrote:
> >
> > > On Thu, Apr 12, 2018 at 08:54:31PM +0300, thevlad at gmail.com wrote:
> > > > From: Vlad Safronov <thevlad at gmail.com>
> > > >
> > > > Bugfix: Encode value and field before calculating cookie digest, the
> > > same way as secure_value() does
> > > > so validating will work correctly on encoded values.
> > >
> > > Missing sign-off (see [1] for what this means).
> > >
> > > [1] https://elinux.org/Developer_Certificate_Of_Origin
> > >
> > > However, I don't think this change is correct.  secure_value() places
> > > the encoded strings into the authstr, which is what validate_value() is
> > > reading, so field and value are already URL encoded here.
> > >
> > > This is why field is url_decode()'d later in this function before
> > > comparing with the expected value.
> > >
> > > > ---
> > > >  filters/simple-authentication.lua | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/filters/simple-authentication.lua b/filters/simple-
> > > authentication.lua
> > > > index de34d09..b40a819 100644
> > > > --- a/filters/simple-authentication.lua
> > > > +++ b/filters/simple-authentication.lua
> > > > @@ -230,6 +230,8 @@ function validate_value(expected_field, cookie)
> > > >               return nil
> > > >       end
> > > >
> > > > +     value = url_encode(value)
> > > > +     field = url_encode(field)
> > > >       -- Lua hashes strings, so these comparisons are time invariant.
> > > >       if hmac ~= crypto.hmac.digest("sha1", field .. "|" .. value ..
> "|"
> > > .. tostring(expiration) .. "|" .. salt, secret) then
> > > >               return nil
> > > > --
> > > > 2.17.0
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20180617/64a713ce/attachment.html>


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-06-17 19:03 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-12 17:54 [PATCH] Encode value and field before calculating cookie digest, the same way secure_value() does thevlad
2018-06-16 16:28 ` john
     [not found]   ` <CALR6X0_NbTSKE0DPDoni7H6M6d1HoXco=PBCJQRQOeVZGaGwZw@mail.gmail.com>
     [not found]     ` <20180617175023.GM1922@john.keeping.me.uk>
2018-06-17 19:03       ` thevlad

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