Bug #3112
closedmod_auth cache password doesn't match
Description
In lighttpd 1.4.59 password cache lookup succeeds for some users, and fails for others
It looks like http_auth_const_time_memeq_pad
(http_auth.c:84) gets the correct strings and lengths, but it compares 1 byte too many (a byte that could be \0 or something else residing after the password in memory).
for (size_t i = 0, j = 0; lim; --lim) {
diff |= (av[i] ^ bv[j]);
i += (i < alen); // <-- -1
j += (j < blen); // <-- -1
}
alen-1 and blen-1 fixed the issue for me.
In later lighttpd versions this code is moved to ck_memeq_const_time
in ck.c .
Updated by gstrauss about 3 years ago
- Category changed from core to mod_auth
- Status changed from New to Patch Pending
- Target version changed from 1.4.xx to 1.4.61
Thank you for digging into this.
http_auth_const_time_memeq_pad()
, and subsequently ck_memeq_const_time()
, expect the strings passed to have '\0' after the len, and this byte is what is compared during the "constant work" extension.
In mod_auth.c:mod_auth_check_basic()
, the user[]
buffer is '\0' terminated.
However, when storing the value in the cache in mod_auth.c:http_auth_cache_entry_init()
, the '\0' terminator may be missing. This may affect HTTP Basic Auth, but does not affect HTTP Digest Auth.
Please try this patch instead of your proposed patch:
--- a/src/mod_auth.c +++ b/src/mod_auth.c @@ -63,7 +63,7 @@ http_auth_cache_entry_init (const struct http_auth_require_t * const require, co *(store pointer to http_auth_require_t, which is persistent * and will be different for each realm + permissions combo)*/ http_auth_cache_entry * const ae = - malloc(sizeof(http_auth_cache_entry) + ulen + pwlen); + malloc(sizeof(http_auth_cache_entry) + ulen + pwlen+1); force_assert(ae); ae->require = require; ae->ctime = log_monotonic_secs; @@ -74,6 +74,7 @@ http_auth_cache_entry_init (const struct http_auth_require_t * const require, co ae->pwdigest = ae->username + ulen; memcpy(ae->username, username, ulen); memcpy(ae->pwdigest, pw, pwlen); + ae->pwdigest[pwlen] = '\0'; return ae; }
Updated by gstrauss about 3 years ago
- Status changed from Patch Pending to Fixed
Applied in changeset b1d1202af8217d3565b862289e1f3d50a633452b.
Updated by manfred about 3 years ago
Tested the patch and it indeed fixes the issue. Thanks!
I certainly agree with fixing the problem 'at the source', it might however be worth considering changing the http_auth_const_time_memeq_pad
/ ck_memeq_const_time
function implementation or its prototype. From a function that takes an arbitraty void* blob of size_t len I personally wouldn't expect it to (try to) use len+1 . Having it iterate up to len would make it more consistent with for instance http_auth_const_time_memeq
, or the functions referred in the comments such as timingsafe_bcmp
, consttime_memequal
, wouldn't it?
Updated by gstrauss about 3 years ago
Yes, I agree that the current interface fails to meet the principle of least surprise. I'll consider a change for the future.
Updated by gstrauss about 3 years ago
See my dev branch for a timing-safe patch to ck_memeq_const_time() which does not require an initialized byte at a[alen]
or b[blen]
. The patch also handles 0-length strings.
https://git.lighttpd.net/lighttpd/lighttpd1.4/commit/7845c2a0706307804ed4c86bd17602f97ae32905
Also available in: Atom