669675 - failure to skip unknown HTTP authentication schemes in WWW-Authenticate

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P5)

Description

[Julian Reschke]

See <http://greenbytes.de/tech/tc/httpauth/#multibasicunknown2>. This generates

  WWW-Authenticate: Newauth realm="newauth", Basic realm="basic"

The recipient should skip the unknown scheme, and pick "Basic" instead. It does work when the order is reversed.

Comment 1

[Julian Reschke]

It appears that the right thing happens if the server sends the two challenges in separate headers:

  WWW-Authenticate: Newauth realm="newauth"
  WWW-Authenticate: Basic realm="basic"

However, this is not supposed to make a difference.

Comment 2

[Daniel Stenberg [:bagder]]

The reason for this treatment seems to be what the comment for nsHttpHeaderArray::MergeHeader() says:

   // Special case these headers and use a newline delimiter to
   // delimit the values from one another as commas may appear
   // in the values of these headers contrary to what the spec says.

The Digest auth being a perfect example of this. Skipping an unknown auth with comma separation is therefore a really nasty task.

Also, I've never seen a live server out there send WWW-Authenticate: headers merged into one, they seem to always send separate lines. Possibly because of the same reason.

Comment 3

[Julian Reschke]

(In reply to Daniel Stenberg [:bagder] from comment #2)
> The reason for this treatment seems to be what the comment for
> nsHttpHeaderArray::MergeHeader() says:
> 
>    // Special case these headers and use a newline delimiter to
>    // delimit the values from one another as commas may appear
>    // in the values of these headers contrary to what the spec says.

Do we have an example of a case like this? (Reminder: Safari and Konqueror get this right).

> The Digest auth being a perfect example of this. Skipping an unknown auth
> with comma separation is therefore a really nasty task.
> 
> Also, I've never seen a live server out there send WWW-Authenticate: headers
> merged into one, they seem to always send separate lines. Possibly because
> of the same reason.

It would be awesome if we could find out whether this is just due to browser bugs, or because it really *needs* to be special-cased (such as with Set-Cookie).

Comment 4

[Frank]

(In reply to Julian Reschke from comment #3)
> Do we have an example of a case like this? (Reminder: Safari and Konqueror
> get this right).

Safari probably gets this right, because Apple uses it. I found this bug, since I am trying to use the iCloud CardDAV server as addressbook in Thunderbird (via SOGo Connector). The Apple server responds with this:

WWW-Authenticate: X-MobileMe-AuthToken realm="Newcastle", Basic realm="Newcastle"

Unfortunately, due to the X-MobileMe-AuthToken scheme, Thunderbird never opens a password dialog.

I can reproduce this with an ownCloud installation. A normal ownCloud installation can be used as addressbook without any problems, but as soon as you insert something like 'Demo realm="Test"' into the WWW-Authenticate header, Thunderbird doesn't bring up the password dialog anymore.

The same problem exists in Firefox aswell.

Comment 5

[Geert Hoekstra]

I stumbled upon the same issue using Sogo to connect with Apple's contacts server.

RFC7235 says:
 User agents are advised to take special care in parsing the field
   value, as it might contain more than one challenge, and each
   challenge can contain a comma-separated list of authentication
   parameters.  Furthermore, the header field itself can occur multiple
   times.

   For instance:

     WWW-Authenticate: Newauth realm="apps", type=1,
                       title="Login to \"apps\"", Basic realm="simple"

   This header field contains two challenges; one for the "Newauth"
   scheme with a realm value of "apps", and two additional parameters
   "type" and "title", and another one for the "Basic" scheme with a
   realm value of "simple".

      Note: The challenge grammar production uses the list syntax as
      well.  Therefore, a sequence of comma, whitespace, and comma can
      be considered either as applying to the preceding challenge, or to
      be an empty entry in the list of challenges.  In practice, this
      ambiguity does not affect the semantics of the header field value
      and thus is harmless.

So the logic in Mozilla should probably check whether the a comma is followed by a name=value pair (in which case it is a parameter belonging to the preceeding challenge), or it is followed by a single token and a space (in which case a new challenge starts)

Comment 6

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 8

[Boris Zbarsky [:bzbarsky]]

This is causing real-life compat problems, per the dev-platform thread with subject "nsIHttpChannel not trying to authenticate if presented BASIC and an unknown auth method".  We should re-evaluate the priority.

Comment 9

[Honza Bambas (:mayhemer)]

This needs a broader discussion and checking on how other browsers behave in various scenarios.

Comment 10

[Selena Deckelmann :selenamarie :selena]

Junior - can you investigate how Chrome behaves in this situation and report back?

Comment 11

[Junior [inactive]]

Firefox and Chrome behaves the same as Comment 0.

Comment 12

[gstrauss]

Patch submitted for review: https://phabricator.services.mozilla.com/D106241
Related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=472823 SHA 256 Digest Authentication
Related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=281851 CVE-2005-2395 Wrong scheme used when server offers both Basic and Digest auth

Comment 13

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 669675 - Use Tokenizer in ParseRealm r=#necko

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42561f42313d
Use Tokenizer in ParseRealm r=necko-reviewers,dragana

Comment 15

[Alexandru Michis [:malexandru]]

Backed out 14 changesets (Bug 1705659, Bug 472823, Bug 669675) for causing bustages in nsHttpChannelAuthProvider.cpp
Backout link: https://hg.mozilla.org/integration/autoland/rev/a9431e068bcd2dc86778d9b12bc7775850e2736b
Push with failures, failure log.

(Update): Also caused mochitest plain failures.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d920aa17a468
Use Tokenizer in ParseRealm r=necko-reviewers,dragana

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/d920aa17a468

Comment 18

[Julian Reschke]

I'm testing with 90.0a1 (2021-05-27) and the problem still persists...

Comment 19

[Valentin Gosu [:valentin] (he/him)]

We intentionally stopped displaying realm info in bug 1694418.
I'll take a look at the still failing test cases - I've converted them into unit tests, and those pass, so there's likely some other issue happening. I might spawn off another bug.

Comment 20

[Cristian Brindusan [:cbrindusan]]

Backed out 14 changesets (Bug 1705659, Bug 472823, Bug 669675) as requested by valentin for causing regressions.
https://hg.mozilla.org/integration/autoland/rev/4207821cb4e016141be3c4b331f1ede47c90c0b7

Comment 21

[Julien Cristau [:jcristau]]

beta backout: https://hg.mozilla.org/releases/mozilla-beta/rev/0830f6eb4b5e3a19d2e5f49213e8b531909443f2

Comment 22

[Iulian Moraru]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/4207821cb4e0

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e92759eacf36
Use Tokenizer in ParseRealm r=necko-reviewers,dragana

Comment 24

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/e92759eacf36