Fixes #2023: incorrect URI parsing caused Digest auth failure.

[Geoffrey-A]

No description provided.

[s-ludwig]

Thanks for the pull request! I have two questions:
Just to make sure, does the incorrect parsing just happen if the URI contains "=", or is there any other case that I'm missing? At which kind of input is the stripLeft() targeted?

It appears that there are a few more issues in this function that I'd later target in a separate PR.

[Geoffrey-A]

Just to make sure, does the incorrect parsing just happen if the URI contains "=", or is there any other case that I'm missing?

Yes that is correct.

At which kind of input is the stripLeft() targeted?

Each parameter is usually separated by a comma and whitespaces: https://en.wikipedia.org/wiki/Digest_access_authentication#Example_with_explanation So param would contain e.g. ` uri="/?a=b"` 2018-02-10 6:49 GMT+13:00 Sönke Ludwig <[email protected]>:

…

Thanks for the pull request! I have two questions: Just to make sure, does the incorrect parsing just happen if the URI contains "=", or is there any other case that I'm missing? At which kind of input is the stripLeft() targeted? It appears that there are a few more issues in this function that I'd later target in a separate PR. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[Geoffrey-A]

It appears that there are a few more issues in this function that I'd later target in a separate PR.

Are these causing incorrect authentication success/failure? 2018-02-10 6:49 GMT+13:00 Sönke Ludwig <[email protected]>:

…

Thanks for the pull request! I have two questions: Just to make sure, does the incorrect parsing just happen if the URI contains "=", or is there any other case that I'm missing? At which kind of input is the stripLeft() targeted? It appears that there are a few more issues in this function that I'd later target in a separate PR. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[s-ludwig]

Are these causing incorrect authentication success/failure?

I think I mostly had parameters without quotes in mind, as well as quoted strings that contain a comma, so since those would lead to wrong parameter value contents they could indeed lead to failures.

[Geoffrey-A]

I think I mostly had parameters without quotes in mind

According to the RFC2069, and RFC2617 which supersedes the former, all parameter values are quoted strings, so we are good here [1, 2].

By the way, I noticed that we send the stale parameter value unquoted; it should be either "true" or "false" [1,5].

as well as quoted strings that contain a comma,

Commas however are a problem indeed, since they could be present in the field domain, and in other fields added by the RFC2617 (such as qop, which we unfortunately do not support). Also, the realm value is, I feel, not clearly defined [1,3], and I think we should expect any character in it, including equal signs and commas; the only character that should not appear in quoted strings is the double quote [4]. I confirmed that firefox escapes " into %22 inside a quoted value.
Therefore I would tend to think that the only character we can split on safely is the double quote.

[1] https://tools.ietf.org/html/rfc2069#page-4
[2] https://tools.ietf.org/html/rfc2617#page-3
[3] https://tools.ietf.org/html/rfc2617#page-4
[4] https://tools.ietf.org/html/rfc2068#page-17
[5] https://tools.ietf.org/html/rfc2617#page-8

[s-ludwig]

According to the RFC2069, and RFC2617 which supersedes the former, all parameter values are quoted strings, so we are good here [1, 2].

There are some fields that are not, for example nonce-count and qop-value. However, quotes are still sometimes accepted, and are important for the validation result (the use of unq(X) in the RFC).

Therefore I would tend to think that the only character we can split on safely is the double quote.

Even that is problematic, because double-quotes can be escaped as \" (see for example https://greenbytes.de/tech/webdav/draft-ietf-httpbis-p1-messaging-16.html#rfc.section.3.2.3). So the process should probably be like this:

1. Find the next equals sign, use the text before that (stripped) as the key
2. See if a quotation mark follows
   1. If yes, parse a quoted string and enforce that a comma (or EOS) follows (with possible WS in-between)
   2. If not, search for the next comma and use that as the delimiter for an unquoted string - we may or may not choose to enforce a quoted string for certain fields here

Parsing the quoted string would then:

1. Skip the leading double quote
2. In a loop search for the next '\' or '"', whichever comes first. In the former case, skip the backslash and the following byte, in the latter case skip the quotation mark and return the resulting string without start/end quotes. Failure to find any of the two raises an exception.

[Geoffrey-A]

There are some fields that are not, for example nonce-count and qop-value.

Indeed, nonce-count must not be quoted it seems. I think qop-value is only a nonterminate symbol of the grammar whose only purpose is to define the terminal qop-options, which corresponds to the qop parameter, whose value is quoted [3].

However I am surprised to see the example from wikipedia with an unquoted qop value from the client [1]. Checking the RFC again, the above is for the WWW-Authenticate header sent by the server. The client responds with an Authorization header, which has a qop parameter value defined as qop-value, which is not redefined in that paragraph [4]. So I understand that qop’s value is not quoted when sent by the client.

Indeed, again.

Note that with the current implementation we should not receive these two parameters, since they are sent only when the server sends a qop. So the current quote handling may not be incorrect. The comma’s and equal sign’s definitely are.
Still, supporting the unquoted values is a necessary condition for supporting the RFC2617 and its qop feature.

However, quotes are still sometimes accepted, and are important for the validation result (the use of unq(X) in the RFC).

Do you mean that quotes could be either specified or omitted for the value of one same given parameter? I have not been able to find that in the RFC.

What do you mean by important? I do not think unq() should validate anything. I understand that unq() is the identity function for unquoted strings. It is applied once in the RFC to the non-quoted qop-value [2].

Even that is problematic, because double-quotes can be escaped as \" (see for example https://greenbytes.de/tech/webdav/draft-ietf-httpbis-p1-messaging-16.html#rfc.section.3.2.3).

I found that firefox does not escape nor encode the backslash in the uri value. It seems to me that if the double quote is to be escaped by a backslash, so should be the backslash itself.

If we allow \" incorrectly, we would fail to parse the whole header for cases where one quoted parameter value ends by an unescaped backslash.

By the way, this document has expired in Feb 2012.
It looks quite complete, so it would be nice and convenient to find it as an accepted and valid RFC document on the IETF site.

[1] https://en.wikipedia.org/wiki/Digest_access_authentication#Example_with_explanation
[2] https://tools.ietf.org/html/rfc2617#section-3.2.2.1
[3] https://tools.ietf.org/html/rfc2617#section-3.2.1
[4] https://tools.ietf.org/html/rfc2617#section-3.2.2

[s-ludwig]

I think qop-value is only a nonterminate symbol of the grammar whose only purpose is to define the terminal qop-options, which corresponds to the qop parameter, whose value is quoted [3].

You are right, I read that too quickly.

What do you mean by important?

Important just in the sense that if something with the quote removal isn't implemented right, the authentication validation result will be wrong. If we handle both possibilities, this will be done correctly more or less automatically though, so the statement was more or less redundant.

I found that firefox does not escape nor encode the backslash in the uri value. It seems to me that if the double quote is to be escaped by a backslash, so should be the backslash itself.

If we allow " incorrectly, we would fail to parse the whole header for cases where one quoted parameter value ends by an unescaped backslash.

By the way, this document has expired in Feb 2012.
It looks quite complete, so it would be nice and convenient to find it as an accepted and valid RFC document on the IETF site.

The quoted-pair is listed identically in the latest HTTP/1.1 RFC: https://tools.ietf.org/html/rfc7230#section-3.2.6

Would be interesting to see whether Firefox actually escapes a trailing backslash, and which encoding it uses for " characters in the URI.

[Geoffrey-A]

Important just in the sense that if something with the quote removal isn't implemented right, the authentication validation result will be wrong.

Yeah, a.k.a slicing blindly.

If we handle both possibilities, this will be done correctly more or less automatically though, so the statement was more or less redundant.

Understood.

The quoted-pair is listed identically in the latest HTTP/1.1 RFC: https://tools.ietf.org/html/rfc7230#section-3.2.6

Thanks!

Would be interesting to see whether Firefox actually escapes a trailing backslash,

It does not, so we really get the string \" at the end.

and which encoding it uses for " characters in the URI.

Percent encoding: %22.

---

I realized that "string" in the RFC grammar means string. When quotes are specified, they are indicated as <">.

According to the RFC2069, and RFC2617 which supersedes the former, all parameter values are quoted strings, so we are good here [1, 2].

That was incorrect, even with the RFC2069 alone. The algorithm parameter value is unquoted. [1]

So the current quote handling may not be incorrect.

I have checked the other fields we currently support. Only algorithm is incorrectly parsed; it should be parsed as an unquoted string.

However I am surprised to see the example from wikipedia with an unquoted qop value from the client [1].

This is actually the example from the RFC2617 [2].

By the way, I noticed that we send the stale parameter value unquoted; it should be either "true" or "false" [1,5].

That is incorrect too [1]. We send it right.

[1] https://tools.ietf.org/html/rfc2069#page-4
[2] https://tools.ietf.org/html/rfc2617#section-3.5

[Geoffrey-A]

I found that Edge and Chrome also percent-encode the double quote.

Edge also does not escape nor encode the backslash.

Chrome escapes the backslash.
I think we have a problem now.

[Geoffrey-A]

Commas however are a problem indeed, since they could be present in the field domain

This field is sent by the server, not the client.

---

I think there are several issues at hand:

1. sending any HTTP parameter via GET causes the digest auth to fail
2. having an equal sign in any parameter value will cause auth failure; apart from the case 1, it is when realm, username or the uri (such as in path or parameters values) contain an =. Other supported fields should not have an that symbol.
3. having a comma in any parameter value will cause auth failure; that is when realm, username, or the uri contain a ,.
4. how to deal with the double quote and backslash parsing (and also percent-decoding which we currently do not perform)
   (5. implement RFC2617 and the quality of protection (qop) feature?)

Case 1 is very common, and I think it should be fixed as soon as possible. Other cases are less common, and may involve research time to find a solution (case 4).
This PR fixes the case 1, and I can trivially adapt it to fix case 2.

I think the whole parsing could be reworked in a following PR. By the way, such parsing of field lists seems to be common when handling HTTP headers, so it might be better done as a function independent from this module so that it is reusable.

What do you think?

[tchaloupka]

Related PR: #1931
I've rebased it to see if tests passes now

[s-ludwig]

I agree that we can split this up into an immediate fix and a full fix. A reusable function for this may also be useful, although obviously it requires extra-care to account for possibly differing specification details.

Regarding percent-decoding, I don't think that we should do that. The string between the quotes is used verbatim when computing the digest, so fortunately at least there we don't have to do any workarounds. For the quoted-pair handling, I'd definitely prefer adhering to the standard. But we could do something like allowing the sequence \", (or \"<EOS>) to terminate a parameter to minimize the potential for incompatibility with the (apparently) buggy implementation of Firefox and Edge.

@tchaloupka: I thought that the PR was merged a long time ago, that's definitely a drawback of the auto-merge feature. We should definitely merge that first and apply the changes afterwards.

[Geoffrey-A]

Sorry for the delayed reply.

I agree that we can split this up into an immediate fix and a full fix.

Too bad that it did not make into 0.8.3; hopefully this immediate fix will be included in the next release.

I extended the fix to cover case 2 in addition to case 1. That is, there should be no failure caused by an equal sign = anymore.
I confirmed indeed that the auth was broken when the username or the realm contained a sign equal, and that it is now fixed.

Regarding percent-decoding, I don't think that we should do that. The string between the quotes is used verbatim when computing the digest, so fortunately at least there we don't have to do any workarounds. For the quoted-pair handling, I'd definitely prefer adhering to the standard. But we could do something like allowing the sequence ", (or ") to terminate a parameter to minimize the potential for incompatibility with the (apparently) buggy implementation of Firefox and Edge.

Yes, I agree.

We should definitely merge that first and apply the changes afterwards.

Note that the linked PR did not overlap in feature nor in code with this one.