View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0007755libmicrohttpddigest authentication (HTTP)public2023-03-08 10:132023-09-03 19:16
Reporterakermen Assigned ToChristian Grothoff  
PriorityurgentSeveritymajorReproducibilityalways
Status confirmedResolutionopen 
Product Version0.9.72 
Summary0007755: Digest authentication nonce uniqueness
DescriptionDigest 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 ReproduceFollow the steps at the "https://github.com/akermen/digest-nonce-test" repository that was specifically created for reproducing this issue.
Additional InformationTo 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);
/*…*/
TagsNo tags attached.

Activities

Christian Grothoff

2023-09-03 00:42

manager   ~0020470

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?

Karlson2k

2023-09-03 09:58

developer   ~0020474

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.

Karlson2k

2023-09-03 09:59

developer   ~0020475

Last edited: 2023-09-03 09:59

 If I not missed something, everything should work fine and the bug could be closed.

Christian Grothoff

2023-09-03 11:47

manager   ~0020479

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.

Karlson2k

2023-09-03 19:16

developer   ~0020489

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.

Issue History

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