Project

General

Profile

Actions
Copy link

Bug #3112

closed

mod_auth cache password doesn't match

Added by manfred over 2 years ago. Updated over 2 years ago.

Status:
Fixed
Priority:
Normal
Category:
mod_auth
Target version:
1.4.61
ASK QUESTIONS IN Forums:
No

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 .

Actions
Copy link
 #1

Updated by gstrauss over 2 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;
 }

Actions
Copy link
 #2

Updated by gstrauss over 2 years ago

  • Status changed from Patch Pending to Fixed
Actions
Copy link
 #3

Updated by manfred over 2 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?

Actions
Copy link
 #4

Updated by gstrauss over 2 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.

Actions
Copy link
 #5

Updated by gstrauss over 2 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

Actions
Copy link

Also available in: Atom