search

whatwg / fetch

Fetch Standard
https://fetch.spec.whatwg.org/
Other
2.12k stars 331 forks source link

Safelist Last-Event-ID #568

Open annevk opened 7 years ago

annevk commented 7 years ago

Per https://github.com/whatwg/html/issues/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 commented 7 years ago

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 commented 7 years ago

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

annevk commented 6 years ago

@whatwg/security thoughts?

annevk commented 3 years ago

@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 commented 3 years ago

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

annevk commented 2 years ago

@johnwilander @yutakahirano ping.

yutakahirano commented 2 years ago

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

Can you elaborate on this a bit more?

annevk commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

annevk commented 2 years ago

Yeah.

yutakahirano commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/whatwg/html/issues/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 commented 2 days ago

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 commented 5 hours ago

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.