Closed Bug 814117 Opened 7 years ago Closed 7 years ago

CORS checks need to be able to tell whether the response had multiple Access-Control-Allow-Origin headers

Categories

(Core :: Networking: HTTP, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: bzbarsky, Assigned: jduell.mcbugs)

References

Details

(Whiteboard: [CORS-testsuite])

Attachments

(1 file)

Specifically, this is OK:

  Access-Control-Allow-Origin: foo, bar

But this should cause the CORS check to fail:

  Access-Control-Allow-Origin: foo
  Access-Control-Allow-Origin: bar

One option is just failing HTTP responses which have this header duplicated completely, either always or in a way CORS consumers can opt in to.

In particular, this needs to happen even in cases like this:

  Access-Control-Allow-Origin: foo
  Access-Control-Allow-Origin:

or

  Access-Control-Allow-Origin:
  Access-Control-Allow-Origin: foo

in which client code is currently not even told about the second, empty, header.
Attached patch v1Splinter Review
This fails HTTP responses which have this header duplicated.

Am I right to assume that we want the same treatment for Access-Control-Max-Age, Access-Control-Allow-Credentials, and possibly Access-Control-Allow-{Methods|Headers}?  Let me know and I can add them as well if needed.
Assignee: nobody → jduell.mcbugs
Attachment #684219 - Flags: review?(bzbarsky)
Huh?? Isn't CORS contradicting HTTP semantics here? Sounds like a bug in the CORS spec to me?
Jonas, We already have such semantics violations in other cases, for security, though admittedly for non-list headers.

For list headers like Access-Control-Allow-Origin one could argue for not having the violation, but that does make you vulnerable to header injection...  I'll let you and Anne fight out the spec issue.  If you think this is a bug in the spec (and testsuite) please say so on the mailing list before that Dec 6 deadline, ok?

Jason, per current spec the desired behavior for those other headers is:

Access-Control-Max-Age: "If there is no such header, there is more than one such header, or parsing failed, let max-age be a value at the discretion of the user agent (zero is allowed)."

Access-Control-Allow-Credentials: "If the omit credentials flag is unset and the response includes zero or more than one Access-Control-Allow-Credentials header values, return fail and terminate this algorithm."

Access-Control-Allow-Methods: Multiple headers are explicitly allowed, with the lists concatenated.

Access-Control-Allow-Headers: Same thing.
Comment on attachment 684219 [details] [diff] [review]
v1

The big worry here is compat: if non-CORS resources are sending a broken version of this header, we'll suddenly stop working with them.  But I guess we can try this as a first cut...
Attachment #684219 - Flags: review?(bzbarsky) → review+
The HTTPbis spec says: "Multiple header fields with the same field name MUST NOT be sent in a message unless the entire field value for that header field is defined as a comma-separated list [i.e., #(values)].  Multiple header fields with the same field name can be combined into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field value to the combined field value in order, separated by a comma."

BTW, what does "can" mean in a IETF spec? 

The Access-Control-Allow-Origin header field isn't a multi-valued list header. Its syntax is a space-separated list of origins:

   origin-list         = serialized-origin *( SP serialized-origin )
   serialized-origin   = scheme "://" host [ ":" port ]

One option is to simply fold the header into a single multi-valued comma-separate value, and rely on the parser of this header to reject it because of the commas.

See also 474845, about handling empty values.

Probably we should make a change to nsIHttpChannel like this:

   typedef uint32_t MultipleValueRule;
   const MultipleValueRule TAKE_FIRST_WHEN_MULTIPLE = 1;
   const MultipleValueRule FAIL_WHEN_MULTIPLE = 2;
   const MultipleValueRule FOLD_MULTIPLE = 3;
   ACString getResponseHeaderSafe(in ACString header, MultipleValueRule rule);

   /* Use getResponseHeaderSafe instead of this. */
   [deprecated] ACString getResponseHeader(in ACString header);

and switch all callers in our codebase to use getResponseHeaderSafe.
Access-Control-Allow-Origin is not a list-based header. It only contains a single origin or *.
Which means that any time multiple headers exist we should be combining them into a value which couldn't be a valid origin, and so the request should be automatically denied.
I think this bug is WFM or INVALID .
> Access-Control-Allow-Origin is not a list-based header.

Ah, ok.  I thought it was a list for some reason.  

> Which means that any time multiple headers exist we should be combining them into a
> value which couldn't be a valid origin

Necko doesn't do that when one of the values is empty; the empty value is just ignored.  So this:

  Access-Control-Allow-Origin: foo
  Access-Control-Allow-Origin:

leads to "Access-Control-Allow-Origin: foo", not "Access-Control-Allow-Origin: foo,".  The test suite actually tests both this case and this one:

  Access-Control-Allow-Origin: *
  Access-Control-Allow-Origin:

which we treat like "Access-Control-Allow-Origin: *".
(In reply to Boris Zbarsky (:bz) from comment #8)
> Necko doesn't do that when one of the values is empty; the empty value is
> just ignored.  So this:
> 
>   Access-Control-Allow-Origin: foo
>   Access-Control-Allow-Origin:
> 
> leads to "Access-Control-Allow-Origin: foo", not
> "Access-Control-Allow-Origin: foo,".  The test suite actually tests both
> this case and this one:
> 
>   Access-Control-Allow-Origin: *
>   Access-Control-Allow-Origin:
> 
> which we treat like "Access-Control-Allow-Origin: *".

I guess I don't care strongly about these. It only is a security problem if the server sends an empty header and an injection inserts a valid header. Arguably that isn't any different form the server not sending any header and someone injects a valid header.

It sounds like the HTTP spec is quite fine with forbidding duplicate headers for non-list headers though so maybe the attached fix is the right one?
So we can either handle this in the HTTP layer, which allows us to also detect the (harmless?) blank dupe header case, or we can fail in XHR (and not detect the blank dupe case).  Opinions?   If I don't hear otherwise I guess I'll land this in the next day or two.
> Access-Control-Max-Age: "If there is no such header, there is more than one
> such header, or parsing failed, let max-age be a value at the discretion of
> the user agent (zero is allowed)."

This patch can't help with this: it fails the whole request.  XHR will have to handle this (with help eventually from bug 669259 to allow it to know about empty headers in the response).

> Access-Control-Allow-Credentials: "If the omit credentials flag is unset and
> the response includes zero or more than one Access-Control-Allow-Credentials
> header values, return fail and terminate this algorithm."

Also needs to be handled at least partly in XHR, though we could barf when seeing multiple Access-Control-Allow-Credentials to handle that part.
Comment on attachment 684219 [details] [diff] [review]
v1

Review of attachment 684219 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/test/unit/test_duplicate_headers.js
@@ +614,5 @@
> +  response.write("HTTP/1.0 200 OK\r\n");
> +  response.write("Content-Type: text/plain\r\n");
> +  response.write("Content-Length: 30\r\n");
> +  response.write("Access-Control-Allow-Origin: www.mozilla.org\r\n");
> +  response.write("Access-Control-Allow-Origin: www.evil.net\r\n");

We should have a test case just like this one, but where one the first header field is blank, and another one where the last header field is blank.

@@ +622,5 @@
> +}
> +
> +function completeTest21(request, data, ctx)
> +{
> +  do_check_eq(request.status, Components.results.NS_ERROR_CORRUPTED_CONTENT);

We should test that get*Header returns a distinct error code that can be used to distinguish the case of a duplicated single-value header, so that we know that the XHR code that will be written on top of this will work.
> We should have a test case just like this one, but where one the first
> header field is blank, and another one where the last header field is blank.

We already have that coverage for those variants for other headers from nsHttpHeaderArray::IsSuspectDuplicateHeader (content-length in particular).

> We should test that get*Header returns a distinct error code that can be 
> used to distinguish the case of a duplicated single-value header

Do we need something besides NS_ERROR_CORRUPTED_CONTENT?  We essentially introduced that code for this case (and a handful of others: bogus content-lengths, etc.).  If we do I'm happy to add it.
(In reply to Jason Duell (:jduell) from comment #10)
> So we can either handle this in the HTTP layer, which allows us to also
> detect the (harmless?) blank dupe header case, or we can fail in XHR (and
> not detect the blank dupe case).  Opinions?

http://www.youtube.com/watch?v=ussCHoQttyQ

(In reply to Jason Duell (:jduell) from comment #11)
> > Access-Control-Max-Age: "If there is no such header, there is more than one
> > such header, or parsing failed, let max-age be a value at the discretion of
> > the user agent (zero is allowed)."
> 
> This patch can't help with this

I think we should handle this one entirely in the XHR code. I.e. the http code should treat this just like any duplicate header. If there are two non-empty headers that means that we'll just get a comma separated value which isn't a valid integer which I think means that we'll treat it as 0. If either of the headers is empty we'll just use the other one.

That all sounds fine to me.
Jonas, are you going to take up getting the spec and CORS test suite changed accordingly then?
The spec seems fine, no? The spec says that we can do whatever we want if there are multiple Access-Control-Max-Age headers, so surely whatever we're doing is conformant.

But yes, I can pursue getting the tests changed.
Oh, sorry.  For Max-Age, I agree, we can just do the easy thing.
> So we can either handle this in the HTTP layer... or we can fail in XHR (and
> not detect the blank dupe case).

OK, let's do it this way then. 

   https://hg.mozilla.org/integration/mozilla-inbound/rev/e63a025943eb
https://hg.mozilla.org/mozilla-central/rev/e63a025943eb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 850468
It would have been useful to pop an error onto the console with a more specific description than ‘Corrupted Content’ page for a CORS header issue.
Depends on: 846314
Note: we're backing this out for breaking too many sites in bug 845273. Work will continue at CORS level in bug 847533.

That makes this logically WONTFIX but I'm leaving as FIXED since it landed at one point, to make branch tracking easier--correct me if that's the wrong thing to do here.
Resolution: FIXED → WONTFIX
Target Milestone: mozilla20 → ---
You need to log in before you can comment on or make changes to this bug.