Open Bug 1528272 Opened 6 years ago Updated 2 years ago

Move ContentPrincipal::mDomain to RealmPrivate

Categories

(Core :: Security: CAPS, enhancement)

enhancement

Tracking

()

People

(Reporter: jandem, Unassigned)

References

Details

A while ago bholley mentioned it would be nice to do this to make principals immutable.

I did an audit to look at the callers (it's possible I missed one though):

(1) nsHTMLDocument::GetDomainURI. This could get the domain from the RealmPrivate.

(2) nsScriptSecurityManager::HashPrincipalByOrigin. This is called by ContentPrincipal::GetHashValue and that's called here:

https://searchfox.org/mozilla-central/rev/38035ee92463c9e9fbca729ac7e66476ac7eb27a/gfx/thebes/gfxFontSrcPrincipal.cpp#20

Does this actually care about mDomain?

(3) GetPrincipalDomainOrigin. Called by nsScriptSecurityManager::CanCreateWrapper. This seems to be for error messages and we could get the domain from the context realm instead.

(4) ContentPrincipal::SubsumesInternal ConsiderDocumentDomain. This has callers:

(4a) FeaturePolicy::AllowsFeatureInternal and Feature::AllowListContains.

(4b) NullPrincipal::MayLoadInternal. Does this actually need to consider domain given a NullPrincipal is involved?

(4c) BasePrincipal::FastEqualsConsideringDomain:

  • BasePrincipal::EqualsConsideringDomain (unused?)
  • MaybeCrossOriginObjectMixins::IsPlatformObjectSameOrigin. This can get it from the realms involved.

(4d) BasePrincipal::FastSubsumesConsideringDomain:

  • BasePrincipal::SubsumesConsideringDomain -> Location::CallerSubsumes. This would need to accept a context or realm instead of the subject principal.

  • MaybeCrossOriginObjectMixins::IsPlatformObjectSameOrigin again.

  • AccessCheck::subsumesConsideringDomain. This one has access to both realms.

The "IgnoringFPD" case is similar.

(In reply to Jan de Mooij [:jandem] from comment #0)

(1) nsHTMLDocument::GetDomainURI. This could get the domain from the RealmPrivate.

We'd need to be a little careful about documents that don't have a JS object or Window or something... Not sure whether this can be reached for thos.

Does this actually care about mDomain?

Looks like no. It just cares about nsIPrincipal::Equals, which ignores domain. Arguably that code is actually wrong now (in the sense of not getting hashtable hits when it should).

(4b) NullPrincipal::MayLoadInternal. Does this actually need to consider domain given a NullPrincipal is involved?

It does not.

  • BasePrincipal::EqualsConsideringDomain (unused?)

It's used in caps/tests/unit/test_origin.js but looks like nowhere else.

  • BasePrincipal::SubsumesConsideringDomain -> Location::CallerSubsumes. This would need to accept a context or realm instead of the subject principal.

Also caps/tests/unit/test_origin.js again.

The FeaturePolicy thing ... I don't know the status of.

The biggest worry here is that in spec terms the domain stuff lives in an origin, which hangs off a document. So it's easy for specs (like feature policy) to try to rely on that...

That said, we could just store domain in a Document directly, in addition to storing it in the RealmPrivate. That should address at least the GetDomainURI bit and probably the feature policy bit.

There's another issue I just thought of: The setter would need to update all the documents that share the same origin per spec. So we may need a shared, heap-allocated, data structure that is pointed to by all the relevant documents and their RealmPrivates and stores the domain, if any. And we'd need to propagate that thing across loads so it can be inherited in all the places where we currently propagate principals. :(

Perhaps a simpler approach would be for principals to hold an immutable reference to a mutable object with the domain info, and then make all the getters and setters on that mutable object (i.e. the stuff delegated to by *ConsideringDomain) assert main thread?

I think that would be fine, yes. It would have the benefit of matching the spec model, too.

(In reply to Bobby Holley (:bholley) from comment #3)

Perhaps a simpler approach would be for principals to hold an immutable reference to a mutable object with the domain info, and then make all the getters and setters on that mutable object (i.e. the stuff delegated to by *ConsideringDomain) assert main thread?

I'm not sure if the separate object is necessary. We could continue to store the information directly within the nsIPrincipal, and just have that particular field be main-thread only. When creating a new nsIPrincipal from off-main-thread, we'd initialize mDomain to nullptr, and never touch it again. As nsIURI is already threadsafe, it's OK for the principal to be destroyed off-main-thread (as nothing on the main thread can be mutating during destruction).

I think the only necessary changes are to make the domain not be serialized when sending a nsIPrincipal over IPC, and to add documentation and assertions that the domain field is only ever accessed or mutated from the main thread.

The main downside here would be that principals have identity (although that identity is only relevant on the main thread), but it sounds like that may actually be necessary in order to implement the spec correctly.

That could work. I mostly just want to be sure that nothing off-main-thread accidentally touches mDomain. But we could put it in a newtype object that asserts main thread upon access.

Might be able to use mozilla::ThreadBound for that, if we always create principals on the main thread.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #7)

Might be able to use mozilla::ThreadBound for that, if we always create principals on the main thread.

We can also create them off-main-thread, so long as we always immediately transfer them to the main thread. It has some extra overhead though (like a mThread atomic member, and a mAccessCount atomic, which I believe are unnecessary).

ContentPrincipal::ContentPrincipal(...) {
  if (nsCOMPtr<nsIThread> mainThread = do_GetMainThread()) {
    PRThread* mainPRThread = nullptr;
    mainThread->GetPRThread(&mainPRThread);
    mDomain.Transfer(mainPRThread);
  }
}

mozilla::ThreadBound<nsCOMPtr<nsIURI>> mDomain;

Alternatively, I believe a small wrapper class which prevents reading or writing the value from off-main-thread would also be sound, given that nsIURI is safe to destroy on any thread.

class DomainWrapper final {
 public:
  nsCOMPtr<nsIURI>& GetOnMainThread() {
    MOZ_DIAGNOSTIC_ASSERT(NS_IsMainThread());
    return mDomain;
  }

 private:
  DomainWrapper(const DomainWrapper&) = delete;
  DomainWrapper& operator=(const DomainWrapper&) = delete;
  nsCOMPtr<nsIURI> mDomain;
};
DomainWrapper mDomain;
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.