Re: Intent to implement and ship: CSP exemptions for content injected by privileged callers

[Kris Maglione]

On Mon, Oct 02, 2017 at 09:07:09PM -0400, Boris Zbarsky wrote:

On 10/2/17 5:35 PM, Kris Maglione wrote:
So far it doesn't look like there's any significant difference on 
any talos test from adding [NeedsCallerPrincipal] to 
setAttribute/setAttributeNS/Attr.value,


OK.  That's a minimum bar, obviously, but I would still like us to 
measure what the (presumably nonzero) impact is, so we know what we're 
dealing with.  In particular, setAttribute may simply not be a huge 
part of most talos tests.


What I'd like to see is how long setAttribute() with some meaningless 
name like "testing" takes with/without this change on an 
HTMLDivElement in both the "in document" and "out of document" cases.  
Just a silly microbenchmark is ok for this purpose; it at least gives 
us an idea of what happens in the L2-cache-hit case.


For the pretty simple micro-benchmark below, here are the 
in-document and out-of-document numbers for three runs without 
the subject principal:


1260ns 1820ns
1260ns 1870ns
1014ns 1332ns

and three runs with:

1002ns 1470ns
1280ns 1722ns
1207ns 1898ns

The difference between the two is within the noise.


Without inserting the div into the document, the numbers were 
closer to 500ns, with or without the subject principal, in- or 
out-of-document.


I haven't tested a sandbox with X-rays, but I'm willing to bet 
X-ray overhead completely overwhelms any other meaningful metric 
in that case.




 
 
   "use strict";

   function run(doc) {
 let div = document.createElement("div");
 doc.body.appendChild(div);

 let iterations = 1024 * 1024;

 let start = performance.now();
 for (let i = 0; i < iterations; i++) {
   div.setAttribute("data-num", i);
 }
 let end = performance.now();
 let delta = (end - start) * 1000 * 1000;

 doc.body.appendChild(document.createTextNode(`Per-iteration: 
${Math.round(delta / iterations)}ns`));
   }

   let iframe = document.querySelector("iframe");
   iframe.addEventListener("load", () => {
 run(iframe.contentDocument);
   }, {once: true});

   window.addEventListener("load", () => {
 run(document);
   }, {once: true});
 

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to implement and ship: CSP exemptions for content injected by privileged callers

[Boris Zbarsky]

On 10/2/17 5:35 PM, Kris Maglione wrote:
So far it doesn't look like there's any significant difference on any 
talos test from adding [NeedsCallerPrincipal] to 
setAttribute/setAttributeNS/Attr.value,


OK.  That's a minimum bar, obviously, but I would still like us to 
measure what the (presumably nonzero) impact is, so we know what we're 
dealing with.  In particular, setAttribute may simply not be a huge part 
of most talos tests.


What I'd like to see is how long setAttribute() with some meaningless 
name like "testing" takes with/without this change on an HTMLDivElement 
in both the "in document" and "out of document" cases.  Just a silly 
microbenchmark is ok for this purpose; it at least gives us an idea of 
what happens in the L2-cache-hit case.


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to implement and ship: CSP exemptions for content injected by privileged callers

[Kris Maglione]

On Mon, Oct 02, 2017 at 11:39:21AM -0700, Kris Maglione wrote:

On Mon, Oct 02, 2017 at 11:13:20AM -0400, Boris Zbarsky wrote:
Passing along a JSContext would work.  We could have something like 
"null means no scripted caller, otherwise caller's compartment is 
the part that matters".  This relies on no one on the setattr path 
messing with the compartment, but that shouldn't be too hard to 
ensure, especially since we only have a few attributes on a few 
elements for which this is relevant...


I'd love it if we could pass along something that couldn't be 
abused/misused like a JSContext.  We could make up a wrapper class, 
but no matter what we do we'd have the fundamental tradeoff that 
either we grab the principal eagerly, and pay the cost for all the 
cases where it doesn't matter, or we grab it lazily and run the risk 
of thing changing under us.  We should probably measure how 
expensive setAttribute is and how expensive grabbing the principal 
from a JSContext (e.g. by marking the method as 
[NeedsCallerPrincipal]) is...


OK, I'll try a talos run with [NeedsCallerPrincipal] added to 
setAttribute and see where that comes out. If it looks good, I'll 
investigate that route some more. Otherwise, I'll probably go with a 
JSContext wrapper and retrieve the principal on demand.


So far it doesn't look like there's any significant difference 
on any talos test from adding [NeedsCallerPrincipal] to 
setAttribute/setAttributeNS/Attr.value, so I'm going to go that 
route for now. If it turns out to be a problem later, I'll 
refactor it to pass a context object that lazily extracts the 
principal.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to implement and ship: CSP exemptions for content injected by privileged callers

[Kris Maglione]

On Sun, Oct 01, 2017 at 12:54:26PM -0700, Luke Crouch wrote:

On Friday, September 29, 2017 at 2:32:57 PM UTC-5, Kris Maglione wrote:

Security & privacy concerns:

This change will allow extensions to inject content into sites which can
(and probably will) cause security and privacy issues. However, it's
already quite easy for malicious or badly-implemented extensions to
create similar issues, and I don't think this change significantly
increases the risk. It may even mitigate it in some cases, since the
alternative of loading or evaling third-party scripts into the content
script sandbox would give them direct access to elevated privileges.

Per the CSP spec, those injections are assumed to be at the user's
behest, and should therefore take priority over the page author's
preferences.


+1 on this part.

As an add-on author, when I need to inject something the page CSP doesn't 
allow, I can already over-write the page CSP to allow it. But that feels more 
dangerous!


Yes, I'll admit that's one of my motivations. Whenever we try to prevent 
extensions from doing things for security or performance reasons, extensions 
authors tend to just find another way to do it with worse security and 
performance characteristics...


I filed bug 1273281 about preventing extensions from changing security headers 
without a special permission, but for now, many extensions still do such 
things.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to implement and ship: CSP exemptions for content injected by privileged callers

[Kris Maglione]

On Mon, Oct 02, 2017 at 11:13:20AM -0400, Boris Zbarsky wrote:
Passing along a JSContext would work.  We could have something like 
"null means no scripted caller, otherwise caller's compartment is the 
part that matters".  This relies on no one on the setattr path messing 
with the compartment, but that shouldn't be too hard to ensure, 
especially since we only have a few attributes on a few elements for 
which this is relevant...


I'd love it if we could pass along something that couldn't be 
abused/misused like a JSContext.  We could make up a wrapper class, 
but no matter what we do we'd have the fundamental tradeoff that 
either we grab the principal eagerly, and pay the cost for all the 
cases where it doesn't matter, or we grab it lazily and run the risk 
of thing changing under us.  We should probably measure how expensive 
setAttribute is and how expensive grabbing the principal from a 
JSContext (e.g. by marking the method as [NeedsCallerPrincipal]) is...


OK, I'll try a talos run with [NeedsCallerPrincipal] added to 
setAttribute and see where that comes out. If it looks good, 
I'll investigate that route some more. Otherwise, I'll probably 
go with a JSContext wrapper and retrieve the principal on 
demand.


The question of how to handler parser-generated nodes is tougher. 
Just using SubjectPrincipal() is one obvious approach, but the 
security characteristics of that worry me (what if the parser gets 
invoked by system code while some JS caller is blocked?).


Yes, that is the fundamental problem with SubjectPrincipal().

But even worse, depending on what you're trying to solve, using 
SubjectPrincipal() from SetAttr() doesn't even work right.  Consider 
this case:


 document.write("");

The SetAttr call for the  happens async, after the 

Re: Intent to implement and ship: CSP exemptions for content injected by privileged callers

[Kris Maglione]

On Mon, Oct 02, 2017 at 07:50:41AM -0700, Daniel Veditz wrote:

On Fri, Sep 29, 2017 at 8:33 PM, Boris Zbarsky  wrote:


On 9/29/17 3:32 PM, Kris Maglione wrote:


For instance, the following should all capture the caller principal for
the `src` URL at call time:

document.write(`http://example.com/favicon.ico;>`);
div.innerHTML = `http://example.com/favicon.ico;>`;
img.setAttribute("src", "http://example.com/favicon.ico;);
img.src = "http://example.com/favicon.ico;;


Do you _need_ to make all those ways work? I'm especially worried about
the parser ones. As long as direct DOM manipulation works, and is easier
than overwriting (or removing) the page's CSP, can't we just encourage
people to use that mechanism?


We do if we want Chrome parity, yes.

But I also have another motivation for wanting to make this as comprehensive 
as possible. I'd like to make web-accessible extension URLs loadable only by 
extension callers, to reduce the fingerprinting risk. Web content would still 
be able to fingerprint based on content injected into pages, but they wouldn't 
be able to poll for extension IDs.


There's already some risk of breaking extensions that inject scripts into the 
page context if we do that. The risk goes up a lot more if we also don't 
support things like innerHTML and insertAdjacentHTML.


document.write, though... To be honest, I'd be perfectly happy to forbid 
content scripts from using document.write altogether, if I could get away with 
it.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: test-verify now running as tier 2

[Chris Peterson]

This is very cool, Geoff! People have been talking about this idea for a 
long, so it is great to see it actually running. I'm glad to see chaos 
mode being tested, too.



On 2017-10-02 10:11 AM, Geoffrey Brown wrote:

Today the test-verify test task will start running as a tier 2 job.
Look for the "TV" symbol on treeherder, on linux-64 test platforms.

TV is intended as an "early warning system" for identifying the
introduction of intermittent test failures. When a mochitest, reftest,
or xpcshell test file is modified on a push, TV runs that particular
test over and over until it fails (orange job, standard failure
messages), or until max iterations are achieved (green job, all's
well), or until TV runs out of time (green job, maybe all's well?). As
a consequence, when a new test is added or a test is modified and an
intermittent failure is introduced, TV will usually be the first job
to fail, and it will fail on the push that modified the test, making
it (usually) simple to identify where the intermittent was introduced.

In future I hope to run TV on more platforms, apply it to more test
suites, and refine the --verify implementation to find intermittent
failures more efficiently. As a tier 2 task, TV failures will be
starred but will not cause backouts. I hope to move to tier 1 once TV
is proven to be effective.

More info at [1]. Bug and enhancement requests welcomed: please file
bugs blocking bug 1357513.

[1] https://developer.mozilla.org/en-US/docs/Test_Verification



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

test-verify now running as tier 2

[Geoffrey Brown]

Today the test-verify test task will start running as a tier 2 job.
Look for the "TV" symbol on treeherder, on linux-64 test platforms.

TV is intended as an "early warning system" for identifying the
introduction of intermittent test failures. When a mochitest, reftest,
or xpcshell test file is modified on a push, TV runs that particular
test over and over until it fails (orange job, standard failure
messages), or until max iterations are achieved (green job, all's
well), or until TV runs out of time (green job, maybe all's well?). As
a consequence, when a new test is added or a test is modified and an
intermittent failure is introduced, TV will usually be the first job
to fail, and it will fail on the push that modified the test, making
it (usually) simple to identify where the intermittent was introduced.

In future I hope to run TV on more platforms, apply it to more test
suites, and refine the --verify implementation to find intermittent
failures more efficiently. As a tier 2 task, TV failures will be
starred but will not cause backouts. I hope to move to tier 1 once TV
is proven to be effective.

More info at [1]. Bug and enhancement requests welcomed: please file
bugs blocking bug 1357513.

[1] https://developer.mozilla.org/en-US/docs/Test_Verification
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

--verify option added to mochitest, reftest, xpcshell test harnesses

[Geoffrey Brown]

The mochitest, reftest, and xpcshell test harnesses now support a
--verify option. For example:

  mach mochitest
docshell/test/test_anchor_scroll_after_document_open.html --verify

In verify mode, the requested test is run multiple times, in various
"modes", in hopes of quickly finding any intermittent failures. Once
tests are complete, a summary of results is printed:

:::
::: Test verification summary for:
:::
::: docshell/test/test_anchor_scroll_after_document_open.html
:::
::: 1. Run each test 10 times in one browser. : Pass
::: 2. Run each test 5 times in a new browser each time. : Pass
::: 3. Run each test 10 times in one browser, in chaos mode. : Pass
::: 4. Run each test 5 times in a new browser each time, in chaos mode. : Pass
:::
::: Test verification PASSED
:::

There's no flexibility in the number of times the test is run in each
mode and that's by design: I wanted a simple, standard way of checking
"Is this test likely to produce a frequent intermittent failure"?

Verify mode was developed for the test-verify task (announcement
coming soon!) but it may also be a convenient addition to your local
testing.

More info at [1]. Bug and enhancement requests welcomed: please file
bugs blocking bug 1357513.

[1] https://developer.mozilla.org/en-US/docs/Test_Verification
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Intent to ship: (hyperlink auditing)

[Anne van Kesteren]

Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=951104

Rationale: There's already a myriad of ways to obtain this data
through script. We might as well ship the protocol that both Chrome
and Safari ship in the hope that along with sendBeacon() it decreases
the usage of the slower alternatives; ultimately giving users a better
experience.

Previously: This was already discussed in
https://groups.google.com/d/msg/mozilla.dev.platform/DxvZVnc8rfo/RxSnyIFqxoQJ
and I think concluded given Jonas Sicking's statement in the
aforementioned bug, but since it's been a few years without action we
thought it would be worth it to send another ping.


-- 
https://annevankesteren.nl/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to implement and ship: CSP exemptions for content injected by privileged callers

[Anne van Kesteren]

On Mon, Oct 2, 2017 at 6:09 PM, Boris Zbarsky  wrote:
> On 10/2/17 12:03 PM, Daniel Veditz wrote:
>> Fair enough. Could we propose improvements to the APIs that would make
>> them more usable? For example an object argument to createElement() that
>> contained attribute/value pairs?
>
> This has definitely been proposed before.  Worth checking with Anne to see
> what the status is.  Specifically, did it die, and if so why? Because I,
> too, think this would be an interesting avenue to explore...

See https://github.com/whatwg/dom/issues/150. There's not really any
dominant pattern that's succeeded here in libraries that we could
adopt. You typically end up looking at templating and that has its own
host of issues. The other thing that would solve some of this is
browser-backed sanitization, but that's also a hard problem to solve
nobody has been willing to tackle and get standardized.


-- 
https://annevankesteren.nl/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to implement and ship: CSP exemptions for content injected by privileged callers

[Boris Zbarsky]

On 10/2/17 12:03 PM, Daniel Veditz wrote:

​Fair enough. Could we propose improvements to the API​s that would make
them more usable? For example an object argument to createElement() that
contained attribute/value pairs?


This has definitely been proposed before.  Worth checking with Anne to 
see what the status is.  Specifically, did it die, and if so why? 
Because I, too, think this would be an interesting avenue to explore...


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to implement and ship: CSP exemptions for content injected by privileged callers

[Daniel Veditz]

On Mon, Oct 2, 2017 at 8:17 AM, Boris Zbarsky  wrote:

> The fact is, direct DOM manipulation with no parser involved is really
> annoying to use.
>

​Fair enough. Could we propose improvements to the API​s that would make
them more usable? For example an object argument to createElement() that
contained attribute/value pairs?

  var div = document.createElement("div", null, {"id":"foo",
"class":"bar"});
  parent.prepend(div);

(the null is for the existing custom elements options param)

Wordier than your parent.prepend("") example, but
safer from developers trying to get clever with it. Such a short example
isn't going to fall victim to CSP blockage. In a longer example with src
URLs the overhead might not be too bad.

-
​Dan Veditz​
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to implement and ship: CSP exemptions for content injected by privileged callers

[Boris Zbarsky]

On 10/2/17 10:50 AM, Daniel Veditz wrote:

As long as direct DOM manipulation works, and is easier
than overwriting (or removing) the page's CSP, can't we just encourage
people to use that mechanism?


The fact is, direct DOM manipulation with no parser involved is really 
annoying to use.  Compare these two snippets:


  var div = document.createElement("div");
  div.id = "foo";
  div.className = "bar";
  parent.prepend(div);

and:

  parent.prepend("");

That said, I am sympathetic to the concern about innerHTML in 
particular.  Specifically, if an extension does:


  parent.innerHTML += "";

instead of doing:

  parent.append("");

and we exempt the former from CSP, then the extension just introduced 
XSS into the page without even noticing...


So to be honest, my gut feeling is that we should not try to make 
innerHTML or document.write() work here, but it would be nice to make 
the ParentNode.append/prepend methods and maybe createContextualFragment 
work...


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to implement and ship: CSP exemptions for content injected by privileged callers

[Boris Zbarsky]

On 9/30/17 12:19 AM, Kris Maglione wrote:
I still haven't settled on the details, but I it will probably have to 
involve capturing the caller principal from SetAttr hooks. Which would 
involve either changing that machinery to pass along a JS context when 
invoked by a scripted caller, or using something like 
SubjectPrincipal(). I'd definitely like to hear opinions on the best 
approach here.


I'd really rather we didn't use SubjectPrincipal() here.  It's too hard 
to reason about, and we have better options.


Passing along a JSContext would work.  We could have something like 
"null means no scripted caller, otherwise caller's compartment is the 
part that matters".  This relies on no one on the setattr path messing 
with the compartment, but that shouldn't be too hard to ensure, 
especially since we only have a few attributes on a few elements for 
which this is relevant...


I'd love it if we could pass along something that couldn't be 
abused/misused like a JSContext.  We could make up a wrapper class, but 
no matter what we do we'd have the fundamental tradeoff that either we 
grab the principal eagerly, and pay the cost for all the cases where it 
doesn't matter, or we grab it lazily and run the risk of thing changing 
under us.  We should probably measure how expensive setAttribute is and 
how expensive grabbing the principal from a JSContext (e.g. by marking 
the method as [NeedsCallerPrincipal]) is...


The question of how to handler parser-generated nodes is tougher. Just 
using SubjectPrincipal() is one obvious approach, but the security 
characteristics of that worry me (what if the parser gets invoked by 
system code while some JS caller is blocked?).


Yes, that is the fundamental problem with SubjectPrincipal().

But even worse, depending on what you're trying to solve, using 
SubjectPrincipal() from SetAttr() doesn't even work right.  Consider 
this case:


  document.write("");

The SetAttr call for the  happens async, after the 

Re: Intent to implement and ship: CSP exemptions for content injected by privileged callers

[Daniel Veditz]

On Fri, Sep 29, 2017 at 8:33 PM, Boris Zbarsky  wrote:

> On 9/29/17 3:32 PM, Kris Maglione wrote:
>
>> For instance, the following should all capture the caller principal for
>> the `src` URL at call time:
>>
>> document.write(`http://example.com/favicon.ico;>`);
>> div.innerHTML = `http://example.com/favicon.ico;>`;
>> img.setAttribute("src", "http://example.com/favicon.ico;);
>> img.src = "http://example.com/favicon.ico;;
>>
>
> What is the plan to do this, concretely?  Changing img.src to thread
> through a principal is not too bad but doing it for setAttribute would be a
> bit of a performance annoyance, and threading them through the parser would
> be _quite_ annoying.
>

​Do you _need_ to make all those ways work? I'm especially worried about
the parser ones. As long as direct DOM manipulation works, and is easier
than overwriting (or removing) the page's CSP, can't we just encourage
people to use that mechanism?

innerHTML ("Satan's candy"), in particular, is often misused and leads to
XSS​. It's the very last thing I'd want to give a "get out of CSP free"
pass.

-
​Dan Veditz​
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

[Firefox Desktop] Issues found: September 25th to September 29th

[Cornel Ionce]

Hi everyone,

Here's the list of new issues found and filed by the Desktop Release QA 
Team last week, *September 25 - September 29* (week 39).


Additional details on the team's priorities last week, as well as the 
plans for the current week are available at:


   https://public.etherpad-mozilla.org/p/DesktopManualQAWeeklyStatus



*RELEASE CHANNEL*
none

*BETA CHANNEL
*
ID  Summary Product Component   Is a regression 
Assigned to
1403118 
The lowest pixel of certain characters is cut off on Outlook Tasks
Core
Layout: Text
TBD NOBODY
1403121 
	New top site input fields are displayed outside the parent window after 
resizing the Firefox window

Firefox
Activity Streams: Newtab
NO  Ed Lee 
1403115 
Stylo: Black dots displayed in video's seekbar at the right and left 
side
Core
CSS Parsing and Computation
	YES 
 
	Emilio Cobos Alvarez 

1403127 
Can't save an Mp4 video from context menu in full screen mode
CoreAudio/Video: Playback
	YES 
 
	Xidorn Quan 

1403883 
The Learn More corresponding link exceeds the button's boundaries
Firefox Developer Tools: Netmonitor
NO  NOBODY
1403898 
The Stack Trace sources are not opened in Debugger
Firefox Developer Tools: Console
NO  NOBODY
1404318 
	Colon sign is displayed in "about:preferences#privacy" page in Clear 
history sub-dialog

Firefox Preferences
NO  Ricky Chien 
1404374 
	Edit top sites "Done" button is displayed outside the parent window 
after resizing the Firefox window

Firefox Activity Streams: Newtab
NO  NOBODY


*NIGHTLY CHANNEL**
*none

*ESR CHANNEL*
none


Regards,
Cornel (:cornel_ionce)
Desktop Release QA
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform