View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0007755 | libmicrohttpd | digest authentication (HTTP) | public | 2023-03-08 10:13 | 2024-01-21 13:36 |
| Reporter | akermen | Assigned To | Christian Grothoff | ||
| Priority | urgent | Severity | major | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Product Version | 0.9.72 | ||||
| Summary | 0007755: Digest authentication nonce uniqueness | ||||
| Description | Digest authentication is working for simple requests bu fails for requests sent concurrently (almost all of if requests are send within a second). The issue appears to be “nonce” value of the authentication header being not unqiue for each (independent) request. This behavior seems to be not compliant with RFC2617 https://datatracker.ietf.org/doc/html/rfc2617#section-3.2.1 and RFC7616 https://datatracker.ietf.org/doc/html/rfc7616#section-3.2 both state “nonce" values should be uniquely generated each time a 401 response is made while the values generated by libmicrohttpd are only unique for each second (by the “MHD_monotonic_sec_counter" function). Digest authentication implementation for popular frameworks (for Flask from https://flask-httpauth.readthedocs.io/en/latest, for Node.js from https://www.npmjs.com/package/http-auth, for httbin from https://hub.docker.com/r/kennethreitz/httpbin) they all seem to produce unique “nonce” values and handle concurrent requests without any issue. This issue report is follow up from the mailing list thread "https://lists.gnu.org/archive/html/libmicrohttpd/2022-01/msg00000.html". | ||||
| Steps To Reproduce | Follow the steps at the "https://github.com/akermen/digest-nonce-test" repository that was specifically created for reproducing this issue. | ||||
| Additional Information | To make values unique, I experimented with basic "mutex wrapped counter mechanism" which appears to working for very limited test scope, but not sure about performance and effect on other parts of the library: /* global scope */ static int make_digest_unique = 1; static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER; /* at "MHD_queue_auth_fail_response" function */ /*…*/ pthread_mutex_lock(&mutex); make_digest_unique = make_digest_unique + 1; pthread_mutex_unlock(&mutex); /*…*/ calculate_nonce ((uint32_t) MHD_monotonic_sec_counter() + make_digest_unique, connection->method, connection->daemon->digest_auth_random, connection->daemon->digest_auth_rand_size, connection->url, realm, nonce); /*…*/ | ||||
| Tags | No tags attached. | ||||
|
|
I see the code is now exclusively using monotonic_msec_counter(), which is better, but not good enough IMO as it would seem to be monotonic but not *strictly* monotonic. Evgeny: I think we should introduce a MHD_strictly_monotonic_msec_counter(), internally there ensure that each value returned is larger than the previous (and possibly guard with a lock or use an atomic increment-and-return instruction) and use that strictly monotonic version for nonce computations. WDYT? |
|
|
This bug was the main reason for major rewrite of MHD authentication framework. The new version, which is currently in git master, should not have such bug. The new implementation uses several techniques: * by default (MHD_OPTION_DIGEST_AUTH_NONCE_BIND_TYPE is MHD_DAUTH_BIND_NONCE_NONE) client's address (the IP + port) is used together with monotonic_msec_counter() and other values to avoid clashes of generated nonces. Also realm is used for nonce generation. * if the new nonce is already in the list of generated nonces (or has conflict with another generated nonce in the same slot and another nonce is not yet expired) then weak random number is used to apply some small negative shift in the timestamp to generate new unique nonce. It is not ideal, but thinks to "retry" mechanism should work even if client is non-IP and for the client generating two requests on the same TCP/IP connection within a single millisecond for the same realm. The better solution would be a proper implementation of weak and strong random generators in MHD, which is already partially implemented in my private branches. See code for calculate_add_nonce_with_retry() and calculate_nonce(). A special test test_digestauth_concurrent was added in testcurl to check for this bug. |
|
|
If I not missed something, everything should work fine and the bug could be closed. |
|
|
We now keep a list of generated nonces!??? That seems very inefficient. And like we need to hold a lock for an undue long time. What would suffice is a strong PRNG once (and we could have the application pass such a seed value into our initialization) and then a strictly monotonic counter. That way, we'd not even need the current time *or* a list of previously used nonces. Just NONCE=HKDF(seed, ctr++). HKDF could be done trivially re-using the existing hash function logic. |
|
|
We keep generated nonces in hashed list. It is not possible to check for "nonce counter" ('nc') reuse without it. It was the same for the long time. The list is locked just for update, which is quick, no lock held for any long time. Monotonic counter is definitely possible now, with git master implementation with default settings. The current version with MHD_OPTION_DIGEST_AUTH_NONCE_BIND_TYPE set to anything except MHD_DAUTH_BIND_NONCE_NONE and the old version (behave like MHD_DAUTH_BIND_NONCE_URI) require nonce to be reproducible, so monotonic counter could not be used in such cases. In new version we should implement atomic variables with increment and decrement and with implementation by native atomic or fallback to non-atomic variable + lock. |
|
|
Ok, so we agree that we'll do it differently in the new version, but I think for now we can resolve this bug. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2023-03-08 10:13 | akermen | New Issue | |
| 2023-09-03 00:42 | Christian Grothoff | Note Added: 0020470 | |
| 2023-09-03 00:42 | Christian Grothoff | Priority | normal => urgent |
| 2023-09-03 00:42 | Christian Grothoff | Product Version | 0.9.75 => 0.9.72 |
| 2023-09-03 00:42 | Christian Grothoff | Assigned To | => Christian Grothoff |
| 2023-09-03 00:42 | Christian Grothoff | Status | new => confirmed |
| 2023-09-03 09:58 | Karlson2k | Note Added: 0020474 | |
| 2023-09-03 09:59 | Karlson2k | Note Added: 0020475 | |
| 2023-09-03 09:59 | Karlson2k | Note Edited: 0020475 | |
| 2023-09-03 11:47 | Christian Grothoff | Note Added: 0020479 | |
| 2023-09-03 19:16 | Karlson2k | Note Added: 0020489 | |
| 2024-01-21 13:35 | Christian Grothoff | Note Added: 0020940 | |
| 2024-01-21 13:35 | Christian Grothoff | Status | confirmed => resolved |
| 2024-01-21 13:35 | Christian Grothoff | Resolution | open => fixed |
| 2024-01-21 13:35 | Christian Grothoff | Fixed in Version | => 0.9.77 |
| 2024-01-21 13:36 | Christian Grothoff | Status | resolved => closed |
