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

Figure out if TrustedURL needs to be absolute. #8

Closed
koto opened this issue Sep 25, 2017 · 2 comments
Closed

Figure out if TrustedURL needs to be absolute. #8

koto opened this issue Sep 25, 2017 · 2 comments
Labels

Comments

@koto
Copy link
Member

koto commented Sep 25, 2017

If we absolutize the TrustedURLs on creation, this might be backwards-incompatible, as String(TrustedURL.unsafelyCreate(foo)) !== foo. On the other hand, relative URLs may actually be a well-formed absolute URLs (http://foo.bar is a valid URL path IIRC, so is //foo.bar/), which may cause obvious problems when assigned to a sink.

@koto koto added the spec label Sep 26, 2017
@mikesamuel
Copy link
Collaborator

I think we should allow relative URLs so that's my bias but below I enumerate some intermediate measures and pros/cons. Not remotely exhaustive.

Possible Postures

Allow relative URLs

Disallow all relative URLs

Including

  • protocol-relative URLs (//authority/path),
  • path-relative (/path),
  • query (?key=value), and
  • fragment-only (#fragment)

Allow Fragment-only URLs but not others

There are several kinds of relative URLs, but Std66 calls them out for special treatment because fragments are part of a URI reference, and a fragment-only change does not cause the browser to issue a network fetch.

Make this per-IDL property

Perhaps with an additional property AllowRelative annotation.
Maybe treating differently the IDL properties that resolve against a base URL besides the document base would be a natural kind of distinction to make.

Risks

Requiring absolute URLs makes migration difficult

I can't really quantify this but my sense is that relative URLs are so widely used that disallowing them would be a significant blocking factor to adoption, and that the main workaround would be to insert code that resolves relative URLs against the document's base URL at creation time which would have a similar risks to just allowing them, and would deprive the IDL property handlers of possibly valuable context.

Safe URL resolved against base URL with unexpected protocol

This is possible since not all URLs are resolved against the document's base. For example, imported stylesheets are resolved against the URL of the stylesheet containing the import statement. Similarly for HTML imports.

Part of a system blesses simple path URLs like /%0aalert(1) and the base URL is something like javascript://foo/, then the system might produce javascript://foo/%0aalert(1) which executes the JavaScript

//foo/
alert(1)

My understanding is that a non-hierarchical URL like the above can be a base URL because some opaque schemes like about: and mailto: do have query components.

javascript: is not a hierarchical URL and does not allow a query component, so my understanding of Std 66 Sec 5 is that resolving the above should yield just the base URL: javascript://foo/.

In any event, resolving a relative URL against an opaque URL should result in a URL in a different origin per https://url.spec.whatwg.org/#origin:

"file"
Unfortunate as it is, this is left as an exercise to the reader. When in doubt, return a new opaque origin.

Otherwise
Return a new opaque origin.

Relative URL masks attack

In some cases, an attack is more apparent when seen in context:

../../../../../../etc/shadow is less obviously problematic than https://example.com/../../../../../../etc/shadow since the latter obviously requires dropping .. silently during normalization.

Encouraging user code to convert a relative URL to an absolute TrustedURL might give user code a place to flag this, and would prevent directory traversal by the server, but network requests by browsers do not contain excess ".." path segments (modulo ), and IDL endpoints can do as good a job of this as user code and doing it in the IDL handlers could provide more context to heuristic intrusion detection systems.

@koto
Copy link
Member Author

koto commented Jan 17, 2019

This seems obsolete as types are just wrappers for strings, and the sinks perform the value normalization; please reopen if not.

@koto koto closed this as completed Jan 17, 2019
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