View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002059 | libmicrohttpd | digest authentication (HTTP) | public | 2012-01-05 15:46 | 2012-01-23 14:21 |
| Reporter | tclaveirole | Assigned To | Christian Grothoff | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Product Version | 0.9.17 | ||||
| Target Version | 0.9.18 | Fixed in Version | 0.9.18 | ||
| Summary | 0002059: digest authentication fails with GET arguments | ||||
| Description | Hello, I just found a bug in libmicrohttpd when using digest authentication and URIs that include GET arguments. Here is a test case, an explanation, and a fix. | ||||
| Steps To Reproduce | The attached test.c file shows the bug. Its code is almost a straight copy from libmicrohttpd's documentation (for the ahc_echo() function) and tutorial (for the main() function). The only differences are I added a fprintf() at the beginning of ahc_echo(), a few includes, and changed the layout a bit. When compiled, this test works properly for any URI without GET arguments (using digest authentication). When one sends a request with GET arguments, however, the authentication fails forever (MHD_digest_auth_check() returns MHD_INVALID_NONCE). At some point the server keep responding 401 errors that include stale=true in the WWW-Authenticate header and therefore the client tries to re-authenticate again. This ends up in an "infinite" loop of requests and 401 responses until the client timeouts. | ||||
| Additional Information | * Explanation MHD_queue_auth_fail_response() uses connection->url to compute the server nonce (calling calculate_nonce(), line 608 of digestauth.c). This string does not include GET arguments. Now, MHD_digest_auth_check() checks the client-returned server nonce was generated by libmicrohttpd (digestauth.c, line 523 to 541). However, it does that by using the URI from the WWW-Authenticate header to compute the "reference" server nonce to compare to, and this URI includes GET arguments. Therefore, this test can never be true and MHD_digest_auth_check() ought to fail on URIs that include GET arguments. * Fix Attached is a patch to fix this bug. The simple solution is to compute the "reference" server nonce in MHD_digest_auth_check() using connection->url instead of the WWW-Authenticate given URI. After a quick examination I do not think this breaks RFC 2617 or this is a security issue. However, I am no expert. Note that another fix could be to compute the server nonce in MHD_queue_auth_fail_response() using an URI that includes GET arguments. I do not know which solution is best. Anyway, I do not know libmicrohttpd's internals enough to implement this second fix. | ||||
| Tags | No tags attached. | ||||
| Attached Files | |||||
|
|
I think this is mostly fine, except that it breaks checking that the URI we got on the server side matches at all the URI that the client believes he is getting. We can improve this a bit like this (SVN : $ svn diff Index: digestauth.c =================================================================== --- digestauth.c (revision 19007) +++ digestauth.c (working copy) @@ -518,13 +518,16 @@ * exceeds `nonce_timeout' then the nonce is * invalid. */ - if (t > nonce_time + nonce_timeout) + if ( (t > nonce_time + nonce_timeout) || + (0 != strncmp (uri, + connection->url, + strlen (connection->url))) ) return MHD_INVALID_NONCE; calculate_nonce (nonce_time, connection->method, connection->daemon->digest_auth_random, connection->daemon->digest_auth_rand_size, - uri, + connection->url, realm, noncehashexp); /* Still, now some MiM could exchange the URI's query arguments for others. So the user authenticates for http://foo.bar/a?q=test and a MiM could substitute http://foo.bar/a?q=evil and the server would still accept the response. So an attacker would only be prevented by digest authentication from visiting http://foo.bar/b with my variation (whereas yours would allow an attacker to access anything on http://foo.bar/ after stealing digest credentials -- at least until the nonce changes). I'm not sure either is fully acceptable; however the second case could be documented and is at least cheap to implement. Reconstructing the full original URI at this time would be eh, much more difficult. |
|
|
SVN 19262 contains a patch that keeps the full check that the original URI and the URI provided in the authentication header are the same. It does a *lot* more work to do this, but avoids 1) having to keep around a copy of the original URI (as digest authentication is uncommon, the extra memory requirement would be bad) 2) allowing the same authentication values to be used for a URI with different arguments This is done by parsing the URI from the authentication header and check that each of the key-value pairs matches those we have in the connection structure (and checking that they also have the same total number). |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2012-01-05 15:46 | tclaveirole | New Issue | |
| 2012-01-05 15:46 | tclaveirole | File Added: test+patch.tar | |
| 2012-01-05 22:02 | Christian Grothoff | Note Added: 0005245 | |
| 2012-01-05 22:02 | Christian Grothoff | Assigned To | => Christian Grothoff |
| 2012-01-05 22:02 | Christian Grothoff | Status | new => feedback |
| 2012-01-05 22:03 | Christian Grothoff | Target Version | => 0.9.18 |
| 2012-01-19 17:43 | Christian Grothoff | Note Added: 0005295 | |
| 2012-01-19 17:43 | Christian Grothoff | Status | feedback => resolved |
| 2012-01-19 17:43 | Christian Grothoff | Fixed in Version | => 0.9.18 |
| 2012-01-19 17:43 | Christian Grothoff | Resolution | open => fixed |
| 2012-01-23 14:21 | Christian Grothoff | Status | resolved => closed |
| 2013-05-06 12:52 | Christian Grothoff | Category | digest authentication => digest authentication (HTTP) |
