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

origin-or-null definition and TAO processing #152

Closed
yoavweiss opened this issue Apr 14, 2018 · 16 comments · Fixed by web-platform-tests/wpt#13518
Closed

origin-or-null definition and TAO processing #152

yoavweiss opened this issue Apr 14, 2018 · 16 comments · Fixed by web-platform-tests/wpt#13518

Comments

@yoavweiss
Copy link
Contributor

I'm failing to find if and where origin-or-null is defined. The "null" case is also not addressed in the "timing allow check" processing model.

IIUC, we need to:

Thoughts?

@igrigorik
Copy link
Member

igrigorik commented Apr 14, 2018

origin-or-null is defined in Fetch: https://fetch.spec.whatwg.org/#origin-header. Related discussion in: #62 (comment)

@yoavweiss
Copy link
Contributor Author

OK, cool. So we can just link to that definition. The rest of the points still stand.

@yoavweiss
Copy link
Contributor Author

@marcoscaceres - It seems that a lot of my origin confusion here resulted from the fact that the links (which exist in the markup) are not shown. Are there known issues around Respec and links inside <pre>?

@marcoscaceres
Copy link
Member

Generally there should be no issues, but it might be confusing the syntax highlighting if it’s inside a pre. I can have a look.

@yoavweiss
Copy link
Contributor Author

Following a discussion on IRC with @annevk, I tend to think this will require larger Fetch integration than I initially thought:

  • "null" handling should work for tainted or opaque origins
  • In order to support that, we're better off relying on some version of Fetch's serializing a request origin, only we'll need an equivalent using the Document's origin. I'm not quite sure how that would work.
  • We'd need tests for all of the above.

@yoavweiss yoavweiss modified the milestones: Level 2, Level 3 Oct 12, 2018
@annevk
Copy link
Member

annevk commented Oct 12, 2018

You'd invoke https://html.spec.whatwg.org/#ascii-serialisation-of-an-origin directly on document's origin (which is defined at https://dom.spec.whatwg.org/#concept-document-origin by the way).

The main problem I see with your current setup is that you have no notion of redirects so if A requests B which redirects to A, this would consider it same-origin, despite it leaking information about B. That seems bad.

@annevk
Copy link
Member

annevk commented Oct 12, 2018

It seems that a number of these things you can and should address today, either by fixing or adding issue markers, so implementers don't implement security vulnerabilities.

@yoavweiss
Copy link
Contributor Author

You'd invoke https://html.spec.whatwg.org/#ascii-serialisation-of-an-origin directly on document's origin (which is defined at https://dom.spec.whatwg.org/#concept-document-origin by the way).

OK, so that's one missing piece that would cover opaque origins and is fairly easy to add.

The main problem I see with your current setup is that you have no notion of redirects so if A requests B which redirects to A, this would consider it same-origin, despite it leaking information about B. That seems bad.

Good point. AFAICT, fixing it would require a) RT to start using Fetch's Response objects rather than "resource" b) Fetch to propagate the "tainted" flag from Request to Response. I can definitely add an issue marker in the short term, but fixing it seems like it would fit well with the larger Fetch-based "rebase".

@annevk
Copy link
Member

annevk commented Oct 12, 2018

Thanks. Calling these issues out inline would already be a huge help, especially since not everyone has time to study the issue list in detail.

@yoavweiss
Copy link
Contributor Author

@annevk Seems like the cross-origin redirect sandwich case is already covered by a WPT.

@annevk
Copy link
Member

annevk commented Oct 15, 2018

As far as I can tell resource-timing/resource_timing_cross_origin_redirect_chain.html doesn't test same-origin to cross-origin, but I'm not really familiar with the harness in use there.

@yoavweiss
Copy link
Contributor Author

yoavweiss commented Oct 15, 2018

You're right. The chain is XO->XO->SO->SO (where TAO is set for the XO resources). I'll add a test for the SO->XO->SO chain

@toddreifsteck
Copy link
Member

@yoavweiss Could you explain why fetch integration is 100% required? Can't we simply add a bit of verbage to monkey patch this in to RT 2? I may simply be naive. :-)

@toddreifsteck toddreifsteck modified the milestones: Level 3, Level 2 Jan 8, 2019
@toddreifsteck
Copy link
Member

Pulling it back so we can discuss. Sorry if that causes any hassle. This one feels like it belongs in 2 from my sniff of it.

@marcoscaceres
Copy link
Member

Generally speaking, if you can avoid monkey patching that's preferred - as when implementing, it means implementers can safely send things into the fetch machinery, which can be updated independently of this spec. The monkey patch risks getting out of date with the spec it is patching, meaning that a middle layer could end up being required for compat with this spec.

@yoavweiss
Copy link
Contributor Author

#172 addresses some of the definitions here that don't require Fetch integration, and adds a note regarding the security concerns. A review of that as well as web-platform-tests/wpt#13518 would be appreciated. A previous comment covers the missing bits in Fetch and related to Fetch integration. Monkey-patching those here don't seem like a small feat, nor does that seem helpful to do outside the context of the wider Fetch integration.

So with that, I'll create a new issue that makes sure the above PRs land for L2, but bump this one back to L3, for the larger scope Fetch integration.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 5, 2019
…ndwich with and without TAO, a=testonly

Automatic update from web-platform-tests
[Resource Timing] Test XO redirection sandwich with and without TAO (#13518)

Add a test to make sure that a Same-Origin=>Cross-Origin=>Same-origin
redirection chain is not exposing timing information unless
Timing-Allow-Origin is set.

Partially fixes w3c/resource-timing#152
--

wpt-commits: dbc26ae4859e45f4df342445c1a622a6e88b3d8b
wpt-pr: 13518
mykmelez pushed a commit to mykmelez/gecko that referenced this issue Jun 6, 2019
…ndwich with and without TAO, a=testonly

Automatic update from web-platform-tests
[Resource Timing] Test XO redirection sandwich with and without TAO (#13518)

Add a test to make sure that a Same-Origin=>Cross-Origin=>Same-origin
redirection chain is not exposing timing information unless
Timing-Allow-Origin is set.

Partially fixes w3c/resource-timing#152
--

wpt-commits: dbc26ae4859e45f4df342445c1a622a6e88b3d8b
wpt-pr: 13518
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Jul 23, 2019
…13518)

Add a test to make sure that a Same-Origin=>Cross-Origin=>Same-origin
redirection chain is not exposing timing information unless
Timing-Allow-Origin is set.

Partially fixes w3c/resource-timing#152
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
…ndwich with and without TAO, a=testonly

Automatic update from web-platform-tests
[Resource Timing] Test XO redirection sandwich with and without TAO (#13518)

Add a test to make sure that a Same-Origin=>Cross-Origin=>Same-origin
redirection chain is not exposing timing information unless
Timing-Allow-Origin is set.

Partially fixes w3c/resource-timing#152
--

wpt-commits: dbc26ae4859e45f4df342445c1a622a6e88b3d8b
wpt-pr: 13518

UltraBlame original commit: c7de277fd48385caefcbba805dd265da4b77b013
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
…ndwich with and without TAO, a=testonly

Automatic update from web-platform-tests
[Resource Timing] Test XO redirection sandwich with and without TAO (#13518)

Add a test to make sure that a Same-Origin=>Cross-Origin=>Same-origin
redirection chain is not exposing timing information unless
Timing-Allow-Origin is set.

Partially fixes w3c/resource-timing#152
--

wpt-commits: dbc26ae4859e45f4df342445c1a622a6e88b3d8b
wpt-pr: 13518

UltraBlame original commit: c7de277fd48385caefcbba805dd265da4b77b013
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
…ndwich with and without TAO, a=testonly

Automatic update from web-platform-tests
[Resource Timing] Test XO redirection sandwich with and without TAO (#13518)

Add a test to make sure that a Same-Origin=>Cross-Origin=>Same-origin
redirection chain is not exposing timing information unless
Timing-Allow-Origin is set.

Partially fixes w3c/resource-timing#152
--

wpt-commits: dbc26ae4859e45f4df342445c1a622a6e88b3d8b
wpt-pr: 13518

UltraBlame original commit: c7de277fd48385caefcbba805dd265da4b77b013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants