<div dir="ltr"><div>Adding list address removed unintentionally.<br><br>> Hmm... but the other caller of validate_value() does pass in an encoded<br>> value from a POSTed form field.<br><br>local redirect = validate_value("redirect", post["redirect"])<br><br>post hash is populated with decoded values by parse_qs() function.<br><br>><br>> I think the real bug is that get_cookie() and set_cookie() are<br>> asymmetric, so we should just remove the url_decode() in get_cookie().<br><br><br>Well, that was my initial thought but secure_value() and validate_value() were those asymmetric.</div><div>get_cookie() and set_cookie() has nothing to do with it.<br><br>secure_value() shall encode values before for digesting by design to avoid clash<br>with user value "|" and one used by function as separator.<br><br>/v<br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jun 17, 2018 at 8:50 PM, John Keeping <span dir="ltr"><<a href="mailto:john@keeping.me.uk" target="_blank">john@keeping.me.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Did you mean to drop the list in this reply?<br>
<br>
I'm not very familiar with this Lua code, so someone else might have<br>
better insight.<br>
<span class="gmail-"><br>
On Sun, Jun 17, 2018 at 08:11:32PM +0300, Vlad wrote:<br>
> Unfortunatelly validate_value() is called with _decoded_ cookie:<br>
> <br>
> local username = validate_value("username", get_cookie(http["cookie"],<br>
> "cgitauth"))<br>
> ..<br>
> <br>
> because get_cookie() returns decoded values:<br>
> ..<br>
> function get_cookie (cookies, name)<br>
>   cookies = string.gsub(";" .. cookies .. ";", "%s*;%s*", ";")<br>
>   return url_decode(string.match(<wbr>cookies, ";" .. name .. "=(.-);"))<br>
> end<br>
> <br>
> <br>
> thus validate_value() will never validate for values/fields with e.g.<br>
> non-ascii characters.<br>
<br>
</span>Hmm... but the other caller of validate_value() does pass in an encoded<br>
value from a POSTed form field.<br>
<br>
I think the real bug is that get_cookie() and set_cookie() are<br>
asymmetric, so we should just remove the url_decode() in get_cookie().<br>
<div class="gmail-HOEnZb"><div class="gmail-h5"><br>
> On Sat, Jun 16, 2018 at 7:28 PM, John Keeping <<a href="mailto:john@keeping.me.uk">john@keeping.me.uk</a>> wrote:<br>
> <br>
> > On Thu, Apr 12, 2018 at 08:54:31PM +0300, <a href="mailto:thevlad@gmail.com">thevlad@gmail.com</a> wrote:<br>
> > > From: Vlad Safronov <<a href="mailto:thevlad@gmail.com">thevlad@gmail.com</a>><br>
> > ><br>
> > > Bugfix: Encode value and field before calculating cookie digest, the<br>
> > same way as secure_value() does<br>
> > > so validating will work correctly on encoded values.<br>
> ><br>
> > Missing sign-off (see [1] for what this means).<br>
> ><br>
> > [1] <a href="https://elinux.org/Developer_Certificate_Of_Origin" rel="noreferrer" target="_blank">https://elinux.org/Developer_<wbr>Certificate_Of_Origin</a><br>
> ><br>
> > However, I don't think this change is correct.  secure_value() places<br>
> > the encoded strings into the authstr, which is what validate_value() is<br>
> > reading, so field and value are already URL encoded here.<br>
> ><br>
> > This is why field is url_decode()'d later in this function before<br>
> > comparing with the expected value.<br>
> ><br>
> > > ---<br>
> > >  filters/simple-authentication.<wbr>lua | 2 ++<br>
> > >  1 file changed, 2 insertions(+)<br>
> > ><br>
> > > diff --git a/filters/simple-<wbr>authentication.lua b/filters/simple-<br>
> > authentication.lua<br>
> > > index de34d09..b40a819 100644<br>
> > > --- a/filters/simple-<wbr>authentication.lua<br>
> > > +++ b/filters/simple-<wbr>authentication.lua<br>
> > > @@ -230,6 +230,8 @@ function validate_value(expected_field, cookie)<br>
> > >               return nil<br>
> > >       end<br>
> > ><br>
> > > +     value = url_encode(value)<br>
> > > +     field = url_encode(field)<br>
> > >       -- Lua hashes strings, so these comparisons are time invariant.<br>
> > >       if hmac ~= crypto.hmac.digest("sha1", field .. "|" .. value .. "|"<br>
> > .. tostring(expiration) .. "|" .. salt, secret) then<br>
> > >               return nil<br>
> > > --<br>
> > > 2.17.0<br>
> ><br>
</div></div></blockquote></div><br></div></div></div>