1328653 - Consider merging all the various *OriginAttributes to just one

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

We have at least
PrincipalOriginAttributes, DocShellOriginAttributes, NeckoOriginAttributes which have some
Inherit* methods.

If you look at those methods, they are identical, except in one case
http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/caps/BasePrincipal.cpp#37-49
http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/caps/BasePrincipal.cpp#51-62
http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/caps/BasePrincipal.cpp#71-82
http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/caps/BasePrincipal.cpp#84-95

The special case is http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/caps/BasePrincipal.cpp#84-95
which has two interesting callers where the second param might be true. Perhaps we could just have a method for those cases?

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

For the background this is done in Bug 1209162, at that time it's possible that the child iframe won't inherit the origin attributes from parent document in some cases in the FirefoxOS new security model project, although now the container project and the firstPartyIsolation won't have this requirement.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: oa.patch

I think this is a nice improvement of our code base. I understand why we had that setup (DocShell/Principal/Necko/GenericOriginAttributes) but I think we don't need this level of complexity today.

I also done some little changes:

1. OriginAttributes::Inherit() instead multiple inherit methods.
2. OriginAttributes::StripAddonId()
3. Only 3 CTORs: no params, appid + isolatedmozBrowser, originAttributesDictionary. all of them public.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that while the different types contain the same members, they are actually used very differently.

In particular, DocshellOriginAttributes doesn't contain the correct mFirstPartyDomain information since the toplevel docshell can't correctly track the first-party-domain in its originAttributes (since there can be multiple differen first-party-domains active in a toplevel docshell at once).

Hence it's very useful to use the type system to ensure that we statically track when we need to grab the first-party information from elsewhere, and when not.

Comment 4

[Dave Huseby [:huseby]]

Comment on attachment 8824082 [details] [diff] [review]
oa.patch

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

::: caps/BasePrincipal.cpp
@@ +34,5 @@
>  
>  using dom::URLParams;
>  
>  void
> +OriginAttributes::StripUserContextIdAndFirstPartyDomain()

Two thoughts:
1) maybe make a single "strip" function that takes flags for which ones to reset?  StripAttributes(USER_CONTEXT_ID | FIRST_PARTY_DOMAIN)
2) if we keep these specialized functions, we should probably at least move them to the header and make them inline.

@@ +47,5 @@
> +  mAddonId.Truncate();
> +}
> +
> +void
> +OriginAttributes::Inherit(const OriginAttributes& aAttrs)

Inherit could take bitflags, similar to the strip function I mention above, and the flags specify which attributes to inherit and which ones to reset.

::: caps/BasePrincipal.h
@@ +43,5 @@
> +  void Inherit(const OriginAttributes& aAttrs);
> +
> +  void SetFirstPartyDomain(const bool aIsTopLevelDocument, nsIURI* aURI);
> +
> +  void StripUserContextIdAndFirstPartyDomain();

if you keep these functions, i think you should make them inline them.

@@ +45,5 @@
> +  void SetFirstPartyDomain(const bool aIsTopLevelDocument, nsIURI* aURI);
> +
> +  void StripUserContextIdAndFirstPartyDomain();
> +
> +  void StripAddonId();

inline

::: caps/nsScriptSecurityManager.cpp
@@ +299,5 @@
>                  nsNullPrincipal::CreateWithInheritedAttributes(loadInfo->LoadingPrincipal());
>              } else {
> +              OriginAttributes attrs;
> +              loadInfo->GetOriginAttributes(&attrs);
> +              attrs.StripAddonId();

attrs.Strip(OriginAttributes::ADDON_ID)

?

@@ +403,4 @@
>  
>      // For addons loadInfo might be null.
>      if (loadInfo) {
> +      attrs.Inherit(loadInfo->GetOriginAttributes());

// inherit all origin attributes except ADDON_ID
attrs.Inherit(loadInfo->GetOriginAttributes(), ~OriginAttributes::ADDON_ID);

?

::: docshell/base/nsDocShell.cpp
@@ +12289,5 @@
>            } else {
>              // get the OriginAttributes
> +            OriginAttributes attrs;
> +            loadInfo->GetOriginAttributes(&attrs);
> +            attrs.StripAddonId();

Strip(flags)?

@@ +14606,5 @@
>    }
>  
>    if (aIsNonSubresourceRequest) {
> +    OriginAttributes attrs;
> +    attrs.Inherit(mOriginAttributes);

Inherit(OA, flags)?

@@ +14658,5 @@
>  
>    bool isReload = mLoadType & LOAD_CMD_RELOAD;
>  
> +  OriginAttributes attrs;
> +  attrs.Inherit(mOriginAttributes);

Inherit(OA, flags)?

::: netwerk/base/nsSocketTransport2.h
@@ +312,1 @@
>      

please remove whitespace

::: netwerk/cookie/nsCookieService.cpp
@@ +2359,5 @@
>    return NS_NewArrayEnumerator(aEnumerator, cookieList);
>  }
>  
>  static nsresult
> +InitializeOriginAttributes(OriginAttributes* aAttrs,

can this be "OriginAttributes const *" ?

@@ +2434,5 @@
>                             bool              aIsSecure,
>                             bool              aIsHttpOnly,
>                             bool              aIsSession,
>                             int64_t           aExpiry,
> +                           OriginAttributes* aOriginAttributes)

can this be "OriginAttributes const *" ?

@@ +2558,5 @@
>  nsCookieService::RemoveNative(const nsACString &aHost,
>                                const nsACString &aName,
>                                const nsACString &aPath,
>                                bool aBlocked,
> +                              OriginAttributes* aOriginAttributes)

can this be "OriginAttributes const *" ?

::: netwerk/ipc/NeckoParent.cpp
@@ +831,5 @@
>  
>    // We only actually care about the loadContext.mPrivateBrowsing, so we'll just
>    // pass dummy params for nestFrameId, and originAttributes.
>    uint64_t nestedFrameId = 0;
> +  OriginAttributes attrs(NECKO_UNKNOWN_APP_ID, false);

is this constant necessary?  i noticed we have a NO_APP_ID in the nsScriptSecurity manager too.  maybe combine them into an OA constant like OriginAttributes::NO_APP_ID ?

@@ +864,5 @@
>  
>    // We only actually care about the loadContext.mPrivateBrowsing, so we'll just
>    // pass dummy params for nestFrameId, and originAttributes;
>    uint64_t nestedFrameId = 0;
> +  OriginAttributes attrs(NECKO_UNKNOWN_APP_ID, false);

same as above

::: netwerk/protocol/http/TunnelUtils.cpp
@@ +1595,5 @@
>  FWD_TS_PTR(GetRecvBufferSize, uint32_t);
>  FWD_TS(SetRecvBufferSize, uint32_t);
>  
>  nsresult
> +SocketTransportShim::GetOriginAttributes(mozilla::OriginAttributes* aOriginAttributes)

can this be "mozilla::OriginAttributes const *" ?

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3085,5 @@
>      }
>  
>      socketTransport->SetConnectionFlags(tmpFlags);
>  
> +    OriginAttributes originAttributes = mEnt->mConnInfo->GetOriginAttributes();

since you're making a copy we should do:

const OriginAttributes originAttributes =

or maybe even a const OriginAttributes &

::: security/manager/ssl/TransportSecurityInfo.h
@@ +70,3 @@
>  
>    PRErrorCode GetErrorCode() const;
>    

please include a separate patch that cleans up the extra whitespace in this file.

Comment 5

[Dave Huseby [:huseby]]

This is an amazing cleanup.  Good job.  There's a couple files you touched that have extra whitespace in them.  Please submit a separate patch that cleans up the whitespace.  This is great.  Much cleaner.

Comment 6

[Andrea Marchesini [:baku]]

> Two thoughts:

I like the StripAttributes();

> // inherit all origin attributes except ADDON_ID
> attrs.Inherit(loadInfo->GetOriginAttributes(), ~OriginAttributes::ADDON_ID);

All our Inherit calls strip addonId. Having an additional param can generate error eventually.
Plus addonID is going away.

> >  static nsresult
> > +InitializeOriginAttributes(OriginAttributes* aAttrs,
> 
> can this be "OriginAttributes const *" ?

No, it calls Init() and it is not const.

> > +  OriginAttributes attrs(NECKO_UNKNOWN_APP_ID, false);
> 
> is this constant necessary?  i noticed we have a NO_APP_ID in the
> nsScriptSecurity manager too.  maybe combine them into an OA constant like
> OriginAttributes::NO_APP_ID ?

Follow up?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe52da5cb90
Merging all the various *OriginAttributes to just one, r=huseby

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8fe52da5cb90

Comment 9

[Jorg K (CEST = GMT+2)]

Greetings from your friends at Thunderbird and SeaMonkey:
https://hg.mozilla.org/comm-central/rev/112f41f81a4115e6f0f130c78a77c0782f33b01d

Comment 10

[Tim Huang[:timhuang]]

Hi, Baku. It is confusing that when and where should we use inherit() since there is only one type of originAttributes after this bug. Do you have any guideline about this? Do we have to use inherit every time when we pass originAttributes across different modules, like from docShell to Necko?

Comment 11

[Andrea Marchesini [:baku]]

> originAttributes across different modules, like from docShell to Necko?

Exactly. We should not use OriginAttribute copy CTOR. I'll file a bug for this.
Unfortunately, we need to keep Inherit() until bug 1314361 is resolved.