Skip to content
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

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

Closed
felipecrs opened this issue May 5, 2024 · 8 comments · Fixed by #23743
Closed

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

felipecrs opened this issue May 5, 2024 · 8 comments · Fixed by #23743

Comments

@felipecrs
Copy link

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 [email protected] run --allow-read main.ts <(echo felipe)
felipe

$ pkgx [email protected] run --allow-read main.ts <(echo felipe)
felipe

$ pkgx [email protected] 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
Copy link
Member

mmastrac commented May 5, 2024

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
Copy link

NeKzor commented May 6, 2024

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/[email protected]/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/[email protected]/examples/cat.ts:10:27

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

@felipecrs
Copy link
Author

felipecrs commented May 6, 2024

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
Copy link
Member

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/[email protected]/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/[email protected]/examples/cat.ts:10:27

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

This has been relaxed by #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 GHSA-23rx-c3g5-hv9w.

@felipecrs
Copy link
Author

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

@lucacasonato
Copy link
Member

lucacasonato commented May 8, 2024

@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: GHSA-23rx-c3g5-hv9w

@felipecrs
Copy link
Author

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

@mmastrac
Copy link
Member

mmastrac commented May 8, 2024

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.

mmastrac added a commit that referenced this issue May 10, 2024
`deno run script.ts <(some command)` is a valid use case -- let's allow
this to work without `--allow-all`.

Fixes #23703
dsherret pushed a commit that referenced this issue May 10, 2024
`deno run script.ts <(some command)` is a valid use case -- let's allow
this to work without `--allow-all`.

Fixes #23703
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants