1405195 - Web content can specify arbitrary triggering principal for images

Status: RESOLVED FIXED

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

Description

[Kris Maglione [:kmag]]

We currently allow web content to specify any arbitrary triggering principal for image loads by adding a loadingprincipal attribute with a serialized principal blob:

http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/dom/base/nsContentUtils.cpp#10452-10463

The code asserts that it's only ever used in system principal documents, but that assertion only runs in debug builds.

I don't know if this vulnerability existed before bug 1376971, but I recall seeing that code before and verifying that it was safe. It is definitely not safe now, though. I've confirmed that setting this attribute in ordinary web content load the image with the given principal.

Comment 1

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

Thanks for filing this, my bad.

Should I change to MOZ_RELEASE_ASSERT?
Or what should we do when we found out content is trying to do so?

Comment 2

[Kris Maglione [:kmag]]

We should probably just ignore it if the document principal is not the system principal. Crashing just because some content page adds a `loadingprincipal` attribute to an image doesn't sound like a great user experience...

Comment 3

[Kris Maglione [:kmag]]

Also probably worth noting that we shouldn't even try to deserialize the object if the document doesn't have the system principal. The deserialization code is not really hardened enough to be exposed to the web.

Comment 4

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

Attachment: Patch.

Comment 6

[Kris Maglione [:kmag]]

Adding bz, since he'll almost certainly also notice this when he reviews bug 1406278.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Wait.  Why are we wasting time getting the attribute in the common, non-systemprincipal, case?

Why is any of this code running at all in the non-systemprincipal case?

Why does GetLoadingPrincipalForXULNode have a misleading name?  It has nothing whatsoever to do with "XUL"; it never checks for "XUL", and it works on all elements.  Maybe the name sort of made sense as long as nsImageBoxFrame was the only consumer, but the changes to use this in nsImageLoadingContent and HTMLMediaElement clearly have nothing to do with "XUL".

Why does the documentation for GetLoadingPrincipalForXULNode talk about "if aLoadingPrincipal has 'loadingprincipal' attributes"?  aLoadingPrincipal is an out param, and principals can't have attributes.

Please file a followup bug to clean this stuff up to be sane.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Also, GetLoadingPrincipalForXULNode is getting a triggering principal, not a loading principal....

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90b9ac6020e9301db7c064ce641ad961b8d1134e

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 58, just to make sure it sticks.

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/90b9ac6020e9

Comment 12

[Kris Maglione [:kmag]]

This shouldn't have landed without sec-approval, especially not with such a descriptive commit message. :(

But at least it hasn't made it to a release channel yet...

Comment 13

[Ryan VanderMeulen [:RyanVM]]

The description made it sound like this only affected trunk. Is that not the case? If so, please update the status flags accordingly.

Comment 14

[Kris Maglione [:kmag]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
> The description made it sound like this only affected trunk. Is that not the
> case? If so, please update the status flags accordingly.

Like I said in comment 0, I'm not sure that this wasn't a problem before bug 1376971, just that bug 1376971 made it worse. The previous code was already pretty suspect, so I would have liked a sec analyst to make the call on whether this was safe to land.

Either way, the commit message painted a clear bulls-eye on the issue, which is undesirable even if it does only affect trunk.

Comment 15

[Boris Zbarsky [:bzbarsky]]

So...  The GetContentPolicyTypeForUIImageLoading function was added in bug 1319908 and hence dates back to Firefox 53.  But it just moved around code that was added in bug 1277803 (Firefox 52).  All that code was totally unsafe if a content node was involved, but was restricted to nsImageBoxFrame (Firefox 52) or Mac menu items (Firefox 53) so wasn't a problem unless remote XUL was enabled.

Bug 1376971 started using this totally-unsafe function from code exposed to the web.  So this _was_ a nightly-only regression, though checking that it is is not trivial.

Andrea, Yoshi, I would really appreciate a response to comment 7.

Comment 16

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

Sorry for the late response, was on holiday in the past week.
I filed bug 1407498 for this.