From mboxrd@z Thu Jan 1 00:00:00 1970 From: thevlad at gmail.com (Vlad) Date: Sun, 17 Jun 2018 22:03:11 +0300 Subject: [PATCH] Encode value and field before calculating cookie digest, the same way secure_value() does In-Reply-To: <20180617175023.GM1922@john.keeping.me.uk> References: <20180412175431.33587-1-thevlad@gmail.com> <20180616162827.GE1922@john.keeping.me.uk> <20180617175023.GM1922@john.keeping.me.uk> Message-ID: 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 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 > wrote: > > > > > On Thu, Apr 12, 2018 at 08:54:31PM +0300, thevlad at gmail.com wrote: > > > > From: Vlad Safronov > > > > > > > > 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: