denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
93.06k stars 5.14k forks source link

Cannot read files from `/proc/self/fd` since Deno 1.43 #23703

Closed felipecrs closed 1 week ago

felipecrs commented 2 weeks ago

Version: Deno 1.43.1

https://github.com/felipecrs/deno-repro-fd-perm

$ git clone https://github.com/felipecrs/deno-repro-fd-perm

$ cd deno-repro-fd-perm

$ pkgx deno@1.41 run --allow-read main.ts <(echo felipe)
felipe

$ pkgx deno@1.42 run --allow-read main.ts <(echo felipe)
felipe

$ pkgx deno@1.43 run --allow-read main.ts <(echo felipe)
error: Uncaught (in promise) PermissionDenied: permission denied: readfile '/proc/self/fd/16'
const text = await Deno.readTextFile(file);
                        ^
    at Object.readTextFile (ext:deno_fs/30_fs.js:878:24)
    at file:///home/felipecrs/repos/deno-repro-fd-perm/main.ts:7:25
mmastrac commented 2 weeks ago

With some changes in 1.43 this unfortunately requires --allow-all now, as we don't currently have a way to discriminate between pipes and files in /dev/fd and /proc/self/fd. I think we may be able to improve this situation.

NeKzor commented 1 week ago

Looks like a lot more is now disallowed which is a big breaking change.

Here is an example from the docs:

$ deno run --allow-read=/etc https://deno.land/std@0.198.0/examples/cat.ts /etc/passwd
error: Uncaught (in promise) PermissionDenied: permission denied: open '/etc/passwd'
  const file = await Deno.open(filename);
                          ^
    at Object.open (ext:deno_fs/30_fs.js:633:21)
    at https://deno.land/std@0.198.0/examples/cat.ts:10:27

Moving from --allow-read to --allow-all is very questionable.

felipecrs commented 1 week ago

I personally don't mind adding more granularity to the --allow flags (supposing some new flag will come to allow these extra use cases).

I just don't think it's a good idea to do it in Deno 1.x.

bartlomieju commented 1 week ago

Looks like a lot more is now disallowed which is a big breaking change.

Here is an example from the docs:

$ deno run --allow-read=/etc https://deno.land/std@0.198.0/examples/cat.ts /etc/passwd
error: Uncaught (in promise) PermissionDenied: permission denied: open '/etc/passwd'
  const file = await Deno.open(filename);
                          ^
    at Object.open (ext:deno_fs/30_fs.js:633:21)
    at https://deno.land/std@0.198.0/examples/cat.ts:10:27

Moving from --allow-read to --allow-all is very questionable.

This has been relaxed by https://github.com/denoland/deno/pull/23718 and will work again in v1.43.2.

I personally don't mind adding more granularity to the --allow flags (supposing some new flag will come to allow these extra use cases).

I just don't think it's a good idea to do it in Deno 1.x.

That's true, but we had to do it because of the security vulnerability that you can see at https://github.com/denoland/deno/security/advisories/GHSA-23rx-c3g5-hv9w.

felipecrs commented 1 week ago

Maybe supporting --allow-read=/dev/fd or --allow-read=/proc/self/fd is enough? User would be being explicit about it.

lucacasonato commented 1 week ago

@felipecrs Read access to /dev/fd allows bypassing all kinds of --allow-* permissions - so we are unlikely to reconsider for /dev/fd or /dev/self/fd. For more info, see: https://github.com/denoland/deno/security/advisories/GHSA-23rx-c3g5-hv9w

felipecrs commented 1 week ago

I see. -A then. Feel free to close this issue.

mmastrac commented 1 week ago

I think we might be able to make this work without relaxing the security sandbox -- we'll allow opening FD magic links on unix systems, but only if they are not stdio, and are pipes.