Closed Bug 994320 Opened 10 years ago Closed 10 years ago

CSP in C++: implement permitsAncestry (frame-ancestors check)

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: geekboy, Assigned: geekboy)

References

Details

Attachments

(2 files, 5 obsolete files)

Add permitsAncestry checks to new parser.
Blocks: 925004
Attached patch csp_permitsAncestry (obsolete) — Splinter Review
Here's a patch.  Requires the patches for bug 951457 to be applied first.  This also hacks up the mochitests for frameancestors, but I'm not sure what we should do there. (Should I make new test files, or fix the old ones, garrett?)
Attachment #8405636 - Flags: review?(mozilla)
Attachment #8405636 - Flags: review?(grobinson)
Comment on attachment 8405636 [details] [diff] [review]
csp_permitsAncestry

Bitrotted due do changes in parser and utils. Can you please export again once we are done reviewing parser and utils? Thanks
Attachment #8405636 - Flags: review?(mozilla)
Attachment #8405636 - Flags: review?(grobinson)
Attached patch csp_permitsAncestry (obsolete) — Splinter Review
Ok, is unbitrotted and ready for review.
Attachment #8405636 - Attachment is obsolete: true
Attachment #8418682 - Flags: review?(mozilla)
Attachment #8418682 - Flags: review?(grobinson)
Comment on attachment 8418682 [details] [diff] [review]
csp_permitsAncestry

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

That is pretty close and pretty much ready! I would like to see it one more time, mostly because I want to make sure that there are no issues, where you have the comment 'this is broken'.
Also, this patch should not include changes for the following files:
* file_CSP_frameancestors.sjs
* file_CSP_frameancestors_main.html 
* file_CSP_frameancestors_main.js 
* file_CSP_frameancestors_main_spec_compliant.html 
* file_CSP_frameancestors_main_spec_compliant.js 
* file_CSP_frameancestors_spec_compliant.sjs 
* mochitest.ini 
* test_CSP_frameancestors.html

I guess for testing we should move those changes into the patch 'new_backend_mochitests' and then Garrett can take it from there when he is changing the testsuite!

::: content/base/src/nsCSPContext.cpp
@@ +451,5 @@
> +  //            ->GI(nsIDocShellTreeItem)
> +  //            ->QI(nsIInterfaceRequestor)
> +  //            ->GI(nsIWebNavigation)
> +  //            ->GetCurrentURI();
> +  nsCOMPtr<nsIInterfaceRequestor> ir(do_QueryInterface(aDocShell));

Mhm, I don't really like that comment, should we rather have a comment that briefly explains what permitsAncestry does and how? Probably a block statement on top of the function?

@@ +462,5 @@
> +         parentTreeItem != nullptr) {
> +    ir     = do_QueryInterface(parentTreeItem);
> +    webNav = do_GetInterface(ir);
> +
> +    nsCOMPtr<nsIURI> currentURI;

No need to define currentURI inside the while loop.

@@ +475,5 @@
> +      if (isChrome) { break; }
> +
> +      // TODO: this is broken.  Something is happening to the objects in the
> +      // array.
> +      // delete the userpass from the URI.

Is that still a problem or a legacy comment?

@@ +476,5 @@
> +
> +      // TODO: this is broken.  Something is happening to the objects in the
> +      // array.
> +      // delete the userpass from the URI.
> +      nsCOMPtr<nsIURI> uriClone;

Can't uriClone also be defined outside the while-loop?

@@ +485,5 @@
> +#ifdef PR_LOGGING
> +      nsAutoCString spec;
> +      uriClone->GetSpec(spec);
> +      CSPCONTEXTLOG(("nsCSPContext::PermitsAncestry, found ancestor: %s", spec.get()));
> +#endif

I just changed such #ifdef PR_LOGGINGs to include {} everywhere. I am pretty sure I did it for this particular case as well. Probably it just hadn't made it in time when exporting the patch. Can you double check?

@@ +494,5 @@
> +    treeItem = parentTreeItem;
> +  }
> +
> +  nsString violatedDirective;
> +

Does this need to be an nsString, can't we use nsAutoString here?

@@ +500,5 @@
> +  // them against any CSP.
> +  for (uint32_t i = 0; i < mPolicies.Length(); i++) {
> +    for (int32_t a = 0; a < ancestorsArray.Count(); a++) {
> +      // TODO(sid) the mapping from frame-ancestors context to TYPE_DOCUMENT is
> +      // forced, we need something better

Same here, update the comment to include bug number 999656.

@@ +505,5 @@
> +#ifdef PR_LOGGING
> +      nsAutoCString spec;
> +      ancestorsArray[a]->GetSpec(spec);
> +      CSPCONTEXTLOG(("nsCSPContext::PermitsAncestry, checking ancestor: %s", spec.get()));
> +#endif

As above, can you double check that the logging part is sourrounded by {}.

@@ +512,5 @@
> +                                 EmptyString(), // no nonce
> +                                 violatedDirective)) {
> +        // Policy is violated
> +        // we have to fire the observer here
> +        // TODO: NotifyObservers need to cleaned up - check arguments

I assume that already got cleaned up and we can delete that comment.

@@ +515,5 @@
> +        // we have to fire the observer here
> +        // TODO: NotifyObservers need to cleaned up - check arguments
> +        nsCOMPtr<nsIObserverService> observerService =
> +          mozilla::services::GetObserverService();
> +        NS_ASSERTION(observerService, "no observer service");

It think we should return here with an error code instead of the assertion, otherwise we might deref a null pointer.

@@ +520,5 @@
> +        observerService->NotifyObservers(ancestorsArray[a],
> +                                         CSP_VIOLATION_TOPIC,
> +                                         violatedDirective.get());
> +        // TODO(sid) generate violation reports.
> +        // TODO(sid) implement logic for report-only

You can update those comments and explain that those two things will be fixed in bug 994322.

::: content/base/src/nsCSPUtils.cpp
@@ +642,1 @@
>  

Can you change the comment, and also include bug number 999656, which is the bug responsible for refactoring that part.

::: content/base/src/nsCSPUtils.h
@@ +70,2 @@
>  };
>  

Nit: can you align CSP_FRAME_ANCESTORS with the above directives?
Attachment #8418682 - Flags: review?(mozilla) → review-
Comment on attachment 8418682 [details] [diff] [review]
csp_permitsAncestry

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

As ckershcb points out, the test changes need to go in new_backend_mochitests.

::: content/base/test/csp/file_CSP_frameancestors.sjs
@@ +30,5 @@
>      response.setHeader("Content-Type", "text/html", false);
>      response.write('<html><head>');
>      if (query['double'])
>        response.write('<script src="file_CSP_frameancestors.sjs?double=1&scriptedreport=' + query['testid'] + '"></script>');
> +    else

Please brace your if's
Attachment #8418682 - Flags: review?(grobinson)
Attached patch csp_permitsAncestry (obsolete) — Splinter Review
Addressed review comments and split out tests into separate patch.
Attachment #8418682 - Attachment is obsolete: true
Attachment #8421867 - Flags: review?(mozilla)
Attachment #8421867 - Flags: review?(grobinson)
Changes for frame ancestors tests (you can apply this to run tests on the other patch in this bug).  This should probably be folded into the work for bug 988616.
Comment on attachment 8421867 [details] [diff] [review]
csp_permitsAncestry

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

r=me with comments addressed

::: content/base/src/nsCSPContext.cpp
@@ +506,5 @@
> +
> +  // Now that we've got the ancestry chain in ancestorsArray, time to check
> +  // them against any CSP.
> +  for (uint32_t i = 0; i < mPolicies.Length(); i++) {
> +    for (int32_t a = 0; a < ancestorsArray.Count(); a++) {

Any reason why this isn't a uint?

@@ +526,5 @@
> +        nsCOMPtr<nsIObserverService> observerService =
> +          mozilla::services::GetObserverService();
> +        NS_ENSURE_TRUE(observerService, NS_ERROR_NOT_AVAILABLE);
> +
> +        observerService->NotifyObservers(ancestorsArray[a],

We just discussed the multiple calls to NotifyObservers in the office. Is this fixed in a different patch?
Attachment #8421867 - Flags: review?(grobinson) → review+
(In reply to Garrett Robinson [:grobinson] from comment #8)
> ::: content/base/src/nsCSPContext.cpp
> > +    for (int32_t a = 0; a < ancestorsArray.Count(); a++) {
> 
> Any reason why this isn't a uint?

Hah, no reason, good catch.  It would be problematic if Count() was < 0.

> We just discussed the multiple calls to NotifyObservers in the office. Is
> this fixed in a different patch?

Will be fixed in the violation reporting work (bug 994322) where AsyncReportViolation is implemented.  Until that's implemented, we need to notify observers here (that patch will remove this call to NotifyObservers).
Comment on attachment 8421867 [details] [diff] [review]
csp_permitsAncestry

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

Please address Garrett's and my comment. After that it's ready to land. Please also add 'r=ckerschb, grobinson' in the comment of the patch.

::: content/base/src/nsCSPContext.cpp
@@ +472,5 @@
> +         parentTreeItem != nullptr) {
> +    ir     = do_QueryInterface(parentTreeItem);
> +    webNav = do_GetInterface(ir);
> +
> +    rv = webNav->GetCurrentURI(getter_AddRefs(currentURI));

Not sure why I haven't commented on this in the first round of reviews. Can webNav possiblye be null? Dereferencing a null pointer might be not that great. Probably worth an additonal check.
Attachment #8421867 - Flags: review?(mozilla) → review+
Attached patch csp_permitsAncestry (obsolete) — Splinter Review
Thanks for the quick turnaround, guys.  Addressed your comments and updated the commit message (here and in the working patch queue repo).  This should be ready to go when its dependencies have landed.  Carrying over r=ckerschb,grobinson
Attachment #8421867 - Attachment is obsolete: true
Attachment #8421968 - Flags: review+
Since we moved the parser tests up in the patch queue, this patch got bitrotted because we had to lift out some of the frame-ancestors tests and put the into this patch. Please export again before landing.
Also, I just realized that we getting a compiler warning when compiling:

0:05.74     for (uint32_t a = 0; a < ancestorsArray.Count(); a++) {
0:05.74                          ~ ^ ~~~~~~~~~~~~~~~~~~~~~~
0:05.74 1 warning generated.

Maybe we should fix that as well before we land this.
Yeah, turns out there was a reason that was int32_t and not unsigned.
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCOMArray.h#96

We could change that to Length() instead to get a uint.  I'll do that.
Attached patch csp_permitsAncestry (obsolete) — Splinter Review
Trivial tweak of previous r+ed patch (changing Count() to Length() in array loop to avoid sign conversion warnings).
Attachment #8421968 - Attachment is obsolete: true
Attachment #8423205 - Flags: review+
Augh.  This built fine locally, and we ran it through try (though with some other patches).

froydnj suggests we need to #include "nsIInterfaceRequestor.h", but inbound builds (linux debug) locally just fine on my machine without it.  Maybe a unified build+includes issue?

Chris pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=78bccabd69a2

I pushed on a few more platforms (exact busted changesets from m-i, plus a one-line include addition):
https://tbpl.mozilla.org/?tree=Try&rev=8746fd18d3da

If this goes green, I'll tweak this patch to add the include and we can reland this bug and bug 994318.
Looks like the try builds work.  Since this code is behind a pref, I don't think it would help to run the full test suite before re-landing the code.

Updated the bouncy patch with an include for nsIInterfaceRequestor.h, carrying over reviews given this trivial change.
Attachment #8423205 - Attachment is obsolete: true
Attachment #8425821 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/bb2b4df7cc65
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: