216041 - principal refactoring was sloppy with JSPrincipals::codebase and changing document principals

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

I noticed this while looking at the code more in depth.  This probably breaks
LiveConnect, as well as potentially other things.  Patch tomorrow after more
testing.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Something like this

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 129786 [details] [diff] [review]
Something like this

Dan, could you have a look at this patch?

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Actually, the stuff in nsJARChannel.cpp needs to also get a principal from the
security manager and then set the URI on that, rather than on the principal it
gets from the JAR.

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

I think this should block beta.  (Thought I had already done this).  Without
this patch, I think LiveConnect is broken, as well as the potential for a few
security holes introduced.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Better patch

This is a slightly less intrusive patch; it leaves the nsIPrincipalPtr member
name alone (that should get renamed eventually, though).  I changed
GetCertificatePrincipal() to take a URI argument as well, since nsJARChannel
needs a URI on a certificate principal.  Ideally, both GetCertificatePrincipal
and GetCodebasePrincipal will be merged into one function since they pretty
much do the same thing, but GetCodebasePrincipal has a lot of callers to
update, so that is best left for a follow-up patch.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 129851 [details] [diff] [review]
Better patch

Actually, new patching coming up....

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

dveditz asked me to mark this confidential for now.

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch

Here we go.

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Alternate patch route

I'm not yet convinced that this is the route to go, but this was suggested by
jst.  I'm starting to think that this bug might best be solved under the
release schedule by backing out the patch to bug 83536, and then to try and
re-land that when 1.6a opens.

Comment 10

[Daniel Veditz [:dveditz]]

Drivers agree -- please back out 83536 and try again in 1.6alpha (*early* in the
alpha)

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

I attached the backout patch in bug 83536.  I will resolve when that lands.

Comment 12

[Asa Dotzler [:asa]]

Chris, so this has landed, right? Can you resolve and create a new bug for
re-fixing in in 1.6a?

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Yeah, the backout landed.  Sure.  Marking fixed.  I'll move these patches over
to a new bug.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Actually, I'll just remove the blocking status and keep this bug.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 130099 [details] [diff] [review]
Patch

- In nsPrincipal::AddRef():

-  nsrefcnt count = PR_AtomicIncrement((PRInt32 *)&mJSPrincipals.refcount);
-  NS_LOG_ADDREF(this, count, "nsPrincipal", sizeof(*this));
+  nsrefcnt count = mJSPrincipals.refcount++;
   return count;

Maybe eliminate the local count and just return mJSPrincipals.refcount++?

- In nsPrincipal::Init():

+  NS_ENSURE_SUCCESS(rv, rv);
+
+  return rv;

Loose the NS_ENSURE_SUCCESS().


- In nsDocument::SetPrincipal():

-  NS_PRECONDITION(aNewPrincipal, "Null principal!");

Don't you want to keep something like above? Or does it make sense to call
SetPrincipal(nsnull)?

Other than that, sr=jst

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 130099 [details] [diff] [review]
Patch

dveditz balked because he couldn't find the time.  Trying peterv.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 130099 [details] [diff] [review]
Patch

>Index: caps/idl/nsIScriptSecurityManager.idl
>     /**
>      * Return a principal with the specified certificate ID.
>      */
>-    [noscript] nsIPrincipal getCertificatePrincipal(in string CertID);
>+    [noscript] nsIPrincipal getCertificatePrincipal(in string CertID, in nsIURI aURI);

"certificate ID and specified URI"?

>Index: caps/include/nsPrincipal.h

>+  nsresult Init(const char *aCertID, nsIURI *aCodebase);

Document that this should always be called before the principal is used for
anything?

>+  void SetURI(nsIURI *aURI);

When can this be called?  What does it do?  Document.

This is a public method; does it need to be?  Could we make the security
manager a friend class and make this protected?

>+  nsresult SetCertificate(const char* aCertID, const char* aName);

Document this, like SetURI.  If this is public, consider also making protected.

>+  PRPackedBool mInitialized;

Does this really affect behavior anywhere?  I guess it keeps Init() from being
called twice...  Is that something we really need to protect from at runtime in
releases?  Or just something to assert on?  If the latter, make this member
#ifdef DEBUG.

>Index: caps/include/nsScriptSecurityManager.h

>-#define DEBUG_CAPS_HACKER
>+//#define DEBUG_CAPS_HACKER

No more caps for you?  ;)

>Index: caps/src/nsPrincipal.cpp

>   // XXXcaa does this need to be threadsafe?  See bug 143559.
>-  nsrefcnt count = PR_AtomicIncrement((PRInt32 *)&mJSPrincipals.refcount);
>-  NS_LOG_ADDREF(this, count, "nsPrincipal", sizeof(*this));
>+  nsrefcnt count = mJSPrincipals.refcount++;

You don't need the intermediate "count" var here, do you?  Unless you log the
addref....  Why did you remove that logging?

Does the XXXcaa comment still apply now that we are not threadsafe?

>-  nsrefcnt count = PR_AtomicDecrement((PRInt32 *)&mJSPrincipals.refcount);
>-  NS_LOG_RELEASE(this, count, "nsPrincipal");

Again, why no more need for logging?

>+nsPrincipal::Init(const char *aCertID, nsIURI *aCodebase)
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  return rv;

Um.... If you want a warning when "rv" fails, just add a
NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "whatever") and don't have this wacky
two-return-statements-in-a-row thing.

>Index: caps/src/nsScriptSecurityManager.cpp

>+    mPrincipals.Get(certificate, getter_AddRefs(fromTable));
>+    if (fromTable) {
>+        if (!aURI) {

Could you please add a nice long comment explaining what this is trying to
accomplish? Something like our irc conversation, the salient points being:

*caillon* if we find one, then we need to clone that one because it was granted
extra
+priveleges, e.g. "UniversalBrowserRead", etc.
*caillon* or by signed scripts
-> *caillon* Since fromTable already has URI aURI... why would
InitFromPersistent not
+end up setting the same URI?
*caillon* because InitFromPersisten only deals with the "primary" identifier.
*caillon* in the case of a cert, that means the certificate id

>Index: content/base/src/nsScriptLoader.cpp

>+static already_AddRefed<nsIPrincipal>
>+IntersectPrincipalCerts(nsIPrincipal *aOld, nsIPrincipal *aNew)

Comment as to when this should be called and what it does?

r=bzbarsky with these nits picked.

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 130099 [details] [diff] [review]
Patch

I'll just add my nits.

> Index: caps/src/nsPrincipal.cpp
> ===================================================================

> +nsresult
> +nsPrincipal::SetCertificate(const char* aID, const char* aName)
>  {
> -  if (!aID) {
> -    mCert = nsnull;
> -    return NS_OK;
> +  NS_ENSURE_STATE(!mCert);
> +
> +  if (!aID && !aName) {
> +    return NS_ERROR_INVALID_POINTER;
>    }

  NS_ENSURE_ARG_POINTER(aID || aName);

> @@ -523,54 +542,53 @@ nsPrincipal::InitFromPersistent(const ch
>                                  const char* aDeniedList,
>                                  PRBool aIsCert,
>                                  PRBool aTrusted)

> +  rv = mJSPrincipals.Init(this, aToken);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
>    //-- Save the preference name
>    mPrefName = aPrefName;
>  
>    const char* ordinalBegin = PL_strpbrk(aPrefName, "1234567890");
>    if (ordinalBegin) {
>      PRIntn n = atoi(ordinalBegin);
>      if (sCapabilitiesOrdinal <= n) {
>        sCapabilitiesOrdinal = n + 1;
>      }
>    }
>  
>    //-- Store the capabilities
> -  nsresult rv = NS_OK;
> +  rv = NS_OK;

No need to reset rv here It hink.

> Index: caps/src/nsScriptSecurityManager.cpp
> ===================================================================

>  NS_IMETHODIMP
>  nsScriptSecurityManager::GetCertificatePrincipal(const char* aCertID,
> +                                                 nsIURI* aURI,
>                                                   nsIPrincipal **result)

>      // Check to see if we already have this principal.
>      nsCOMPtr<nsIPrincipal> fromTable;
> -    mPrincipals.Get(principal, getter_AddRefs(fromTable));
> -    if (fromTable)
> -        principal = fromTable;
> +    mPrincipals.Get(certificate, getter_AddRefs(fromTable));
> +    if (fromTable) {
> +        if (!aURI) {
> +            certificate = NS_STATIC_CAST(nsPrincipal*,
> +                                         NS_STATIC_CAST(nsIPrincipal*,
> +                                                        fromTable));

Why this casting stuff? Can't you just set *result to fromTable?

> +        } else {
> +            nsXPIDLCString prefName;
> +            nsXPIDLCString id;
> +            nsXPIDLCString granted;
> +            nsXPIDLCString denied;
> +            rv = fromTable->GetPreferences(getter_Copies(prefName),
> +                                           getter_Copies(id),
> +                                           getter_Copies(granted),
> +                                           getter_Copies(denied));
> +            if (NS_SUCCEEDED(rv)) {
> +                certificate = new nsPrincipal();
> +                if (!certificate)
> +                    return NS_ERROR_OUT_OF_MEMORY;
>  
> -    NS_ADDREF(*result = principal);
> +                rv = certificate->InitFromPersistent(prefName, id,
> +                                                     granted, denied,
> +                                                     PR_TRUE, PR_FALSE);
> +                if (NS_SUCCEEDED(rv))
> +                    certificate->SetURI(aURI);
> +            }
> +        }
> +    }
>  
> -    return NS_OK;
> +    NS_ADDREF(*result = certificate);
> +
> +    return rv;

seems this could simply be

if (!fromTable) {
  NS_ADDREF(*result = certificate);

  return NS_OK;
}

if (!aURI) {
  NS_ADDREF(*result = fromTable);

  return NS_OK;
}

...

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fix was checked in 10/21/2003 15:11.