Closed Bug 1011024 Opened 9 years ago Closed 9 years ago

Fix nsScriptSecurityManager::GetChannelPrincipal so that it doesn't fail to get the correct nsIPrincipal for some resource documents

Categories

(Core :: DOM: Core & HTML, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file, 1 obsolete file)

If I add an nsIDocument::AllowXULXBL() call to nsDocumentViewer::CreateStyleSet I get a debug abort in nsPrincipal::GetAppId (because the mAppId is UNKNOWN_APP_ID) when running reftests/svg/use-01-extref.svg:

  nsPrincipal::GetAppId
  nsPermissionManager::CommonTestPermission
  nsPermissionManager::TestPermissionFromPrincipal
  TestSitePerm
  nsContentUtils::IsSitePermAllow
  nsContentUtils::AllowXULXBLForPrincipal
  nsDocument::InternalAllowXULXBL
  nsIDocument::AllowXULXBL
  nsIDocument::LoadsFullXULStyleSheetUpFront
  nsDocumentViewer::CreateStyleSet
  nsDocumentViewer::InitPresentationStuff

The test loads use-01-extref-resource.svg as a resource document, which in turn loads the test's own file (use-01-extref.svg) as a nested resource document. So we end up with:

  file:///.../layout/reftests/svg/use-01-extref.svg
    file:///.../layout/reftests/svg/use-01-extref-resource.svg
      file:///.../layout/reftests/svg/use-01-extref.svg

The nested instance of use-01-extref.svg has its document nsIPrincipal set under nsDocument::Reset after that function calls nsScriptSecurityManager::GetChannelPrincipal to get the nsIPrincipal from the nsIChannel (nsFileChannel) that is passed in.

  nsScriptSecurityManager::GetChannelPrincipal
  nsDocument::Reset
  mozilla::dom::XMLDocument::Reset
  nsDocument::StartDocumentLoad
  mozilla::dom::XMLDocument::StartDocumentLoad
  nsContentDLF::CreateDocument
  nsContentDLF::CreateInstance
  nsExternalResourceMap::PendingLoad::SetupViewer
  nsExternalResourceMap::PendingLoad::OnStartRequest
  nsBaseChannel::OnStartRequest
  nsInputStreamPump::OnStateStart
  nsInputStreamPump::OnInputStreamReady
  nsInputStreamReadyEvent::Run

Turns out that channel doesn't have an owner, or an nsIDocShell, though, so GetChannelPrincipal uses UNKNOWN_APP_ID the principal that it returns, which is what causes us to abort later in nsPrincipal::GetAppId().

I'm not sure what the expected behavior is here, but I'm guessing that the entries created for an nsExternalResourceMap should be using an nsFileChannel (nsIRequest/nsIChannel) which has its owner set to the embedding document, or maybe even the mDisplayDocument ancestor, if that's a different document.
Summary: Figure out why resource documents may not have a useful nsIPrinciple → Figure out why resource documents may not have a useful nsIPrincipal
The problem is that nsScriptSecurityManager::GetChannelPrincipal is broken.  It's trying to query a docshell from notification callbacks, which is explicitly NOT guaranteed.  What's guaranteed is an nsILoadContext.

Jonathan, are you up for trying to fix this stuff?
Flags: needinfo?(jwatt)
Attached patch patch (obsolete) — Splinter Review
Like this?

Note that we don't seem to always get a nsILoadContext. Specifically I noticed that we failed to get one when loading chrome://global/skin/toolbar.css and chrome://global/skin/menu.css with the following stack:

  nsScriptSecurityManager::GetChannelPrincipal
  mozilla::css::SheetLoadData::OnStreamComplete
  nsUnicharStreamLoader::OnStopRequest
  nsSyncLoadService::PushSyncStreamToListener
  mozilla::css::Loader::LoadSheet
  mozilla::css::Loader::InternalLoadNonDocumentSheet
  mozilla::css::Loader::LoadSheetSync
  mozilla::css::Loader::LoadSheetSync
  nsXBLResourceLoader::LoadResources
  nsXBLPrototypeResources::LoadResources
  nsXBLPrototypeBinding::LoadResources
  nsXBLService::GetBinding
  nsXBLService::GetBinding
  nsXBLService::GetBinding
  nsXBLService::LoadBindings
  nsCSSFrameConstructor::AddFrameConstructionItemsInternal
  nsCSSFrameConstructor::AddFrameConstructionItems
  nsCSSFrameConstructor::ProcessChildren
  nsCSSFrameConstructor::ConstructDocElementFrame
  nsCSSFrameConstructor::ContentRangeInserted
  nsCSSFrameConstructor::ContentInserted
  PresShell::Initialize
  mozilla::dom::XULDocument::StartLayout
  mozilla::dom::XULDocument::DoneWalking
  mozilla::dom::XULDocument::ResumeWalk
  mozilla::dom::XULDocument::OnScriptCompileComplete
  NotifyOffThreadScriptCompletedRunnable::Run
  nsThread::ProcessNextEvent

Hence I left the nsIDocShell code in there.
Attachment #8423941 - Flags: review?(bzbarsky)
Flags: needinfo?(jwatt)
> Hence I left the nsIDocShell code in there.

Are you getting a _docshell_ in the sync-xbl-load case?
Oh, no, I am not, sorry for the confusion. I can remove that code.
Attached patch patchSplinter Review
Attachment #8423941 - Attachment is obsolete: true
Attachment #8423941 - Flags: review?(bzbarsky)
Attachment #8423967 - Flags: review?(bzbarsky)
Comment on attachment 8423967 [details] [diff] [review]
patch

r=me, but:

1)  Please mark those two properties as [infallible] in the IDL so you don't have to mess with the out params.

2)  nsDocument::ResetToURI should use the new thing, since we have that now.
Attachment #8423967 - Flags: review?(bzbarsky) → review+
Blocks: 1008455
(In reply to Boris Zbarsky [:bz] from comment #6)
> 1)  Please mark those two properties as [infallible] in the IDL so you don't
> have to mess with the out params.

As noted on IRC we can only do that for [builtinclass], which this is not.
Summary: Figure out why resource documents may not have a useful nsIPrincipal → Fix nsScriptSecurityManager::GetChannelPrincipal so that it doesn't fail to get the correct nsIPrincipal for some resource documents
https://hg.mozilla.org/mozilla-central/rev/73cd98bb4f40
Assignee: nobody → jwatt
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.