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

Should ORB block application/javascript with either JSON or JS-parser-breakers #30

Open
anforowicz opened this issue Nov 12, 2021 · 6 comments
Labels

Comments

@anforowicz
Copy link
Collaborator

There is a difference between CORB and ORB behavior (I've detected the difference when applying CORB tests to an initial implementation of CORB->ORB transition):

  • Current CORB implementation always (for all MIME types, ignoring X-Content-Type-Options: nosniff) sniffs the first bytes for a beginning of a JSON object (e.g. { "propertyName":) and/or for JS parser breakers (e.g. {}&& used by Apache struts, or for(;;); observed on Facebook, or )]}' observed on Google). When such prefix is detected, then CORB will block such response. This feature seems like a good idea in general, although AFAIK there is no hard data for how often this protection matters in practice.

  • ORB's initial steps ask to return true (allowed) if mimeType is an opaque-safelisted MIME type. This includes JavaScript MIME types.

I think that we could in theory adjust ORB to match CORB by removing "JavaScript MIME type" from ORB's definition of an "opaque-safelisted MIME type". After this change the "If response's body parses as JavaScript and does not parse as JSON, then [...]" sniffing step in ORB would execute more often. Still, this is a bit more complicated, because this requires additional tweaks to allow Javascript with nosniff (which would currently be blocked by post-media-sniffing steps: "If nosniff is true, then return false.").

/cc @csreis

@annevk
Copy link
Owner

annevk commented Nov 15, 2021

Can you expand on the risk of safelisting the JavaScript MIME type? Is the worry that JSON is often mislabeled?

It seems unfortunate to not be able to offer folks a fast path (except for adopting CORS I suppose).

@anforowicz
Copy link
Collaborator Author

Can you expand on the risk of safelisting the JavaScript MIME type? Is the worry that JSON is often mislabeled?

FWIW always-on sniffing for JSON was added in https://crrev.com/c/835003/. I think that one motivation was to make CORB cover not only known JSON MIME types but also things like application/octet-stream. OTOH, the CL description only singles out application/javascript.

It seems unfortunate to not be able to offer folks a fast path (except for adopting CORS I suppose).

Ack. I am not sure what the right answer here is.

FWIW CORB gets away today with the performance penalty. OTOH CORB sniffs only the first 1024 bytes (and adding anti-JSON sniffing of the first 1024 bytes to ORB would complicate the algorithm).

@annevk
Copy link
Owner

annevk commented Nov 17, 2021

Yeah, sniffing the first 1024 bytes is probably fine.

Perhaps Cross-Origin-Resource-Policy: cross-origin can be a fast path, but that would mainly be relevant for the full parse, not sniffing. To be consistent with its cross-origin embedder policy semantics, the header would have to be present from the first cross-origin redirect onward as well. That might not be too involved to track however.

@annevk
Copy link
Owner

annevk commented May 17, 2022

So thinking about this more it's not technically equivalent so I think we have to specify this if we want to keep it around in this form.

E.g., while (1); would execute in ORB, but result in a network error in Chrome's current implementation, as I understand it. So I think we should adjust the algorithm for this. @anforowicz do you want to work out a patch?

cc @farre

@annevk annevk added the mvp label May 17, 2022
@anforowicz
Copy link
Collaborator Author

I think/hope that figuring out what to do here won't block rolling out 1) initial Fetch spec changes for ORB and 2) ORB v0.1 rollout in Chromium. (My thinking/hope is based on the fact that handling of JS parser breakers seems like a relatively minor aspect of ORB that A) shouldn't need major spec changes and B) shouldn't be observable by more than a handful of WPT tests.)

@anforowicz do you want to work out a patch?

Let me /cc @otherdaniel who is currently looking into next ORB steps from Chromium side. Either they or me or on the hook for eventually incorporating JS parser breakers into ORB spec and WPTs. (But for now it seems that there are other, higher priority items for driving the cross-browser alignment - e.g. exploring switching Chromium to block-via-net-error from block-by-injecting-empty-response-body is important for ensuring that Chromium passes majority of ORB's WPT tests - this will be important for driving/pushing Chromium's alignment with the full ORB spec and Firefox implementation in the future.)

@annevk
Copy link
Owner

annevk commented May 19, 2022

I agree that it's somewhat minor from an observability perspective, but I think it does end up changing the algorithm in some substantive ways. As in, we have to wait for 1024 bytes first, then scan for this, and only at that point safelist. (I guess never-sniff blocklist MIME types can still be early.)

Once I have the "wait for 1024 bytes" formalized better it seems pretty easy to add the JSON parser breaker sniffer, but not sure about the JSON object sniffer.

(The main thing I worry about at the moment is media still.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants