Closed Bug 1687364 Opened 3 years ago Closed 9 months ago

CORS Authorization Mishandling

Categories

(Core :: DOM: Networking, defect, P2)

Firefox 86
defect

Tracking

()

RESOLVED FIXED
115 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox85 --- wontfix
firefox86 --- wontfix
firefox87 --- wontfix
firefox115 --- fixed

People

(Reporter: steven.hartz, Assigned: kershaw)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, sec-moderate, Whiteboard: [necko-triaged][secdom:spec], [wptsync upstream][adv-main115+])

Attachments

(6 files)

Attached file test-case.tar

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:83.0) Gecko/20100101 Firefox/83.0

Steps to reproduce:

  1. Extract echo.py and test.html from the tarball
  2. Run echo.py, which will start a simple echo server on http://localhost:8081
  3. Open test.html in a new tab
  4. Open the developer tools Network tab
  5. Click the "Send Request" button
  6. Observe that the XHR is sent with the Authorization header in the Network tab
  7. Switch to the Console tab. Observe that JavaScript can access the application's response and log it in the Console.

HTTP Traffic:

CORS Pre-flight Request:
OPTIONS / HTTP/1.1
Host: localhost:8081
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:83.0) Gecko/20100101 Firefox/83.0
Accept: /
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Access-Control-Request-Method: POST
Access-Control-Request-Headers: authorization,content-type
Origin: null
Connection: close

CORS Pre-flight Response:
HTTP/1.0 200 OK
Server: BaseHTTP/0.6 Python/3.8.5
Date: Mon, 18 Jan 2021 19:17:09 GMT
Content-type: text/html
Access-Control-Allow-Origin: *
Access-Control-Allow-Headers: *

=========
CORS Request:
POST / HTTP/1.1
Host: localhost:8081
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:83.0) Gecko/20100101 Firefox/83.0
Accept: /
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate
Content-Type: application/json
Authorization: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c
Content-Length: 31
Origin: null
Connection: close

{
"some_json": "contents"
}

CORS Response:
HTTP/1.0 200 OK
Server: BaseHTTP/0.6 Python/3.8.5
Date: Mon, 18 Jan 2021 19:17:09 GMT
Content-type: text/html
Access-Control-Allow-Origin: *
Access-Control-Allow-Headers: *

POST request for /

Actual results:

The Authorization header with a JSON Web Token (JWT) is sent on an XmlHttpRequest (XHR) without Access-Control-Allow-Credentials, without xhr.withCredentials=true, and without Authorization explicitly listed in Access-Control-Allow-Headers.

Expected results:

Authorization should not be sent when ACAH is wildcard (*):
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers#directives

Credentialed request should not be sent when ACAO is wildcard (*)
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin#directives

Credentialed requests should not be sent without Access-Control-Allow-Credentials, nor should be sent when the XHR object's withCredentials property has not been set to true:

  1. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Credentials
  2. https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/withCredentials
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM: Networking
Product: Firefox → Core

This was implemented in bug 1309358 but looking at the patches there I don't see the behavior that is warranted for the Authorization header. Unfortunately there is no test coverage for that either that I can see.

Kershaw, can you take a look?

Flags: needinfo?(kershaw)
See Also: → 1309358

I believe this is sec-moderate, looking at our severity ratings page, but open to be convinced otherwise.

Keywords: sec-moderate

So, I take a look at our implementation of CORS check. The only thing we missed is about Access-Control-Allow-Headers. The problem is at this line, which allow all headers when the header value of Access-Control-Allow-Headers is *. We should add a check to Authorization header.

About Access-Control-Allow-Origin and Access-Control-Allow-Credentials, I think our [implementation]((https://searchfox.org/mozilla-central/rev/4dac9993b609fccc87e82682614faf2a44cda306/netwerk/protocol/http/nsCORSListenerProxy.cpp#567-601) is correct. When Access-Control-Allow-Origin is *, credentialed request are not allowed.

Assignee: nobody → kershaw
Severity: -- → S3
Flags: needinfo?(kershaw)
Priority: -- → P2
Whiteboard: [necko-triaged]

Hi Anne,

I have a question about Access-Control-Expose-Headers. It seems the spec doesn't mention that Authorization header can't be wildcarded, but mdn does.
I think mdn is not correct, right? Or do I interpret the spec wrongly?

Thanks.

Flags: needinfo?(annevk)

Yeah, -Expose-Headers does not handle it and I don't think it should as Authorization is a request header. I'll add dev-doc-needed so that can be corrected. It's probably worth adding a test for though.

Flags: needinfo?(annevk)
Keywords: dev-doc-needed

Authorization header can't be wildcarded for Access-Control-Allow-Headers r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/4ffa919d35f99eb2120e255da5a6d35b7672e26b

Group: dom-core-security → core-security-release
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch

Is this something we need to consider for backport?

Flags: needinfo?(kershaw)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Is this something we need to consider for backport?

I'm inclined not to uplift this for the following reasons.

  1. This is not a sec-high bug. However, I am not sure what vulnerability could be exploited by sending 'authorization' header to the server.
    @freddy, what do you think?
  2. This patch could break some websites, like bug 1691824. Not sure if this is a good idea to uplift to ESR 78.
Flags: needinfo?(kershaw) → needinfo?(fbraun)

Steven, did you also file this bug against Chrome and Safari? Could you copy me on those bugs?

Flags: needinfo?(steven.hartz)

(In reply to Kershaw Chang [:kershaw] from comment #10)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Is this something we need to consider for backport?

I'm inclined not to uplift this for the following reasons.

  1. This is not a sec-high bug. However, I am not sure what vulnerability could be exploited by sending 'authorization' header to the server.
    @freddy, what do you think?
  2. This patch could break some websites, like bug 1691824. Not sure if this is a good idea to uplift to ESR 78.

+1 for uplifting. Especially if this affects other browsers (which is a bit unclear to me. looking at comment 11).
Does it apply cleanly to ESR 78? If so, please see if you can uplift.

Flags: needinfo?(fbraun)

I don't think we should uplift this until we know more about the situation in other browsers. It seems like they don't enforce this either so uplifting would be best avoided in that case I think and we might even have to back this out if they don't plan on enforcing this around the same time. (I'm trying to find out.)

Thanks.

I created https://github.com/whatwg/fetch/security/advisories/GHSA-6rrj-jgxg-wf55 so we can discuss this with other browsers, who indeed do not seem to implement this. Contact me with your GitHub ID if you want to be added.

Chrome folks informed me there did not seem to be a bug reported against them so with all the information I have now I would recommend that we do not ship fix until we have more information.

(In reply to Anne (:annevk) from comment #15)

I created https://github.com/whatwg/fetch/security/advisories/GHSA-6rrj-jgxg-wf55 so we can discuss this with other browsers, who indeed do not seem to implement this. Contact me with your GitHub ID if you want to be added.

Chrome folks informed me there did not seem to be a bug reported against them so with all the information I have now I would recommend that we do not ship fix until we have more information.

Thanks. I'll request to back this out.

Backed out changeset 4ffa919d35f9 (Bug 1687364) as requested by kershaw.

Backout link: https://hg.mozilla.org/integration/autoland/rev/20982512637917c0a73129f8eb99cafbadff9901

Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Target Milestone: 87 Branch → ---
Blocks: 1691824

(In reply to Anne (:annevk) from comment #11)

Steven, did you also file this bug against Chrome and Safari? Could you copy me on those bugs?

I have not done so yet, but it looks like you've already discovered that. Do you think I still should?

Flags: needinfo?(steven.hartz)

You could, but if you do please do mention that this is an issue in all browsers.

Reported to Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1176753
Reported to Safari, no link to share.

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:kershaw, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(kershaw)
Flags: needinfo?(dd.mozilla)

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #22)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:kershaw, could you have a look please?
For more information, please visit auto_nag documentation.

The patch was backed out.

Flags: needinfo?(kershaw)
Flags: needinfo?(dd.mozilla)

Do you know the consensus with the other browsers? Chrome's issue is still hidden, and there's no link for Safari.

Flags: needinfo?(annevk)

The Chromium bug now links to a whatwg github discussion I do not have access to. Link: https://github.com/whatwg/fetch/security/advisories/GHSA-6rrj-jgxg-wf55.

Additionally, they added a use counter for this, so they could determine impact: https://chromium.googlesource.com/chromium/src/+/2dfc61141303d4c4633a1366b8e34e32aefec614

I haven't seen any other updates on the Chromium bug tracker, but I'm not sure what is happening in the WHATWG discussion which may be relevant.

Chrome folks gathered some telemetry (see https://www.chromestatus.com/metrics/feature/timeline/popularity/3873), but otherwise nothing changed. I asked them about next steps.

Flags: needinfo?(annevk)

As a follow-up, Chrome said they will try to deprecate this behavior and stop supporting it, despite the relatively high usage. I suggest we closely monitor that and follow once they ship (or try to ship around the same time).

Whiteboard: [necko-triaged] → [necko-triaged][secdom:spec]
Status: REOPENED → ASSIGNED

I think this is public information at this point and the planned cross-browser deprecation would benefit from giving this issue more visibility. If noone objects, I suggest we make this bug public in the following week (week of May 22nd 2023).

Can we aim to land a patch with a console warning (and ideally Telemetry?) as a first step here? Ideally, this should happen soon.

Flags: needinfo?(kershaw)

(In reply to Frederik Braun [:freddy] from comment #28)

I think this is public information at this point and the planned cross-browser deprecation would benefit from giving this issue more visibility. If noone objects, I suggest we make this bug public in the following week (week of May 22nd 2023).

Can we aim to land a patch with a console warning (and ideally Telemetry?) as a first step here? Ideally, this should happen soon.

Sure. I am working on this.

Flags: needinfo?(kershaw)
Attachment #9199030 - Attachment description: Bug 1687364 - Authorization header can't be wildcarded for Access-Control-Allow-Headers → Bug 1687364 - Authorization header can't be wildcarded for Access-Control-Allow-Headers, r=#necko

Depends on D102932

Attachment #9334179 - Flags: data-review?
Attachment #9334179 - Flags: data-review? → data-review?(chutten)

Before I begin with the data review, is there a process by which this bug will become public when this data collection ships? Data Review must be done in the open, so if this bug must remain closed while data collection happens, we'll need a separate open bug for the review.

Flags: needinfo?(kershaw)

(In reply to Chris H-C :chutten from comment #33)

Before I begin with the data review, is there a process by which this bug will become public when this data collection ships? Data Review must be done in the open, so if this bug must remain closed while data collection happens, we'll need a separate open bug for the review.

Yes, please see comment #28. This bug will be public soon.

Flags: needinfo?(kershaw)

Comment on attachment 9334179 [details]
bug1687364_data_review.txt

PRELIMINARY NOTE:
Since this collection is Glean, you might find ./mach data-review <bug number> to be of particular use to help generate your Data Collection Review Request for future requests.

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection can be controlled through Firefox's Preferences.

If the request is for permanent data collection, is there someone who will monitor the data over time?

No. This collection will expire in Firefox 130.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

Is the data collection request for default-on or default-off?

Default on for all channels.

Does the instrumentation include the addition of any new identifiers?

No.

Is the data collection covered by the existing Firefox privacy notice?

Yes.

Does the data collection use a third-party collection tool?

No.


Result: datareview+

Attachment #9334179 - Flags: data-review?(chutten) → data-review+
Group: core-security-release

Unhiding because the issue is public in fetch, and we're running into webcompat trying to fix it (as is Chrome) so clearly people need to know this is changing.

Pushed by kjang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95f1e52ddf84
Authorization header can't be wildcarded for Access-Control-Allow-Headers, r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/04c0e2ee9aa8
Tests, r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/ff16d0fa8ed8
Telemetry probe for the outcomes of Authorization header, r=freddyb,necko-reviewers,valentin
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/40230 for changes under testing/web-platform/tests
Whiteboard: [necko-triaged][secdom:spec] → [necko-triaged][secdom:spec], [wptsync upstream]
Status: ASSIGNED → RESOLVED
Closed: 3 years ago9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 115 Branch
Upstream PR merged by moz-wptsync-bot

FF115 MDN docs for this can be tracked in https://github.com/mdn/content/issues/27230

Can you please confirm my understanding:

  1. A server sends Access-Control-Allow-Headers in a response to a preflight request to indicate which non-CORS-safelisted headers it is OK to be included in the real request.

    • If used with a wildcard * mdn says any of the headers may be send in the real request except authorization, which must be explicitly specified.
    • However FF has been allowing Authorization in the actual response even if Access-Control-Allow-Headers is wildcard (*)
    • So:
      • this fix enforces the rule that Authorization must be included in the server response header, behind a pref network.cors_preflight.authorization_covered_by_wildcard which is default off.
    • The intent would then be to turn this on at some point? For now this is an experimental feature.
    • right now no one obeys the rule - right?
  2. In a comment c4 above it sounds like there is another bug with MDN docs.

    • Specifically Access-Control-Expose-Headers indicates also mentions that Authorization header can't be wildcarded.
    • The bug is that this makes no sense - Access-Control-Expose-Headers indicates the response headers that can be exposed to user code, while Authorization is a request header. So that comments should probably be stripped out.

Is that all correct?

Flags: needinfo?(kershaw)

(In reply to Hamish Willee from comment #41)

FF115 MDN docs for this can be tracked in https://github.com/mdn/content/issues/27230

Can you please confirm my understanding:

  1. A server sends Access-Control-Allow-Headers in a response to a preflight request to indicate which non-CORS-safelisted headers it is OK to be included in the real request.

    • If used with a wildcard * mdn says any of the headers may be send in the real request except authorization, which must be explicitly specified.
    • However FF has been allowing Authorization in the actual response even if Access-Control-Allow-Headers is wildcard (*)
    • So:
      • this fix enforces the rule that Authorization must be included in the server response header, behind a pref network.cors_preflight.authorization_covered_by_wildcard which is default off.
    • The intent would then be to turn this on at some point? For now this is an experimental feature.
    • right now no one obeys the rule - right?
  2. In a comment c4 above it sounds like there is another bug with MDN docs.

    • Specifically Access-Control-Expose-Headers indicates also mentions that Authorization header can't be wildcarded.
    • The bug is that this makes no sense - Access-Control-Expose-Headers indicates the response headers that can be exposed to user code, while Authorization is a request header. So that comments should probably be stripped out.

Is that all correct?

Yes, that's all correct.
Thanks!

Flags: needinfo?(kershaw)
Flags: behind-pref+
Whiteboard: [necko-triaged][secdom:spec], [wptsync upstream] → [necko-triaged][secdom:spec], [wptsync upstream][adv-main115+]
Attached file advisory.txt
Alias: CVE-2023-37204

Please note that the pref network.cors_preflight.authorization_covered_by_wildcard is still true in Firefox 115, which means that Authorization header is still covered by wildcard.
I've filed bug 1841019 for turning it off, but the timeline is still unknown.

Blocks: 1841019
Alias: CVE-2023-37204
You need to log in before you can comment on or make changes to this bug.