775713 - nsStrictTransportSecurityService should use Principal to access the PermissionManager

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

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

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +602,5 @@
>  
>      // Use whatever we ended up with, which defaults to UNKNOWN_ACTION.
>      return NS_OK;
>  }
> +

namespace {

@@ +603,5 @@
>      // Use whatever we ended up with, which defaults to UNKNOWN_ACTION.
>      return NS_OK;
>  }
> +
> +/* static */ nsresult

remove /* static */

@@ +615,5 @@
> +      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> +   NS_ENSURE_SUCCESS(rv, rv);
> +
> +   return securityManager->GetNoAppCodebasePrincipal(aURI, aPprincipal);
> +}

} // unnamed namespace.

::: security/manager/boot/src/nsStrictTransportSecurityService.h
@@ +124,5 @@
> +  /**
> +   * Returns a principal (aPrincipal) corresponding to aURI.
> +   * This is used to interact with the permission manager.
> +   */
> +  static nsresult GetPrincipalForURI(nsIURI* aURI, nsIPrincipal** aPrincipal);

Remove this declaration. This does not need to be a member function of nsSTSS.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

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

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +608,5 @@
> +GetPrincipalForURI(nsIURI* aURI, nsIPrincipal** aPrincipal)
> +{
> +   // The permission manager wants a principal but don't actually check a
> +   // permission but a data we saved in the permission manager so we are good by
> +   // creating a no-app codebase principal and send it to the permission manager.

Replace this comment with this one:

// We want all apps to share HSTS state, so this is one
// of the few places where we do not silo persistent
// state by extended origin.

Comment 3

[Honza Bambas (:mayhemer)]

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

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

This should, from fairness, land (and also merge) after the patch for bug 760307 since this one is much more simpler.  But depends on priorities.

::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
@@ +614,5 @@
> +   nsCOMPtr<nsIScriptSecurityManager> securityManager =
> +      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> +   NS_ENSURE_SUCCESS(rv, rv);
> +
> +   return securityManager->GetNoAppCodebasePrincipal(aURI, aPprincipal);

typo: aPprincipal

What exactly GetNoAppCodebasePrincipal does?  It probably still just sits in some patch yet landed that is not listed in deps, so I cannot do the proper review here.

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> Comment on attachment 643986 [details] [diff] [review]
> Patch
> 
> Review of attachment 643986 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This should, from fairness, land (and also merge) after the patch for bug
> 760307 since this one is much more simpler.  But depends on priorities.
> 
> ::: security/manager/boot/src/nsStrictTransportSecurityService.cpp
> @@ +614,5 @@
> > +   nsCOMPtr<nsIScriptSecurityManager> securityManager =
> > +      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> > +   NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +   return securityManager->GetNoAppCodebasePrincipal(aURI, aPprincipal);
> 
> typo: aPprincipal
> 
> What exactly GetNoAppCodebasePrincipal does?  It probably still just sits in
> some patch yet landed that is not listed in deps, so I cannot do the proper
> review here.

It's like getCodebasePrincipal(), see bug 758258. Basically, we have getAppCodebasePrincipal() and getNoAppCodebasePrincipal() to get a principal with app info or not. GetNoAppCodebasePrincipal() is exactly siimilar to getCodebasePrincipal() because it has no app info. However, we are going to remove getCodebasePrincipal() to make sure people know what they are doing.

Comment 5

[Mounir Lamouri (:mounir)]

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

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

I fixed the typo and apply bsmith review comments locally.

Comment 6

[Honza Bambas (:mayhemer)]

Mounir, can you please add dep bug where GetNoAppCodebasePrincipal() has been introduced?

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Mounir Lamouri (:mounir) from comment #5)
> I fixed the typo and apply bsmith review comments locally.

Can you please submit the newer patch?

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/09d63c143799