[PATCH] Encode value and field before calculating cookie digest, the same way secure_value() does

[Vlad]

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>