Note: There are a few cases of duplicates in user autocompletion which are being worked on.

disallow inheriting of system principal in type=content docshells

RESOLVED FIXED in Firefox 11

Status

()

Core
Document Navigation
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

(Depends on: 1 bug)

Trunk
mozilla13
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, firefox13 fixed, firefox-esr1011+ fixed, blocking1.9.2 .28+, status1.9.2 .28-fixed)

Details

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

Attachments

(4 attachments, 1 obsolete attachment)

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.
Created attachment 594059 [details] [diff] [review]
patch
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #594059 - Flags: review?(bzbarsky)
Created attachment 594061 [details] [diff] [review]
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).
Attachment #594059 - Attachment is obsolete: true
Attachment #594059 - Flags: review?(bzbarsky)
Attachment #594061 - Flags: review?(bzbarsky)

Comment 3

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 6

6 years ago
\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 8

6 years ago
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 9

6 years ago
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+
https://hg.mozilla.org/releases/mozilla-beta/rev/56eb942d7c13
https://hg.mozilla.org/releases/mozilla-aurora/rev/07b25bee8bd1
status-firefox11: --- → fixed
status-firefox12: --- → fixed
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: --- → ?
status1.9.2: --- → wanted
status-firefox-esr10: --- → affected
status-firefox13: --- → fixed
tracking-firefox-esr10: --- → 11+

Updated

6 years ago
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+
Created attachment 601357 [details] [diff] [review]
ESR10 patch
Attachment #601357 - Flags: approval-mozilla-esr10?
Attachment #601357 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Attachment #594061 - Flags: approval-mozilla-esr10+
https://hg.mozilla.org/releases/mozilla-esr10/rev/c2b28e09323c
status-firefox-esr10: affected → fixed
Followup bustage fix for ESR10: https://hg.mozilla.org/releases/mozilla-esr10/rev/bc4a7e406d63
Created attachment 602502 [details] [diff] [review]
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.
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+
https://hg.mozilla.org/releases/mozilla-1.9.2/rev/855980ed722d
status1.9.2: wanted → .28-fixed
(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 :)

Comment 22

6 years ago
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

6 years ago
> 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.
Created attachment 603327 [details] [diff] [review]
inline the code for 1.9.2
Attachment #603327 - Flags: review?(bzbarsky)

Comment 27

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

r=me
Attachment #603327 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/releases/mozilla-1.9.2/rev/c00a4ef21ac2
You need to log in before you can comment on or make changes to this bug.