GNOME Bugzilla – Bug 794208
SoupAuthDomain doesn't work correctly with Digest authentication.
Last modified: 2018-03-17 08:38:35 UTC
SoupAuthDomain blocks all html links escaped, unescaped and utf with escape characters (even 'space' or any not English letter). Ex.SoupAuthDomain's path: "/" : https://192.168.1.10:1234/0/A%20B/ https://192.168.1.10:1234/0/A B/ http://192.168.1.10:1234/0/%CE%B5%CE%BB%CE%BB%CE%B1%CE%B4%CE%B1/ http://192.168.1.10:1234/0/ελλαδα/ ... Any of them work! even fully escaped links. Looks like uri.path is coming to SoupAuthDomain already unescaped and SoupAuthDomain doesn't understand if uri path covers SoupAuthDomain's path (EVEN "/" path doesn't work which means any links must be covered!!!) BTW without authorization both HTTP and HTTPS work well and support utf-8, escaped and not links. Maybe here is a method to fix/avoid it? I don't want to make a dictonary or db to make ascii links only. Thank you!
I have calculated it for digest auth. Looks like when server checks the digest response it's not using the uri from the request Digest in the 'Authorization' header! *WIKIPEDIA:* HA1=MD5(username:realm:password) HA2=MD5(method:digestURI) If the qop directive's value is "auth-int", then HA2 is HA2=MD5(method:digestURI:MD5(entityBody)) response=MD5(HA1:nonce:nonceCount:cnonce:qop:HA2) IF YOU WILL PICK URI FROM DIGEST ALL SHOULD WORK WELL! Basic authentication should work bcs it doesn't use uri in the encoding.
To be honest I don't understand what you're trying to say. Would you mind trying to be more clear in what you're explaining? Is this a problem with the server or the client-side of digest authentication? What exactly the problem is? Do you have a test case that can be used to test this?
> Looks like when server checks the digest response it's not using the uri from the request Digest in the 'Authorization' header! > IF YOU WILL PICK URI FROM DIGEST ALL SHOULD WORK WELL! FWIW, if you're talking about server-side digest authentication, I see that there is a check to see whether the URI in the Digest headers is the same as the URI in the SoupMessage (soup-auth-domain-digest.c, check_hex_urp()): req_uri = soup_message_get_uri (msg); dig_uri = soup_uri_new (uri); if (dig_uri) { if (!soup_uri_equal (dig_uri, req_uri)) { soup_uri_free (dig_uri); return FALSE; } soup_uri_free (dig_uri); If the URI is not the same, then the authentication is rejected. Is this what you say is causing issues?
(In reply to Claudio Saavedra from comment #2) > To be honest I don't understand what you're trying to say. Would you mind > trying to be more clear in what you're explaining? Is this a problem with > the server or the client-side of digest authentication? What exactly the > problem is? Do you have a test case that can be used to test this? I'm sorry for not clear explanation. It's a server side problem. (In reply to Claudio Saavedra from comment #3) > > Looks like when server checks the digest response it's not using the uri from the request Digest in the 'Authorization' header! > > > IF YOU WILL PICK URI FROM DIGEST ALL SHOULD WORK WELL! > > FWIW, if you're talking about server-side digest authentication, I see that > there is a check to see whether the URI in the Digest headers is the same as > the URI in the SoupMessage (soup-auth-domain-digest.c, check_hex_urp()): > > req_uri = soup_message_get_uri (msg); > dig_uri = soup_uri_new (uri); > if (dig_uri) { > if (!soup_uri_equal (dig_uri, req_uri)) { > soup_uri_free (dig_uri); > return FALSE; > } > soup_uri_free (dig_uri); > > If the URI is not the same, then the authentication is rejected. Is this > what you say is causing issues? Looks like all modern browsers (Firefox, Chrome tested) are always using 'escaped' uri paths in the digest auth. Maybe I have been escaping not in the same way on server side, I don't know but actually I'm trying to use just utf-8 encoding for uris by default and this works well everywhere including even China but not with Soup Digest Auth. For temporary fix I made a filter hook to manual checking if it was rejected through manual recalculating respond token from the digest. I'm not sure about this checking of uris in the digest bcs here is other protection methods in the digest: dynamic client cnonce counter - nc and uri encoded in response throgh the ha2. Later I could play with libsoup in jhbuild... Do you want me patch it? :)
Maybe it's a good idea to unescape uris before that checking at least to avoid client and server escaping differences?
Or soup_uri_equal could make unescaping too bcs https://192.168.1.10:1234/0/A%20B/ https://192.168.1.10:1234/0/A B/ are different now
(In reply to konkor from comment #6) > Or soup_uri_equal could make unescaping too bcs > > https://192.168.1.10:1234/0/A%20B/ > https://192.168.1.10:1234/0/A B/ > > are different now I tested it and this works. But I found next: 1) uri in the digest just relative path and query without scheme, host, user, password parts like: /0/a%20b/?foo=6732587&bar=2467029837 > dig_uri = soup_uri_new (uri); If I'm trying to make new uri from it I get null. Here is some data from the client request // WORKING REQUEST Referer: https://192.168.1.2:8088/6/ Authorization: Digest username="test", realm="test access", nonce="385902561520961116", uri="/6/ABCDE/", algorithm=MD5, response="4c16d5be4ad268a4b00fd2a4fd38c9e5", qop=auth, nc=0000000e, cnonce="9066d859a28b191a" Connection: keep-alive Upgrade-Insecure-Requests: 1 Cache-Control: max-age=0 msg uri: https://192.168.1.2:8088/6/ABCDE/ // NOT WORKING Host: 192.168.1.2:8088 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 Accept-Language: en-US,en;q=0.5 Accept-Encoding: gzip, deflate, br Referer: https://192.168.1.2:8088/6/ Authorization: Digest username="test", realm="test access", nonce="385902561520961116", uri="/6/a%20b%20c%20dir/", algorithm=MD5, response="b863c095605052bb7289046c358dc986", qop=auth, nc=00000015, cnonce="1220d65c686855c9" Connection: keep-alive Upgrade-Insecure-Requests: 1 msg uri: https://192.168.1.2:8088/6/a b c dir/
> Later I could play with libsoup in jhbuild... Do you want me patch it? :) If you want to work on a patch for this, please go ahead. Otherwise it would be helpful if you could write a test case.
> 1) uri in the digest just relative path and query without scheme, host, user, > password parts like: > /0/a%20b/?foo=6732587&bar=2467029837 >> dig_uri = soup_uri_new (uri); > If I'm trying to make new uri from it I get null. This is what the spec says about this: The "request-uri" value is the Request-URI from the request line as specified in section 5.1.2 of [2]. This may be "*", an "absoluteURL" or an "abs_path" as specified in section 5.1.2 of [2], but it MUST agree with the Request-URI. In particular, it MUST be an "absoluteURL" if the Request-URI is an "absoluteURL". So I think that expecting a full URI is probably wrong.
Sorry, I forgot about the second part of that code that I didn't copy/paste in comment 3. If the URI is NULL, then the 'else' part compares only the absolute paths.
Created attachment 369632 [details] [review] SoupAuthDomainDigest: Fix authentication with encoded uris If the client returns a absolute path instead of an URI in the digest, we need to normalize it before comparing with the message URI, otherwise we risk failing to authenticate.
Admittedly the above is a long shot, but if I understand correctly the logs you pasted, this might solve the issue. Would you mind testing it?
Review of attachment 369632 [details] [review]: ::: libsoup/soup-auth-domain-digest.c @@ +225,1 @@ g_free (req_path); g_free (dig_path);
NICE! But you have to use soup_uri_decode not soup_uri_normalize And you forget to free g_free (dig_path); in both cases!
Review of attachment 369632 [details] [review]: But you have to use soup_uri_decode not soup_uri_normalize And you forget to free g_free (dig_path); in both cases!
(In reply to konkor from comment #14) > NICE! I don't understand; does this mean that the patch works?
Created attachment 369637 [details] [review] SoupAuthDomainDigest: Fix Digest Authentication with encoded uris If the client returns a absolute path instead of an URI in the digest, we need to decode/unescape it before comparing with the message URI, otherwise we risk failing to authenticate. Firefox, Chrome browsers including mobile variants use just an absolute path and its query in the escaped form in the Digest Authentication. So Soup Servers with Digest Authentication unable to handle request from them if requests path contain some escaped characters. https://bugzilla.gnome.org/show_bug.cgi?id=794208
I have tested and built it with my server and debugging strings in the libsoup in jhbuild. it's not quick sorry.
You still haven't answered my question; did this patch solve the issue?
Created attachment 369650 [details] [review] SoupAuthDomainDigest: Fix Digest Authentication with encoded uris If the client returns a absolute path instead of an URI in the digest, we need to decode/unescape it before comparing with the message URI, otherwise we risk failing to authenticate. Firefox, Chrome browsers including mobile variants use just an absolute path and its query in the escaped form in a Digest Authentication. So Soup Servers with Digest Authentication unable to handle requests from them if requests paths contain some escaped/encoded characters. https://bugzilla.gnome.org/show_bug.cgi?id=794208
(In reply to Claudio Saavedra from comment #19) > You still haven't answered my question; did this patch solve the issue? No your patch didn't fix the issue. Take a look at my patch in the attachment, please. BTW Is there IRC channel for libsoup devs?
Review of attachment 369650 [details] [review]: ::: libsoup/soup-auth-domain-digest.c @@ +221,2 @@ + /* decoding both paths to be sure they have same encoding */ + req_path = soup_uri_decode (soup_uri_to_string (req_uri, TRUE)); This leaks the string uri.
> + req_path = soup_uri_decode (soup_uri_to_string (req_uri, TRUE)); > This leaks the string uri. Nice, I wanted to discuss about it. It's why I have commented it. I'll fix it now. I didn't code a lot on C last years. There is an other issue I found from tests. But I think I have to discuss about it in a separated thread. Actually, it's not clear behavior.
Created attachment 369677 [details] [review] SoupAuthDomainDigest: Fix Digest Authentication with encoded uris If the client returns a absolute path instead of an URI in the digest, we need to decode/unescape it before comparing with the message URI, otherwise we risk failing to authenticate. Firefox, Chrome browsers including mobile variants use just an absolute path and its query in the escaped form in a Digest Authentication. So Soup Servers with Digest Authentication unable to handle requests from them if requests paths contain some escaped/encoded characters. https://bugzilla.gnome.org/show_bug.cgi?id=794208
I think we need to stop stomping on each others shoes right now. I have been able to test this manually by connecting to a soup server locally with telnet and feeding it manually the Digest authentication. I found out that none of the patches above are going to work because later on the original URI, which was encoded, will be used to calculate the Digest response, which will be wrong. This is why I don't understand why you keep posting patches. You are not explaining whether they solve the issue for you or not in any of them, but I presume that they don't. It would be more helpful if you explained if the patch solves the problem -- or even better, if you could provide a simple test case that can be used to check this. In any case, I have a patch that, at least from what I tested, seems to solve the issue and I will clean it and upload it next.
Created attachment 369683 [details] [review] SoupAuthDomainDigest: Fix authentication with encoded uris If the client returns a absolute path instead of an URI in the digest, we need to normalize it before using it to calculate the digest response, otherwise we risk failing to authenticate.
1) Me neither 2) I'm going to use my internal patch anyway to able to work on all libsoup versions. I just wanted to contribute something to it for the future versions. 3) This patch is working because none of them > char *req_path_orig; > char *req_path; > char *dig_path; is using in a computing of the response. soup_auth_digest_compute_response (msg->method, uri, hex_a1, SOUP_AUTH_DIGEST_QOP_AUTH, nonce, cnonce, nonce_count, computed_response); It's why I talked about this checking early. I think it's no need to check it at all. 4) So I can't even find yet any client with FULL URI in a request digest and this part dig_uri = soup_uri_new (uri); is always NULL.
5) It needs a time to make even simplest server for testing... (In reply to Claudio Saavedra from comment #26) > Created attachment 369683 [details] [review] [review] > SoupAuthDomainDigest: Fix authentication with encoded uris > > If the client returns a absolute path instead of an URI in the digest, > we need to normalize it before using it to calculate the digest response, > otherwise we risk failing to authenticate. You don't need to change nothing it's calculating like expected. This uri checking is not even standard! The simplest patch is to remove it checking and additional work at all.
The patch in attachment 369683 [details] [review] solves the problem as I understand it. I would appreciate if you could test the patch and confirm this. > It's why I talked about this checking early. I think it's no need to check it > at all. We need to check because the specification is clear in that the digest uri and the request uri need to match. > 4) So I can't even find yet any client with FULL URI in a request digest and > this part dig_uri = soup_uri_new (uri); is always NULL. The specification indicates that the digest URI can be an absolute URI. Furthermore, the HTTP spec is clear in that absolute URIs are required when requests are being made by proxies and that for this reason the absolute URIs need to be supported. > This uri checking is not even standard! The simplest patch is to remove it > checking and additional work at all. This wouldn't suffice for the reasons I indicated above. Please test the patch?
> uri = dig_path = soup_uri_decode (uri); It's destroying all checking. You can't change 'uri' variable from request authentication because it's encoded in the other Digest tokens. No this is not working.
I'll try again. Is the patch not working because the URI is encoded at the time the Digest response is calculated in the client, therefore it needs to remain encoded when calculating the Digest response in the server?
> Is the patch not working because the URI is encoded at the time the Digest > response is calculated in the client, therefore it needs to remain encoded > when calculating the Digest response in the server? Exactly. If you remove uri = part it have to work. But there is a related issue it's why I'm decoding req_path too. I think how to explain it: 1) Soup Server path is always already unescaped in the request message. 2) Soup URI from soup_message_get_uri (msg) is always unescaped too. 3) But soup_uri_new (uri); is always escaped by default. 1) and 2) could make server handling or forwarding messages for proxies for example. 3) It's okay but it's not always the same escaping/encoding with other clients and platforms.
> 3) But soup_uri_new (uri); is always escaped by default. soup_uri_new() always normalizes the given URI. So when the uri is /Foo/A%20B, once it is made into a SoupURI you'll get "/Foo/A B". So there is no need to decode req_path, I think.
I don't know it needs more info. I just noticed to an example: /Foo/A | B/ - decoded uri /Foo/A%20|%20B/ - from soup_uri_new Firefox /Foo/A%20%7C%20B/ - from Chrome 3 different URIs for 1 thing /Foo/A | B/ And it's just one example
Created attachment 369689 [details] [review] SoupAuthDomainDigest: Fix authentication with encoded uris When the client is using absolute paths for Digest authentication, we need to make sure that the digest uri is not encoded before comparing it to the request uri, as some clients might provide them encoded. Also modify server-auth-test.c to make this problem reproducible and add a couple of test cases to make sure we don't regress.
I'm fairly confident that this solves the problem. If you only apply the changes to the test, you will see the failure you reported originally. I'd still like to get someone to review this, Dan, Michael, anyone?
Review of attachment 369689 [details] [review]: Looks sane to me. ::: tests/server-auth-test.c @@ +176,3 @@ + /* 5. Digest auth with a mixture of encoded and decoded URI. See #794208. + */ + do_test (base_uri, "/Digest/A%20|%20B", I guess the | is a character that would normally be encoded? But that's a normal ASCII character... I would also try with a Unicode-only character like ௹.
I added another test case with the character Michael mentioned above and pushed.
Review of attachment 369677 [details] [review]: Thanks for your help with this bug, please let us know if there are still issues.