After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 794208 - SoupAuthDomain doesn't work correctly with Digest authentication.
SoupAuthDomain doesn't work correctly with Digest authentication.
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: API
unspecified
Other Linux
: Normal critical
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on:
Blocks:
 
 
Reported: 2018-03-09 15:42 UTC by konkor
Modified: 2018-03-17 08:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
SoupAuthDomainDigest: Fix authentication with encoded uris (1.22 KB, patch)
2018-03-13 18:10 UTC, Claudio Saavedra
none Details | Review
SoupAuthDomainDigest: Fix Digest Authentication with encoded uris (693 bytes, patch)
2018-03-13 21:20 UTC, konkor
none Details | Review
SoupAuthDomainDigest: Fix Digest Authentication with encoded uris (849 bytes, patch)
2018-03-14 10:10 UTC, konkor
none Details | Review
SoupAuthDomainDigest: Fix Digest Authentication with encoded uris (942 bytes, patch)
2018-03-14 14:08 UTC, konkor
rejected Details | Review
SoupAuthDomainDigest: Fix authentication with encoded uris (1.77 KB, patch)
2018-03-14 15:22 UTC, Claudio Saavedra
none Details | Review
SoupAuthDomainDigest: Fix authentication with encoded uris (3.53 KB, patch)
2018-03-14 17:57 UTC, Claudio Saavedra
accepted-commit_now Details | Review

Description konkor 2018-03-09 15:42:04 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!
Comment 1 konkor 2018-03-09 17:43:11 UTC
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.
Comment 2 Claudio Saavedra 2018-03-13 15:04:32 UTC
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?
Comment 3 Claudio Saavedra 2018-03-13 15:10:27 UTC
> 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?
Comment 4 konkor 2018-03-13 16:20:06 UTC
(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? :)
Comment 5 konkor 2018-03-13 16:27:37 UTC
Maybe it's a good idea to unescape uris before that checking at least to avoid client and server escaping differences?
Comment 6 konkor 2018-03-13 16:32:44 UTC
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
Comment 7 konkor 2018-03-13 17:25:29 UTC
(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/
Comment 8 Claudio Saavedra 2018-03-13 17:43:19 UTC
> 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.
Comment 9 Claudio Saavedra 2018-03-13 17:50:24 UTC
> 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.
Comment 10 Claudio Saavedra 2018-03-13 17:54:25 UTC
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.
Comment 11 Claudio Saavedra 2018-03-13 18:10:16 UTC
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.
Comment 12 Claudio Saavedra 2018-03-13 18:11:08 UTC
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?
Comment 13 Claudio Saavedra 2018-03-13 18:12:30 UTC
Review of attachment 369632 [details] [review]:

::: libsoup/soup-auth-domain-digest.c
@@ +225,1 @@
 			g_free (req_path);

g_free (dig_path);
Comment 14 konkor 2018-03-13 18:38:37 UTC
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!
Comment 15 konkor 2018-03-13 18:47:50 UTC
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!
Comment 16 Claudio Saavedra 2018-03-13 20:01:06 UTC
(In reply to konkor from comment #14)
> NICE!

I don't understand; does this mean that the patch works?
Comment 17 konkor 2018-03-13 21:20:57 UTC
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
Comment 18 konkor 2018-03-13 21:30:35 UTC
I have tested and built it with my server and debugging strings in the libsoup in jhbuild. it's not quick sorry.
Comment 19 Claudio Saavedra 2018-03-14 09:18:49 UTC
You still haven't answered my question; did this patch solve the issue?
Comment 20 konkor 2018-03-14 10:10:26 UTC
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
Comment 21 konkor 2018-03-14 10:17:49 UTC
(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?
Comment 22 Claudio Saavedra 2018-03-14 13:41:38 UTC
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.
Comment 23 konkor 2018-03-14 13:57:44 UTC
> +		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.
Comment 24 konkor 2018-03-14 14:08:40 UTC
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
Comment 25 Claudio Saavedra 2018-03-14 14:36:10 UTC
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.
Comment 26 Claudio Saavedra 2018-03-14 15:22:50 UTC
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.
Comment 27 konkor 2018-03-14 15:24:25 UTC
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.
Comment 28 konkor 2018-03-14 15:31:01 UTC
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.
Comment 29 Claudio Saavedra 2018-03-14 15:39:46 UTC
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?
Comment 30 konkor 2018-03-14 15:53:03 UTC
> 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.
Comment 31 Claudio Saavedra 2018-03-14 15:58:04 UTC
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?
Comment 32 konkor 2018-03-14 16:27:54 UTC
> 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.
Comment 33 Claudio Saavedra 2018-03-14 16:45:17 UTC
> 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.
Comment 34 konkor 2018-03-14 17:14:27 UTC
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
Comment 35 Claudio Saavedra 2018-03-14 17:57:20 UTC
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.
Comment 36 Claudio Saavedra 2018-03-14 18:04:47 UTC
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?
Comment 37 Michael Catanzaro 2018-03-16 14:56:20 UTC
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 ௹.
Comment 38 Claudio Saavedra 2018-03-17 08:37:36 UTC
I added another test case with the character Michael mentioned above and pushed.
Comment 39 Claudio Saavedra 2018-03-17 08:38:35 UTC
Review of attachment 369677 [details] [review]:

Thanks for your help with this bug, please let us know if there are still issues.