Sid Stamm [:geekboy or :sstamm] Reporter
	Description • 10 years ago

CSP 1.1 defines a base-uri directive.  Need two things:
1. Fix the spec to be a bit clearer (some text in the draft doesn't make sense)
2. Implement it.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 1 • 10 years ago

For this patch to work we have to introduce a function called allowsBaseURI() in CSPUtils, which is unfortunate but since the base-uri is too special, we can not handle it with our ::allows() function family which currenlty does not accept a URI as an argument. The alternative to the approach in this patch would be to extend the ::allows functions with aURI. In terms of semantics I really don't want to do that, because semantic-wise I would like to keep the separation between ::allows() and ::permits(), where ::permits() handles all external resource loads. Restricting base-uri falls somewhere in between ::allows and ::permits. The same argument also holds for LogViolationDetails and the introduction of LogViolationDetailsForBaseURI.

Future Refactoring (follow up):
Once Bug 999656 [1] lands, we could slightly improve the code in this patch by using
restrictsContentType instead of restrictsBaseURI (also getDirectiveStringForContentType instead of getDirectiveStringForBaseURI).
We could also wait till Bug 999656 lands which introduces an intermediated layer of mappings between contentTYpes and CSP-restricting types.
If you want to do that, we could make Bug 999656 block this one. However, I think we should land as is and refactor later (once a type for base-uri becomes available).

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=999656

	Sid Stamm [:geekboy or :sstamm] Reporter
	Comment 3 • 10 years ago

Comment on attachment 8475455 [details] [diff] [review]
bug_1045897_base-uri.patch

Review of attachment 8475455 [details] [diff] [review]:
-----------------------------------------------------------------

You should make the base URI check much like the permitsAncestry check to simplify... unless there's a good reason for doing the violation reporting outside nsCSPContext.

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +141,5 @@
> +   *     Whether or not the effects of base-uri should be allowed
> +   *     (block the setting if false).
> +   */
> +  boolean getAllowsBaseURI(in nsIURI aURI,
> +                           out boolean shouldReportViolations);

Wacky idea: what if we model this new type of check after permitsAncestry?  Should have almost the same function signature (though nsIURI instead of nsIDocShell).  I'm thinking it would take care of the reporting internally and the signature would be simple like: 
> boolean permitsBaseUri(in nsIURI aURI);

@@ +189,5 @@
> +   *
> +   * (Note: Unfortunately base-uri is too special which makes
> +   *  logViolationDetails unusable for that directive.)
> +   */
> +  void LogViolationDetailsForBaseURI(in nsIURI aURI);

Can you just do the violation reporting inside getAllowsBaseURI() like we do for permitsAncestry()?  That avoids adding this function to the IDL and simplifies the call-site.

::: content/base/src/nsCSPUtils.cpp
@@ +926,5 @@
> +  for (uint32_t i = 0; i < mDirectives.Length(); i++) {
> +    if (mDirectives[i]->restrictsBaseURI()) {
> +      mDirectives[i]->toString(outDirective);
> +      return;
> +    }

Is there a reason you added a restrictsBaseURI() function instead of using mDirectives[i]->equals(CSP_BASE_URI) like getReportURIs does?  base-URI is very special-purpose and there's no mapping from content types to this directive, so there's no many::one mappings we need to do here.

	Sid Stamm [:geekboy or :sstamm] Reporter
	Comment 4 • 10 years ago

Comment on attachment 8475457 [details] [diff] [review]
bug_1045897_base-uri_tests.patch

Review of attachment 8475457 [details] [diff] [review]:
-----------------------------------------------------------------

These look good.  Would it be hard to add a positive check to make sure <base> works with tags that satisfy the policy (or when the base-uri directive is not present)?

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 5 • 10 years ago

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #3)
> You should make the base URI check much like the permitsAncestry check to
> simplify... 

For some reason I wanted to model this patch based on the other allows() functions in our code, but baseURI in fact is soo different from the regular allows(), that it makes sense to rather model it based on permitsAncestry. I performed that change, which allows us to remove LogViolationDetailsForBaseURI because the callsite indeed should not be responsible for logging the violations. Good point - looks way better now!

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 6 • 10 years ago

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #4)
> Would it be hard to add a positive check to make sure
> <base> works with tags that satisfy the policy

Basically that is what I wanted, but it makes the test setup way more complicated. We could do it if you really want, but I don't think it's worth putting in the effort. We would have to change the test setup to not only include a csp-observer but also a meachnism to check if the base-uri call succeeded. Particularly time intensive because setting the base-uri to the same domain doesn't make a whole lot of sense and if we use different domains, then we would have to somehow circumvent the same origin policy. As I said, probably not worth the effort. If you insist, I'll do it of course.

	Sid Stamm [:geekboy or :sstamm] Reporter
	Comment 7 • 10 years ago

Comment on attachment 8486797 [details] [diff] [review]
bug_1045897_base-uri.patch

Review of attachment 8486797 [details] [diff] [review]:
-----------------------------------------------------------------

In general, looks good.  I have lots of questions.  Please answer them and re-flag me (or post a new patch and re-flag).

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +136,5 @@
> +   * @return
> +   *     Whether or not the effects of base-uri should be allowed
> +   *     (block the setting if false).
> +   */
> +  boolean allowsBaseURI(in nsIURI aURI);

Because this is more like permitsAncestry, we should call it "permits" as well.  For consistency: lets stick with "allows" for content-type mappings and "permits" for specific things that do their own reporting.

::: content/base/src/nsCSPContext.cpp
@@ +402,5 @@
> +                                 i,             /* policy index        */
> +                                 EmptyString(), /* no observer subject */
> +                                 EmptyString(), /* no source file      */
> +                                 EmptyString(), /* no script sample    */
> +                                 0);            /* no line number      */

Are there ways to get the line number and script sample from the base tag parser and pass them into this function?  Might help developers.  If it's nontrivial, don't do it.

::: content/base/src/nsCSPUtils.cpp
@@ +866,5 @@
> +  for (uint32_t i = 0; i < mDirectives.Length(); i++) {
> +    if (mDirectives[i]->equals(CSP_BASE_URI)) {
> +      // calling permits internally since we want to
> +      // perform a host-src match
> +      if (mDirectives[i]->permits(aURI, dummyNonce)) {

Note to self: we really need to overload permits() to have a single-arg permits that passes in a dummy nonce so we don't have to keep creating empty strings.

@@ +874,5 @@
> +    }
> +  }
> +
> +  // if no base-uri directive can be found, default to true
> +  return true;

Please cite the spec section here since it is different than the other stuff. Looks like section 7.1.

::: content/base/src/nsCSPUtils.h
@@ +89,5 @@
>    "connect-src",     // CSP_CONNECT_SRC
>    "report-uri",      // CSP_REPORT_URI
>    "frame-ancestors", // CSP_FRAME_ANCESTORS
> +  "reflected-xss",   // CSP_REFLECTED_XSS
> +  "base-uri",        // CSP_BASE_URI

Nit: trailing comma.  Might be ok sometimes, but some compilers may balk.

::: content/base/src/nsDocument.cpp
@@ +3417,5 @@
>    }
>  
> +  // Check if CSP allows this base-uri
> +  nsCOMPtr<nsIContentSecurityPolicy> csp;
> +  nsresult rv = GetPrincipal()->GetCsp(getter_AddRefs(csp));

Please use NodePrincipal() (its name guarantees a return value and is more common in this file).

@@ +3422,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (csp) {
> +    bool allowBaseURI = false;
> +    rv = csp->AllowsBaseURI(aURI,
> +                            &allowBaseURI);

No need to wrap this line.

@@ +3425,5 @@
> +    rv = csp->AllowsBaseURI(aURI,
> +                            &allowBaseURI);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    if (!allowBaseURI) {
> +      return NS_OK;

Is it a success, or should we throw an error?  What happens for other invalid base URI cases?

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 8 • 10 years ago

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #7)
> Because this is more like permitsAncestry, we should call it "permits" as
> well.  For consistency: lets stick with "allows" for content-type mappings
> and "permits" for specific things that do their own reporting.

Agreed, that sounds like a clean way of distinguishing allows() and permits().

> Are there ways to get the line number and script sample from the base tag
> parser and pass them into this function?  Might help developers.  If it's
> nontrivial, don't do it.

I looked into this, but it seems not worth the effort, we would have to change too many function signatures. I propose we stick with what we have for now; if it ever becomes an issue and developers really express their desire, we can incorporate that change and include a code sample and a line number when reporting the error.

> Note to self: we really need to overload permits() to have a single-arg
> permits that passes in a dummy nonce so we don't have to keep creating empty
> strings.

Totally agree, I overloaded permits, so we can call it with a single 'uri' argument.

> @@ +874,5 @@
> > +    }
> > +  }
> > +
> > +  // if no base-uri directive can be found, default to true
> > +  return true;
> 
> Please cite the spec section here since it is different than the other
> stuff. Looks like section 7.1.

Well, the spec is actually not completely explicit if setting the base-uri should be allowed if it's not present in the policy, see:
http://www.w3.org/TR/CSP11/#directive-base-uri

In my opinion it makes sense to default to 'true', which also reflects the behavior we use for permitsAncestry:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPUtils.cpp#781
and also for 'script-src' in ::allows():
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPUtils.cpp#837

So, in my opinion it doesn't behave different to any other checks in allows(), or permits().

> ::: content/base/src/nsCSPUtils.h
> @@ +89,5 @@
> >    "connect-src",     // CSP_CONNECT_SRC
> >    "report-uri",      // CSP_REPORT_URI
> >    "frame-ancestors", // CSP_FRAME_ANCESTORS
> > +  "reflected-xss",   // CSP_REFLECTED_XSS
> > +  "base-uri",        // CSP_BASE_URI
> 
> Nit: trailing comma.  Might be ok sometimes, but some compilers may balk.

Wanted to save the next person who extends that list the trouble of a compile error. Usually you forget to add a comma on the previous line if you extend :-). Anyway, fixed that.

> ::: content/base/src/nsDocument.cpp
> @@ +3417,5 @@
> >    }
> >  
> > +  // Check if CSP allows this base-uri
> > +  nsCOMPtr<nsIContentSecurityPolicy> csp;
> > +  nsresult rv = GetPrincipal()->GetCsp(getter_AddRefs(csp));
> 
> Please use NodePrincipal() (its name guarantees a return value and is more
> common in this file).

I thought the prefix 'Get' signals that something never returns null in general. In the end it doesn't make a difference in nsDocumnet, see:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#3056
Anyway, NodePrincipal() it is.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 9 • 10 years ago

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #7)
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    if (!allowBaseURI) {
> > +      return NS_OK;
> 
> Is it a success, or should we throw an error?  What happens for other
> invalid base URI cases?

I think we should return NS_OK, because permitsBaseURI logs the error to the webconsole anyway. Currently, ::SetBaseURI never returns anything else than NS_OK:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#3453

Nevertheless, some callsites are set up to handle an error as well:
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLSharedElement.cpp#176

I am not completely sure whether we should return NS_ERROR_FAILURE or NS_OK. Johnny, what would you suggest?

	Johnny Stenback (:jst)
	Comment 10 • 10 years ago

Comment on attachment 8492409 [details] [diff] [review]
bug_1045897_base-uri_documents_changes.patch

Returning NS_OK for a URI that is not permitted matches what we do today if we fail same origin checks etc, so IMO that's what we want in this case as well.

r=jst

	Sid Stamm [:geekboy or :sstamm] Reporter
	Comment 11 • 10 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> > @@ +874,5 @@
> > > +    }
> > > +  }
> > > +
> > > +  // if no base-uri directive can be found, default to true
> > > +  return true;
> > 
> > Please cite the spec section here since it is different than the other
> > stuff. Looks like section 7.1.
> 
> Well, the spec is actually not completely explicit if setting the base-uri
> should be allowed if it's not present in the policy, see:
> http://www.w3.org/TR/CSP11/#directive-base-uri

But the spec does say that base-uri is not enforced when default-src is present (i.e., base-uri does not get enforced unless it is explicitly listed in the policy).  Maybe cite the part of the spec about default-src (by number, not URL):
http://www.w3.org/TR/CSP11/#directive-default-src

> In my opinion it makes sense to default to 'true', which also reflects the
> behavior we use for permitsAncestry:
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPUtils.
> cpp#781

Yes.  Agreed.  The logic is correct, I was just asking for a comment with a spec reference.

> So, in my opinion it doesn't behave different to any other checks in
> allows(), or permits().

Oh it does behave differently than some directives.  For example, script-src "falls through" to the default directive (so do image-src, etc).  base-uri one does *NOT*.  If there's a default-src but no base-uri, we do not use default-src for base-uri (it's like frame-ancestors as you said above).  I want to clearly comment the code so it's obvious that an absence of base-uri does not fall back to default-src.  Same as with frame-ancestors.

> Wanted to save the next person who extends that list the trouble of a
> compile error. Usually you forget to add a comma on the previous line if you
> extend :-). Anyway, fixed that.

Yeah, if you're worried about that you can start the lines with commas (but that's uglier).

	Sid Stamm [:geekboy or :sstamm] Reporter
	Comment 12 • 10 years ago

Comment on attachment 8492407 [details] [diff] [review]
bug_1045897_base-uri.patch

Review of attachment 8492407 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  r=me with some minor tweaks.

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +196,5 @@
>     */
>    boolean permitsAncestry(in nsIDocShell docShell);
>  
>    /**
> +   * Whether this policy permits to set the base-uri.

Nit: this is awkward, maybe:
" Whether this policy allows setting the document's base URI to a given value".

@@ +200,5 @@
> +   * Whether this policy permits to set the base-uri.
> +   *
> +   * @return
> +   *     Whether or not the effects of base-uri should be allowed
> +   *     (block the setting if false).

Again, awkward comment:
"Whether or not the provided URI is allowed to be used as the document's base URI."

::: content/base/src/nsCSPContext.cpp
@@ +1054,5 @@
>  }
>  
> +NS_IMETHODIMP
> +nsCSPContext::PermitsBaseURI(nsIURI* aURI,
> +                             bool*   outPermitsBaseURI)

No need to wrap here, you are adding some lines longer than this will be as one line.

@@ +1056,5 @@
> +NS_IMETHODIMP
> +nsCSPContext::PermitsBaseURI(nsIURI* aURI,
> +                             bool*   outPermitsBaseURI)
> +{
> +  *outPermitsBaseURI = true;

Wouldn't hurt to check for null aURI here (you can bail out sooner if it is null, maybe even return NS_ERROR_FAILURE like PermitsAncestry() does).

::: content/base/src/nsCSPUtils.cpp
@@ +621,5 @@
> +nsCSPDirective::permits(nsIURI* aUri) const
> +{
> +  nsString dummyNonce;
> +  return permits(aUri, dummyNonce);
> +}

Great.

@@ +823,5 @@
> +    if (mDirectives[i]->equals(CSP_BASE_URI)) {
> +      if (mDirectives[i]->permits(aUri)) {
> +        return true;
> +      }
> +      return false;

Nit: this can be simplified to

  if (mDirectives[i]->equals(CSP_BASE_URI)) {
    return mDirectives[i]->permits(aUri));
  }

@@ +828,5 @@
> +    }
> +  }
> +
> +  // if no base-uri directive can be found, default to true
> +  return true;

This is where I want to add a comment about the spec saying we don't fall back to default-src if there's no base-uri directive.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 13 • 10 years ago

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #12)
> Comment on attachment 8492407 [details] [diff] [review]
> bug_1045897_base-uri.patch

> > +NS_IMETHODIMP
> > +nsCSPContext::PermitsBaseURI(nsIURI* aURI,
> > +                             bool*   outPermitsBaseURI)
> > +{
> > +  *outPermitsBaseURI = true;
> 
> Wouldn't hurt to check for null aURI here (you can bail out sooner if it is
> null, maybe even return NS_ERROR_FAILURE like PermitsAncestry() does).

That makes sense - incorporated that change.

> Nit: this can be simplified to
> 
>   if (mDirectives[i]->equals(CSP_BASE_URI)) {
>     return mDirectives[i]->permits(aUri));
>   }

You really like that pattern ;-) - makes sense to me.

> > +  // if no base-uri directive can be found, default to true
> > +  return true;
> 
> This is where I want to add a comment about the spec saying we don't fall
> back to default-src if there's no base-uri directive.

Yes, now I get what you mean. Added a comment and linked to the spec. Should be fine now.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 14 • 10 years ago

Testing on all platforms to make sure everything is fine - since we are adding a new CSP feature:
  https://tbpl.mozilla.org/?tree=Try&rev=d8d587588dae

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 15 • 10 years ago

 https://hg.mozilla.org/integration/mozilla-inbound/rev/04dc86a457c1
 https://hg.mozilla.org/integration/mozilla-inbound/rev/30758e745b32
 https://hg.mozilla.org/integration/mozilla-inbound/rev/19c7a29f7676

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/04dc86a457c1
https://hg.mozilla.org/mozilla-central/rev/30758e745b32
https://hg.mozilla.org/mozilla-central/rev/19c7a29f7676

	Jean-Yves Perrier [:teoli]
	Comment 17 • 9 years ago

Doc updated:
https://developer.mozilla.org/en-US/Firefox/Releases/35
and
https://developer.mozilla.org/en-US/docs/Web/Security/CSP/CSP_policy_directives