Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safelist Last-Event-ID #568

Open
annevk opened this issue Jul 20, 2017 · 22 comments
Open

Safelist Last-Event-ID #568

annevk opened this issue Jul 20, 2017 · 22 comments
Labels
security/privacy There are security or privacy implications topic: cors

Comments

@annevk
Copy link
Member

annevk commented Jul 20, 2017

Per whatwg/html#689 it can contain any header value. And it seems that a page with a cooperating server can send that header anywhere due to redirects.

Basically, use EventSource to open a connection. Get the server to set the ID. Then trigger a reconnection. Then when the server gets the header, redirect that request to wherever. That request to wherever will include the Last-Event-ID header with a value that can be controlled by the server that did the redirect.

Given that this has been the case for forever, we might as well enshrine it. (Should probably write a test to confirm though.)

@annevk annevk added the security/privacy There are security or privacy implications label Jul 20, 2017
@annevk
Copy link
Member Author

annevk commented Sep 4, 2017

Chrome definitely leaks Last-Event-ID as per the above description. I need a better tool to know whether Firefox or Safari does it as neither debugger shows much information.

@annevk
Copy link
Member Author

annevk commented Sep 4, 2017

Confirmed that Firefox and Safari do it though using Charles Proxy. We should just safelist the header.

annevk added a commit that referenced this issue Sep 4, 2017
It turns out that you can set the Last-Event-ID request header to arbitrary values and get it across origins. That seems like sufficient reason to safelist it and hopefully make it clear to server administrators to pay extra attention to this header.

Tests: ...

Fixes #568.
@annevk
Copy link
Member Author

annevk commented Apr 12, 2018

@whatwg/security thoughts?

@annevk
Copy link
Member Author

annevk commented Jan 19, 2021

@johnwilander @horo-t I'm no longer convinced that safelisting this is correct as it would allow for the kind of values that might result in script execution on the server and they could potentially bypass the length checks we now have in place. Do you have thoughts on how to fix this?

@horo-t
Copy link

horo-t commented Jan 20, 2021

Sorry, I'm not familiar with EventSource. @yutakahirano knows more than me.

@annevk
Copy link
Member Author

annevk commented Nov 24, 2021

@johnwilander @yutakahirano ping.

@yutakahirano
Copy link
Member

and they could potentially bypass the length checks we now have in place

Can you elaborate on this a bit more?

@annevk
Copy link
Member Author

annevk commented Nov 25, 2021

@yutakahirano create a same-origin server-sent events endpoint. Generate a very large ID and then end the stream. Upon seeing the very large ID, redirect to the victim server.

@yutakahirano
Copy link
Member

If we safelist last-event-id, values larger than 128 octets will be rejected by https://fetch.spec.whatwg.org/#cors-safelisted-request-header. Do you think that's enough?

@annevk
Copy link
Member Author

annevk commented Nov 25, 2021

The other problem is that it can contain essentially any header value byte, which historically has been an attack vector (and is why we have restrictions on the other headers now).

We'd have to apply the same restrictions as with accept.

@yutakahirano
Copy link
Member

Sounds good. Then what we need to do is to check how many site would be broken by such a change, right?

@annevk
Copy link
Member Author

annevk commented Nov 26, 2021

Yeah.

@yutakahirano
Copy link
Member

yutakahirano commented Nov 26, 2021

FWIW I think I can help such an investigation. I'm not sure I'll have enough time to ship the change in the near future because we'll need to talk with enterprise people to actually ship this kind of change.

@yutakahirano
Copy link
Member

FWIW I think I can help such an investigation. I'm not sure I'll have enough time to ship the change in the near future because we'll need to talk with enterprise people to actually ship this kind of change.

@yoichio is working on this.

@yoichio
Copy link
Contributor

yoichio commented Jun 16, 2022

Hi, I got a statistics.
The Chrome UMA shows usage is 0.00020% and Blink API owner, or @yoavweiss said "the low usage suggests that the intent may not get a lot of pushback, assuming other browsers are planned to follow."

So I'd say the change looks safe in terms of compatibility risk.

@annevk
Copy link
Member Author

annevk commented Jun 16, 2022

That is great news! Here is what we need to do specification-wise:

  1. Update https://fetch.spec.whatwg.org/#cors-safelisted-request-header to list last-event-id right below accept so it ends up sharing the same algorithm.
  2. Update corresponding tests.
  3. Add a comment to Last-Event-ID value space follow-up html#7363 indicating that HTML should probably also document that certain values result in a CORS preflight. (Fixing that issue would be a nice stretch goal.)

I suspect that all implementers are on board aligning here given that it currently represents a hole in our same-origin policy protections.

cc @youennf @KershawChang

@rexxars
Copy link

rexxars commented Nov 16, 2024

Is there any movement on this, or ways that I as a non-browser developer can help move this along? Some environments do not have a native EventSource implementation, and polyfilling these with a fetch implementation currently triggers CORS preflights when a last event ID is sent, which not all servers have safelisted.

@annevk
Copy link
Member Author

annevk commented Nov 18, 2024

None of the outlined steps involve browser engineering. It involves making a change to the standard defined in this repository and corresponding test changes in https://github.com/web-platform-tests/wpt. This is some work, but it should be relatively straightforward. If you get stuck somewhere feel free to reach out on https://whatwg.org/chat.

rexxars added a commit to rexxars/fetch that referenced this issue Nov 19, 2024
Since EventSource implementations in most environments already send this header
without CORS preflight request, it makes sense to make it a safelisted header.

Fixes whatwg#568
rexxars added a commit to rexxars/wpt that referenced this issue Nov 19, 2024
The EventSource API does _not_ run any preflight request when specifying
the `Last-Event-ID` header, and so `fetch` requests should also allow
this header to be set manually.

See whatwg/fetch#568 for more information
rexxars added a commit to rexxars/fetch that referenced this issue Nov 19, 2024
Since EventSource implementations in most environments already send this header
without CORS preflight request, it makes sense to make it a safelisted header.

See whatwg#568
@rexxars
Copy link

rexxars commented Nov 19, 2024

In whatwg/html#689 the discussion seemed to land on allowing UTF-8 bytes except for 0x00, 0x0A, and 0x0D. This issue is about safelisting the header for cross-origin requests, but applying restrictions to it beyond what was discussed in whatwg/html#689. In essence, a "safe" last-event-id header value going cross-origin needs to:

Questions:

  1. Are we intending to change the HTML EventSource specification to have the same restrictions as the CORS-safelisted header rules? Or will same-origin EventSources have less restrictions?

  2. What is the intended behavior when an EventSource server responds with an ID that does not conform to the safelisted header restrictions?

    A. Ignore the value?
    B. Dispatch an error and close?
    C. Use the value if same-origin but fail the request if cross-origin?

@yoichio
Copy link
Contributor

yoichio commented Nov 20, 2024

Add @ricea, or current network owner.
I also bit consider the trend is growing(0.0008%~)
https://chromestatus.com/metrics/feature/timeline/popularity/4167

@annevk
Copy link
Member Author

annevk commented Nov 20, 2024

  1. EventSource should only impose these restrictions when CORS applies. All of that should automatically follow from it invoking fetch.
  2. I think the answer here is D. HTML invokes fetch as per usual and what happens follows from that. (You'd get a CORS preflight in the cross-origin case.)

rexxars added a commit to rexxars/wpt that referenced this issue Nov 20, 2024
The EventSource API does _not_ run any preflight request when specifying
the `Last-Event-ID` header, and so `fetch` requests should also allow
this header to be set manually.

See whatwg/fetch#568 for more information
@rexxars
Copy link

rexxars commented Nov 20, 2024

Thanks for the clarification! Have updated WPT tests to match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security/privacy There are security or privacy implications topic: cors
Development

No branches or pull requests

6 participants
@rexxars @annevk @yutakahirano @horo-t @yoichio and others