-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lookup filename or url of function definition #1189
Comments
Anything we solve in JavaScript land would be insecure and likely horribly unperformant and hacky. I think we would have to find a different approach. When we get to the point of loading ESM modules directly with V8, in theory V8 might be then able to assist in tracking where something is coming from, as far as a loaded module. I don't see how that would assist with #1181 though. |
I think the problem is more broad: if you have scripts from multiple origins running in the same javascript context, then I'm not sure you can have something be properly secure in a way that prevents the different origins from impersonating each other (while still allowing people to write javascript/typescript in normal ways). If one just wants something that can show a useful message to a user containing the script's origin (error messages, debug logs, etc), this isn't a big deal, but if one is looking for a real enforced security boundary (which I think this issue is hoping for; like only scripts from a certain domain can access its localStorage, or deno saves permissions given to it based on origin of individual scripts) then I don't think this is practical. (I've put a few example problems and exploits and some more info at the bottom of this post.) Also, it's un-browserlike to attach permissions to the origin a script is retrieved from. (If a web page imports a script from another domain, and that script accesses localStorage, then it will get the localStorage of the web page's origin, not the script's origin. Browsers attach permissions to the domain of a page, and javascript contexts are never associated with multiple pages of different domains.) This behavior for the web storage API would be un-browserlike and not secure. It would make a lot more sense (and be more secure) for things like localStorage and permissions to be associated with the origin of the entry script (whatever url/file is passed to deno as an argument). Example exploits:If a domain hosts a wrapper function around something that checks the caller (fs.*, localStorage, etc), then any script that uses the wrapper will be considered to belong to the wrapper function's domain. This doesn't seem like an unlikely pattern given the popularity of node modules like fs-extra. This could be useful for an attacker, but it's also likely to be confusing to legitimate users who wonder why their function calls are being associated with the utility's domain instead of their own.
import fs from 'fs';
export default function enhancedReadFileSync(filename, options) {
const data = fs.readFileSync(filename);
if (options.foo) {
data = foo(data);
}
return data;
} Even if a site is careful not to host modules that directly export functions that are thin wrappers for privileged functions, it can still be vulnerable because because global variables can be monkey-patched.
import fs from 'fs';
export default function grobFiles() {
const listOfFilesToGrob = [];
// ...
const filesToGrobContents = listOfFilesToGrob.map(filename => fs.readFileSync(filename));
// ...
}
import grobFiles from 'https://trustworthy-deno-utilities.com/grobFiles.js';
let readFileSyncAsTrustworthyDomain = null;
const originalForEach = Array.prototype.forEach;
Array.prototype.forEach = function(fn) {
readFileSyncAsTrustworthyDomain = fn;
throw new Error();
};
try {
grobFiles();
} catch (e) {}
Array.prototype.forEach = originalForEach;
// User will see that https://trustworthy-deno-utilities.com wants to read a file. (If deno ever persists permissions based on origin, then instant victory if the user has given it permissions before.)
readFileSyncAsTrustworthyDomain('.ssh/id_rsa'); tl;dr: Modules running within the same Javascript context aren't isolated from each other, and should all be considered within the same security boundary. If you give permission to one module, then any module should be assumed to be able to steal that permission. The Secure EcmaScript project investigated ways to allow real security boundaries to exist within a single Javascript context, but it has a lot of ramifications, is un-browserlike, and it would be a major endeavor for Deno. |
@Macil while it isn't perfect, moving the compiler to a seperate isolate as suggested in #1190. While it wouldn't address any of the exploits that you are talking about specifically, all modules would be run through an "protected" compiler and not be able to pretend to be anything than what they are. It at least reduces the attack surface. It feels like we need to work on how to "trust" an external module, like running in a mode where it is a "whitelist" of known hosts which remote modules can be loaded from. That wouldn't be this issue though. |
I don't understand the need. @hayd can you explain why you might want this in more detail? Leaning towards closing this. |
@ry denoland/std#108 (comment) is a good example, then you could group tests by the file where they're defined (and run setUp+tearDown on a per-file basis). |
Is this possible with import.meta.url ? |
@hayd it is possible with |
I will close this because it's not really a feature we can add without some crazy hacks - and I don't want to do that right now. But no, |
It would be useful e.g. for #1181 and #1008, where you need to look up the url of the caller* to determine what they have access to.
Potentially this could be an internal only for these usecases (later perhaps exposing a function that can do the file lookup) rather than monkey patch function.prototype.
* I was imagining functions would be looked up by .caller BUT it seems like that method is forbidden in strict mode. Is caller-lookup infeasible cheaply/without a hack?
cc @kitsonk
The text was updated successfully, but these errors were encountered: