Last Comment Bug 723808 - disallow inheriting of system principal in type=content docshells
: disallow inheriting of system principal in type=content docshells
Status: RESOLVED FIXED
[sg:want P1][qa-]
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
Depends on: 728313
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-02 19:29 PST by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-03-06 11:37 PST (History)
10 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed
11+
fixed
.28+
.28-fixed


Attachments
patch (10.73 KB, patch)
2012-02-02 19:30 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
real patch (8.84 KB, patch)
2012-02-02 19:33 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
ESR10 patch (8.13 KB, patch)
2012-02-28 11:54 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review
1.9.2 patch (7.66 KB, patch)
2012-03-02 14:54 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
lukasblakk+bugs: approval1.9.2.28+
Details | Diff | Splinter Review
inline the code for 1.9.2 (1.16 KB, patch)
2012-03-06 10:05 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
bzbarsky: review+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-02 19:29:32 PST
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: gavin@gavinsharp.com] 2012-02-02 19:30:27 PST
Created attachment 594059 [details] [diff] [review]
patch
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-02 19:33:00 PST
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).
Comment 3 Boris Zbarsky [:bz] 2012-02-02 19:47:44 PST
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: gavin@gavinsharp.com] 2012-02-03 15:35:31 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fc8e624ab13
Comment 5 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-02-04 02:44:03 PST
https://hg.mozilla.org/mozilla-central/rev/1fc8e624ab13
Comment 6 Jesse Ruderman 2012-02-04 14:48:58 PST
\o/
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-14 15:15:12 PST
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] 2012-02-15 16:12:29 PST
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] 2012-02-15 17:37:01 PST
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 11 Daniel Veditz [:dveditz] 2012-02-16 16:29:01 PST
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-22 11:29:53 PST
Is there something QA can do to help verify this bug?
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-23 12:32:58 PST
verifiying bug 719994 is sufficient
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-28 11:54:17 PST
Created attachment 601357 [details] [diff] [review]
ESR10 patch
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-28 11:57:23 PST
https://hg.mozilla.org/releases/mozilla-esr10/rev/c2b28e09323c
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-28 14:59:28 PST
Followup bustage fix for ESR10: https://hg.mozilla.org/releases/mozilla-esr10/rev/bc4a7e406d63
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-02 14:54:36 PST
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.
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-03-05 11:38:18 PST
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: gavin@gavinsharp.com] 2012-03-05 16:31:17 PST
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) 2012-03-05 20:10:41 PST
(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).
Comment 21 Smokey Ardisson (offline for a while; not following bugs - do not email) 2012-03-05 22:55:47 PST
(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] 2012-03-06 04:39:43 PST
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 [:bz] 2012-03-06 08:18:47 PST
> 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) 2012-03-06 08:28:47 PST
(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: gavin@gavinsharp.com] 2012-03-06 10:02:36 PST
(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: gavin@gavinsharp.com] 2012-03-06 10:05:00 PST
Created attachment 603327 [details] [diff] [review]
inline the code for 1.9.2
Comment 27 Boris Zbarsky [:bz] 2012-03-06 11:32:55 PST
Comment on attachment 603327 [details] [diff] [review]
inline the code for 1.9.2

r=me
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-06 11:37:56 PST
https://hg.mozilla.org/releases/mozilla-1.9.2/rev/c00a4ef21ac2

Note You need to log in before you can comment on or make changes to this bug.