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

I just had to remove all my fine grained permissions because of --allow-run changes in deno 2. #26839

Closed
justinmchase opened this issue Nov 12, 2024 · 12 comments

Comments

@justinmchase
Copy link
Contributor

justinmchase commented Nov 12, 2024

deno --version
deno 2.0.5 (stable, release, x86_64-apple-darwin)
v8 12.9.202.13-rusty
typescript 5.6.2

Using git blame I am able to see this PR is the cause of this issue: #25370 @dsherret

I had an app that I upgraded from deno 1.x to 2.x in just the last couple of days and I was very dismayed to find that I had to replace all of my fine grained permissions with --allow-all, which I really hate.

Here was my entrypoint:

ENTRYPOINT ["deno", "run", \
  "--config=deno.jsonc", \
  "--lock=deno.lock", \
  "--cached-only", \
  "--no-prompt", \
  "--allow-env=APP_CHECK,LOG_LEVEL,DATA_DIR,WEBHOOK_PATH,WEBHOOK_SECRET,WEBSITES_PORT,GITHUB_APP_ID,GITHUB_PRIVATE_KEY,GITHUB_APP_SENDER_ID,GITHUB_DEBUG_TOKEN,LD_LIBRARY_PATH", \
  "--allow-read=.", \
  "--allow-write=.data", \
  "--allow-net=0.0.0.0,api.github.com,github.com", \
  "--allow-run=semver,git,gh,mkdir", \
  "main.ts" \
]

And you can see it has a few --allow-run permissions which then causes this runtime error:

NotCapable: Requires --allow-all permissions to spawn subprocess with LD_LIBRARY_PATH environment variable.

So because I am calling a few subprocesses I now need to remove all of that for --allow-run, which will allow anything I import to call out to any domain, read and write any file on the whole operating system and invoke any child process... This is insane.

I'm sorry but I get that allowing a child process to run means that you can't guarantee that that child process will be restricted to the same permissions of scripts running inside of denos process but I am explicitly stating that I am willing to take on that risk by whitelisting certain child processes. I really don't think this is remotely comparable to allowing all permissions in process.

I really don't understand how this made it through code review and strongly request that you revert this change and allow people utilize allow-run in conjunction with the other permissions. By all mean document the ramifications of allow-run and let them know that these deno permissions really only pertain to scripts running within deno itself but requiring us to remove all in process fine-grained permissions just because I'm allowing certain subprocesses to run, makes no sense at all.


If you really wanted to lock down --allow-run further you could maybe add the ability to specify which module is allowed to run these subprocesses so that its not available for imported scripts to run these subprocesses without being explicitly allowed to.

For example:

allowing a specific module in local code

deno run --allow-run=./src/example.ts=git,gh,mkdir main.ts

allowing 3rd party module

deno run --allow-run=jsr:@example/foo=kubectl main.ts

Please, let me choose to take on that risk without having to completely forego all in-process permission restrictions.

@justinmchase justinmchase changed the title I just had to remove all my fine grained permissions because of allow-run changes in deno 2. I just had to remove all my fine grained permissions because of --allow-run changes in deno 2. Nov 12, 2024
@justinmchase
Copy link
Contributor Author

Relevant docs:
https://deno.com/blog/v2.0-release-candidate#--allow-run-flag-changes

Then, anytime a subprocess is spawned with LD_* or DYLD_* environmental variables, a full --allow-run permission will be required. Note that these environmental variables should rarely be relied on. If you really need to use them, you should consider further sandboxing Deno by running additional layers of protection (e.g. a Docker container).

The documentation above says "a full --allow-run permission will be required" but in the actual error it says "Requires --allow-all permissions" (all vs run) so maybe that error is just misleading and I could have done --allow-run instead of --allow-all?

@justinmchase
Copy link
Contributor Author

Ok yes confirmed, it does actually appear to be a bug in the error message here:
https://github.com/denoland/deno/pull/25370/files#diff-a3b49a848fa532dde7996aea509fc1fc88c9c9bb3936210e8574cf1257f296b1R605

Instead of removing all of my permisisons and replacing them with --allow-all as the error seems to indicate, if I instead replace just --allow-run=semver,git,gh,mkdir with --allow-run then it works.

So it seems like the error message is just wrong, which would explain the disconnect here.

Also, I still don't understand why I can't lock it down to certain subprocess names but this is far less bad than I was thinking at least.

@dsherret
Copy link
Member

dsherret commented Nov 12, 2024

So because I am calling a few subprocesses I now need to remove all of that for --allow-run, which will allow anything I import to call out to any domain, read and write any file on the whole operating system and invoke any child process... This is insane.

This is to fix a security issue. A malicious script could write to .data then load any executable code it wants in that directory via the LD_ env vars.

Probably your environment already had the LD_*/DYLD_* variables set and you can clear them before launching the sub process to fix the issue. For example:

new Deno.Command(..., {
 env: {
   "LD_LIBRARY_PATH": "",
   "LD_PRELOAD": "",
   "DYLD_FALLBACK_LIBRARY_PATH": ""
 }
}

@justinmchase
Copy link
Contributor Author

Ok but now I have just replaced it with --allow-run so now it can just run whatever it wants from wherever it wants, without even needing to try anything tricky so how is it better?

Also, they can't set any of the LD env vars and the .data dir is not in any of the paths in any LD env vars so how can it load any executable code it wants?

Maybe those vars should just always be unset in Deno.Command and you could make the caller explicitly set them if desired?

Anyway, I guess I'm unblocked but I would say the issue where the error message says --allow-all vs --allow-run is the main source of the confusion.

@justinmchase
Copy link
Contributor Author

So my env vars by default are:

LD_LIBRARY_PATH: "/usr/local/lib",
DENO_DIR: "/deno-dir/",
PATH: "/app/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
DENO_INSTALL_ROOT: "/usr/local",
DYLD_FALLBACK_LIBRARY_PATH: "",
LD_PRELOAD: "",

So given that deno has --allow-write=.data only how can executing a subprocess in this case load a binary a malicious script is writing?

Could it essentially detect if any path is writable and then produce this error?

What if a directory under PATH itself is writable, is it not the same problem?

@dsherret
Copy link
Member

Here's the relevant issue: #11964

Also, they can't set any of the LD env vars and the .data dir is not in any of the paths in any LD env vars so how can it load any executable code it wants?

When a sub process is launched it can set any environment variable for that sub process, so as long as it can point an LD env variable at some place writable then I believe it could launch whatever code it wants.

Maybe those vars should just always be unset in Deno.Command and you could make the caller explicitly set them if desired?

I think it's too late to make that change, but it might have been nice (though maybe a bit confusing in some cases).

Anyway, I guess I'm unblocked but I would say the issue where the error message says --allow-all vs --allow-run is the main source of the confusion.

Yeah it should. I'm going to open a PR to update the message and suggest unsetting the environment variables too.

@justinmchase
Copy link
Contributor Author

I have a binary thats created via deno compile... and its one of the subprocesses I'm trying to run and it has this error when I try to run with LD_LIBRARY_PATH=""

/app/bin/semver: error while loading shared libraries: libdl.so.2:

So deno compile appears to create binaries dependent on libdl.so.2 by default, this is in alpine linux.

@justinmchase
Copy link
Contributor Author

justinmchase commented Nov 12, 2024

And so you're saying that malicious script could create a file at .data/libdl.so.2 and then run...

Deno.Command('semver', { env: { LD_LIBRARY_PATH: "./data" } })

...and it would end up trying to load their binary and arbitrary code could be run. 🤔

So I can change my flag from --allow-run=semver to --allow-run and it will no longer print the error but it will still be vulnerable to the same potential problem, correct?

It appears that I can't run the tool without the path being set and I can't stop malicious code from setting it 🤔


I think the notion that allowing calls to Deno.Command could only come from certain modules rather than the entire process being allowed would solve it.

Otherwise another possible solution might be to add another allow such as:

--allow-ld=/usr/local/lib

And then for all the (DY)?LD_.* env vars you could set them to "" by default for child processes and if someone tries to set it explicitly then you would throw a permission error for any path not in the allow-ld list.

You could also check if there is any overlap between the allow-write locations and the allow-ld locations and that could be forbidden also.

@dsherret
Copy link
Member

So I can change my flag from --allow-run=semver to --allow-run and it will no longer print the error but it will still be vulnerable to the same potential problem, correct?

Yeah and in this case, someone could just write an executable to the .data folder and run that.

I think the notion that allowing calls to Deno.Command could only come from certain modules rather than the entire process being allowed would solve it.

It's very hard to solve this properly.

You could also check if there is any overlap between the allow-write locations and the allow-ld locations and that could be forbidden also.

I'm thinking maybe it should check theses values on startup and only disallow launching if the values aren't the same as at startup.

@justinmchase
Copy link
Contributor Author

You're saying if the child process has a different ld variable than the parent process then it would get blocked?

What if I actually need the subprocess to load libraries from a specific directory thats not in the parent processes LD_LIBRARY_PATH?

Then I would just need to make sure that the path is included before invoking the parent and I could lock the folder location down with file system permissions to ensure its readable but not writable... 🤔

The only problem would be if the parent and the child needed to load different versions of the same library and therefore needed different paths. This seems pretty contrived though.

It certainly seems like a simple solution and a welcome improvement. It would work for me and would be safe, probably most others as well. Perhaps if people encounter the more contrived scenario you have --allow-ld as a backup plan.

@dsherret
Copy link
Member

You're saying if the child process has a different ld variable than the parent process then it would get blocked?

Yeah, it would relax the restriction only for that case. I'd like to get more feedback before doing it though (just in case it would cause more security issues).

It certainly seems like a simple solution and a welcome improvement. It would work for me and would be safe, probably most others as well. Perhaps if people encounter the more contrived scenario you have --allow-ld as a backup plan.

Yeah, I think we probably need an --allow-ld flag, but I'm not sure what the demand for it is.

dsherret added a commit that referenced this issue Nov 12, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
For #26839
@justinmchase
Copy link
Contributor Author

The error messages were fixed and that was probably enough to close this issue if you want.

The workaround is to just use --allow-run with no restrictions, I think the --allow-ld would actually give you the ultimate precision on allowing precise run permissions without risking the ld issue.

Perhaps its not worth fixing now vs later when you get more people caring but at least you have a plan of attack.

Please feel free to close this if you want and thank you for your attention!

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

No branches or pull requests

3 participants