Closed Bug 658037 Opened 9 years ago Closed 9 years ago

nsContentUtils::CanCallerAccess does not check for GetSubjectPrincipal failure

Categories

(Core :: Security, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: jorendorff, Assigned: mounir)

References

(Blocks 1 open bug)

Details

(Whiteboard: [sg:audit] (could be critical if some code can get itself system principal this way))

Attachments

(2 files, 2 obsolete files)

In nsContentUtils::CanCallerAccess:
>  nsCOMPtr<nsIPrincipal> subjectPrincipal;
>  sSecurityManager->GetSubjectPrincipal(getter_AddRefs(subjectPrincipal));
>
>  if (!subjectPrincipal) {
>    // we're running as system, grant access to the node.
>
>    return PR_TRUE;
>  }

As it stands, GetSubjectPrincipal can fail in a few ways. Either we make it really infallible or CanCallerAccess needs to check.
Similar code in nsContentUtils::CheckSameOrigin:
>  sSecurityManager->SubjectPrincipalIsSystem(&isSystem);
>  if (isSystem) {

But it looks like nsScriptSecurityManager::SubjectPrincipalIsSystem, whether through prudence or serendipity, sets isSystem to false even if it fails. Still: more precarious than security code should be.
Jason, given that SubjectPrincipalIsSystem is [noscript] anyway, do you want to make it [notxpcom] as well and make it infallible? I'll buy you a beer at the next all-hands if you do!
Jst says Mounir can take a crack and cleaning this up.
Assignee: nobody → mounir.lamouri
Whiteboard: [sg:audit] (could be critical if some code can get itself system principal this way)
Attachment #539777 - Flags: review?(jorendorff)
Attachment #539777 - Attachment description: Fix nsContentUtils::ChekSameOrigin → Fix nsContentUtils::CheckSameOrigin
Attached patch Fix GetSubjectPrincipal callers (obsolete) — Splinter Review
Making GetSubjectPrincipal infallible requires to make |nsScriptSecurityManager::GetPrincipalAndFrame| infallible and that task doesn't sound trivial for someone that doesn't know that code...
Attachment #539781 - Flags: review?(jorendorff)
Comment on attachment 539781 [details] [diff] [review]
Fix GetSubjectPrincipal callers

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

Looks good generally, but I'm not a peer in this code. Forwarding review to bz.

I noticed one issue, pointed out below.

::: layout/style/nsCSSStyleSheet.cpp
@@ +1583,5 @@
>      return NS_ERROR_DOM_SECURITY_ERR;
>    }
>  
>    PRBool subsumes;
> +  subjectPrincipal->Subsumes(mInner->mPrincipal, &subsumes);

This line needs to assign the return value to rv, since the next line does NS_ENSURE_SUCCESS(rv, rv).
Attachment #539781 - Flags: review?(jorendorff) → review?(bzbarsky)
Comment on attachment 539777 [details] [diff] [review]
Fix nsContentUtils::CheckSameOrigin

Setting r- on this even though I'm not a peer in caps either.

Subject principals are the principals of code running in the system. Object principals are the principals of objects in the system, the things code wants to be able to work with, the things we want to secure.

(It's just like the subject and object of a sentence in English grammar: in "Chrome code can access content DOM nodes", "Chrome code" is the subject and "content DOM nodes" is the object.)

So PR_FALSE is the right thing here; it means that sloppy users that don't check the return code will, on failure, think that the currently running code is *not* system code.

At least, that's my thinking. Any number of people cc'd in this bug can correct me if I'm wrong. :)
Attachment #539777 - Flags: review?(jorendorff) → review-
Comment on attachment 539781 [details] [diff] [review]
Fix GetSubjectPrincipal callers

>+++ b/content/base/src/nsContentUtils.cpp
> PRBool
> nsContentUtils::CanCallerAccess(nsPIDOMWindow* aWindow)
...
>+  NS_ENSURE_SUCCESS(rv, rv);

Second rv should be PR_FALSE, no?

>+++ b/content/events/src/nsDOMDataTransfer.cpp

The callers of GetCurrentPrincipal() here assume null means "system".  So that's not a safe thing to do on failure...  I guess it's no worse than what we do right now, though.  But can we file a followup on this?

>+++ b/dom/src/storage/nsDOMStorage.cpp
>@@ -132,18 +132,19 @@ GetPrincipalURIAndHost(nsIPrincipal* aPr
>+  ns_result rv = nsContentUtils::GetSecurityManager()->

nsresult.  Did this compile?

>+++ b/layout/style/nsCSSStyleSheet.cpp
>-  nsresult rv = subjectPrincipal->Subsumes(mInner->mPrincipal, &subsumes);
>+  subjectPrincipal->Subsumes(mInner->mPrincipal, &subsumes);

Need to assign to rv still.

>+++ b/xpinstall/src/nsJSInstallTriggerGlobal.cpp
>+  NS_ENSURE_SUCCESS(rv, rv);

This function returns JSBool.  You probably want to report an error and return JS_FALSE.

>+++ b/xpinstall/src/nsXPITriggerInfo.cpp
>@@ -279,17 +279,19 @@ XPITriggerEvent::Run()
>+    NS_ENSURE_SUCCESS(rv, rv);

Do we not need to JS_ReportError?
Attachment #539781 - Flags: review?(bzbarsky) → review-
Comment on attachment 539777 [details] [diff] [review]
Fix nsContentUtils::CheckSameOrigin

Comment 1 was not clear. And I realize it's not event the same file as comment 1 was pointing to... Have no idea why I did that change here...
Attachment #539777 - Attachment is obsolete: true
(In reply to comment #8)
> >+++ b/content/events/src/nsDOMDataTransfer.cpp
> 
> The callers of GetCurrentPrincipal() here assume null means "system".  So
> that's not a safe thing to do on failure...  I guess it's no worse than what
> we do right now, though.  But can we file a followup on this?

Will do.

> >+++ b/dom/src/storage/nsDOMStorage.cpp
> >@@ -132,18 +132,19 @@ GetPrincipalURIAndHost(nsIPrincipal* aPr
> >+  ns_result rv = nsContentUtils::GetSecurityManager()->
> 
> nsresult.  Did this compile?

Sorry, I think I forgot to compile the patch :(

> >+++ b/xpinstall/src/nsXPITriggerInfo.cpp
> >@@ -279,17 +279,19 @@ XPITriggerEvent::Run()
> >+    NS_ENSURE_SUCCESS(rv, rv);
> 
> Do we not need to JS_ReportError?

Yes. And we should probably open a follow-up to remove the ugly |return 0| in this file ;)
Attachment #539781 - Attachment is obsolete: true
Attachment #540063 - Flags: review?(bzbarsky)
Depends on: 665012
To make SubjectPrincipalIsSystem infaillible we could return PR_FALSE if GetSubjectPrincipal() fails. Do we want to do that and make it infaillible? Could be done in a follow-up though.
Attachment #540067 - Flags: review?(bzbarsky)
(In reply to comment #10)
> (In reply to comment #8)
> > >+++ b/content/events/src/nsDOMDataTransfer.cpp
> > 
> > The callers of GetCurrentPrincipal() here assume null means "system".  So
> > that's not a safe thing to do on failure...  I guess it's no worse than what
> > we do right now, though.  But can we file a followup on this?
> 
> Will do.

bug 665012 

> Yes. And we should probably open a follow-up to remove the ugly |return 0|
> in this file ;)

bug 665023
Status: NEW → ASSIGNED
Comment on attachment 540067 [details] [diff] [review]
Fix SubjectPrincipalIsSystem callers

r=me
Attachment #540067 - Flags: review?(bzbarsky) → review+
Comment on attachment 540063 [details] [diff] [review]
Fix GetSubjectPrincipal callers

r=me
Attachment #540063 - Flags: review?(bzbarsky) → review+
Pushed:
http://hg.mozilla.org/mozilla-central/rev/a2d922bd59a6
http://hg.mozilla.org/mozilla-central/rev/b25c7c9606b9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Group: core-security
You need to log in before you can comment on or make changes to this bug.