723808 - disallow inheriting of system principal in type=content docshells

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

We've had a number of bugs over the years that were the result of somehow managing to load principal-inheriting URIs (data: and javascript:) on chrome-privileged pages loaded in the browser content area (e.g. about:config). Now that we have more and more chrome-privileged "in-content" pages, that's even more likely to be possible. As a precautionary measure, it makes sense to disallow these loads from inheriting the system principal - it seems quite unlikely that there are any valid use cases for this.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: real patch

oops, uploaded the wrong patch.

Ran into a couple of tests that depend on this, but none for good reason (and this is unlikely to occur in practice outside of the test harness).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 594061 [details] [diff] [review]
real patch

The setTimeout in that last test needs to happen from onload, not sync from script execution.

r=me with that fixed.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fc8e624ab13

Comment 5

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1fc8e624ab13

Comment 6

[Jesse Ruderman]

\o/

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 594061 [details] [diff] [review]
real patch

[Approval Request Comment]
This patch:
- has been on m-c for 10 days, with no reported regressions
- is moderately risky: it blocks certain types of loads that are quite unlikely to be user-triggered, but could potentially affect extensions doing weird things.

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 594061 [details] [diff] [review]
real patch

[Triage Comment]
Based upon the moderate risk evaluation due to possible add-on regressions caused by this patch, we'd prefer this rides the trains. Although any regressions sound like they'd be related to add-on misbehavior, we know that add-on blocklisting post-release is a pain. Let's try to avoid that.

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 594061 [details] [diff] [review]
real patch

[Triage Comment]
Upon further review and discussions with Gavin, this is only risky in terms of scope. We feel that the testing received in Firefox 11 beta 3 and up (along with Aurora 12 and m-c 13) will uncover regressions if caused by this bug. Approved.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-beta/rev/56eb942d7c13
https://hg.mozilla.org/releases/mozilla-aurora/rev/07b25bee8bd1

Comment 11

[Daniel Veditz [:dveditz]]

we'll want to include this on the ESR for consistency with 10. Potentially 1.9.2 has the same problem, we'll have to think about whether it's worth taking at this point in the lifecycle.

Comment 12

[u279076]

Is there something QA can do to help verify this bug?

Comment 13

[:Gavin Sharp [email: [e-mail]]]

verifiying bug 719994 is sufficient

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Attachment: ESR10 patch

Comment 15

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/c2b28e09323c

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Followup bustage fix for ESR10: https://hg.mozilla.org/releases/mozilla-esr10/rev/bc4a7e406d63

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Attachment: 1.9.2 patch

Changes from the trunk version:
- browser_463205.js doesn't exist on 1.9.2 (no need to fix it)
- need to use PRBool instead of bool
- chrome test harness on 1.9.2 requires use of packed.js, and explicit calls to enablePrivilege
- I removed one of the tests that I added that wasn't strictly related to this change, since I couldn't figure out how to get it to work on 1.9.2 (it passed, but it wasn't effective at catching a bug I introduced in the code). The rest of the tests work fine locally.

I wish I could push this to try, but try is missing ~3000 changesets from 1.9.2, and I don't think its build configuration would work with 1.9.2 builds.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 602502 [details] [diff] [review]
1.9.2 patch

Let's get this landed and see how the builds & tests turn out.  Sorry there's no try for 1.9.2 but soon enough we won't need that, so yay! :)

Comment 19

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-1.9.2/rev/855980ed722d

Comment 20

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #19)
> https://hg.mozilla.org/releases/mozilla-1.9.2/rev/855980ed722d

That changeset broke shared builds (e.g., my daily development debug build, Camino's cb-mini* tindeboxen) with 

Undefined symbols:
  "nsContentUtils::IsSystemPrincipal(nsIPrincipal*)", referenced from:
      nsDocShell::GetInheritedPrincipal(int)    in nsDocShell.o

linking libdocshell.dylib.

I "fixed" that error by adding 
$(DEPTH)/content/base/src/$(LIB_PREFIX)gkconbase_s.$(LIB_SUFFIX) \
to docshell/build/Makefile.in's SHARED_LIBRARY_LIBS (and the equivalent directive to LOCAL_INCLUDES; not sure if that was needed or not, but it didn't make any difference).

That, however, led to 440 lines of undefined symbol spew (see http://pastebin.mozilla.org/1504374) :(

Worse, when I tried to perform the same trick to resolve the first unresolved symbol in that list of 440, I ended up with 790 lines of unresolved symbol spew :(

I'm over my head here in terms of figuring out the right way to resolve these symbols correctly in shared builds; obviously continuing to add shared libs for each unresolved symbol is not it :(  Someone more familiar with the build system and dependencies needs to look at it.

This really is a blocker and needs to be resolved or backed out (in the short term, I can back this out locally to be able to continue to debug, but it's not a solution for more than a couple of days).

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #20)
> This really is a blocker and needs to be resolved or backed out (in the
> short term, I can back this out locally to be able to continue to debug, but
> it's not a solution for more than a couple of days).

A) Reading back over this, it sounds a lot more stringent and hand-wavy than I intended (the unfortunate side effects of spending an evening staring at linker errors rather than reviewing patches)

B) For anyone who wasn't on irc, Gavin says he has a fix that he'll land tomorrow morning sometime after builds start or something like that :)

Comment 22

[Axel Hecht [:Pike]]

To clarify, this means that we can't use bookmarklets on chrome pages no more?

I'm asking because I've used that as a hack to get good dialog sizes for l10n, see https://wiki.mozilla.org/L10n:Dialog_sizes.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> That changeset broke shared builds

Er... do we still support those?  I was under the impression that we could assume that docshell is linked into libxul...

Comment 24

[Mark Banner (:standard8)]

(In reply to Boris Zbarsky (:bz) from comment #23)
> > That changeset broke shared builds
> 
> Er... do we still support those?  I was under the impression that we could
> assume that docshell is linked into libxul...

On 1.9.2 builds yes we do. We currently can't go for the next Thunderbird build as the test builders are busted and hence we have no test results. Although we shipped Thunderbird as static, there was no way in that timeframe that we were able to shift to libxul.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

(In reply to Axel Hecht [:Pike] from comment #22)
> To clarify, this means that we can't use bookmarklets on chrome pages no
> more?

Yes.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Attachment: inline the code for 1.9.2

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 603327 [details] [diff] [review]
inline the code for 1.9.2

r=me

Comment 28

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-1.9.2/rev/c00a4ef21ac2