Closed Bug 723808 Opened 12 years ago Closed 12 years ago

disallow inheriting of system principal in type=content docshells

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13
Tracking Status
firefox11 --- fixed
firefox12 --- fixed
firefox13 --- fixed
firefox-esr10 11+ fixed
blocking1.9.2 --- .28+
status1.9.2 --- .28-fixed

People

(Reporter: Gavin, Assigned: Gavin)

References

Details

(Whiteboard: [sg:want P1][qa-])

Attachments

(4 files, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #594059 - Flags: review?(bzbarsky)
Attached patch real patchSplinter Review
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).
Attachment #594059 - Attachment is obsolete: true
Attachment #594059 - Flags: review?(bzbarsky)
Attachment #594061 - Flags: review?(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.
Attachment #594061 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fc8e624ab13
Flags: in-testsuite+
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/1fc8e624ab13
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
\o/
Whiteboard: [sg:want P1]
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.
Attachment #594061 - Flags: approval-mozilla-beta?
Attachment #594061 - Flags: approval-mozilla-aurora?
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.
Attachment #594061 - Flags: approval-mozilla-beta?
Attachment #594061 - Flags: approval-mozilla-beta-
Attachment #594061 - Flags: approval-mozilla-aurora?
Attachment #594061 - Flags: approval-mozilla-aurora-
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.
Attachment #594061 - Flags: approval-mozilla-beta-
Attachment #594061 - Flags: approval-mozilla-beta+
Attachment #594061 - Flags: approval-mozilla-aurora-
Attachment #594061 - Flags: approval-mozilla-aurora+
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.
blocking1.9.2: --- → ?
Depends on: 728313
Is there something QA can do to help verify this bug?
Whiteboard: [sg:want P1] → [sg:want P1][qa?]
verifiying bug 719994 is sufficient
Whiteboard: [sg:want P1][qa?] → [sg:want P1][qa-]
blocking1.9.2: ? → .28+
Attachment #594061 - Flags: approval-mozilla-esr10?
Attachment #594061 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Attached patch ESR10 patchSplinter Review
Attachment #601357 - Flags: approval-mozilla-esr10?
Attachment #601357 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Attachment #594061 - Flags: approval-mozilla-esr10+
Attached patch 1.9.2 patchSplinter Review
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.
Attachment #602502 - Flags: approval1.9.2.28?
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! :)
Attachment #602502 - Flags: approval1.9.2.28? → approval1.9.2.28+
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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).
(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 :)
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.
> 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...
(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.
(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.
Attachment #603327 - Flags: review?(bzbarsky)
Comment on attachment 603327 [details] [diff] [review]
inline the code for 1.9.2

r=me
Attachment #603327 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.