denoland / deno

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

Permissions checks should use canonical paths #9607

Open caspervonb opened 3 years ago

caspervonb commented 3 years ago

Currently permission checks only look at the paths they were provided which may be a symbolic link. This allows you to escape the sandbox with symlinks which were just stabilized in https://github.com/denoland/deno/pull/9226

Most of the filesystem ops that deal with paths should always use the canonical path when checking permissions within the ops, the exception would be stat since there is an lstat and the link won't be followed later in the actual syscall.

There are other ways to we can go about it (capability based preopens with openat for example ala WASI), but not with our current permission model resolving to the true path is the way to go forward imho.

Related but without an actionable item https://github.com/denoland/deno/issues/2318.

Spoonbender commented 3 years ago

I agree with the general approach as I specified in the other issues.

I want to better understand what is being proposed and what implementation tradeoffs are we willing to make.

About the permissions list itself, do we:

  1. Resolve the symlink-based paths in the list upon startup? (but they might change in runtime)
  2. Resolve the symlink-based paths in the list whenever checking permissions? (performance penalty)
  3. Disallow symlink-based paths in the permissions list entirely? (but again, a non-symbolic path may change to symbolic path at runtime)

Similar issues for the input paths in filesystem ops:

  1. Do we resolve once and cache the result (which may change over time)?
  2. Resolve each time an op is initiated?
  3. Something in-between? Some scoped caching? e.g. force the user code to hold a "file" object and run all ops on it (so we won't have Deno.readTextFile() but instead let file = Deno.getFileHandle('/tmp/myfile'); let s = file.readText(); file.writeTest('something'); (this enables us to hold a file-descriptor inside the file object, so it doesn't change "beneath our feet")