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

sandboxing prevents access to /proc/ directory #169

Open
ghost opened this issue Feb 23, 2023 · 7 comments
Open

sandboxing prevents access to /proc/ directory #169

ghost opened this issue Feb 23, 2023 · 7 comments
Labels
area: nsjail Related to NsJail and its configuration type: feature New feature or request

Comments

@ghost
Copy link

ghost commented Feb 23, 2023

The directory /proc/[pid]/ isn't available, which I think is a bit too harsh of a sandboxing measure in this case. I don't know of any use case where accessing /proc/[pid]/* could be abused. I understand not letting it mess with other processes, but you can restrict it to only its own process using symlinks or something similar, so that shouldn't be much of an issue.

Let me know if that idea sounds good! If it does I'll probably submit a PR within the next week or so to implement this idea.

@MarkKoz MarkKoz added type: feature New feature or request area: nsjail Related to NsJail and its configuration labels Mar 22, 2023
@MarkKoz
Copy link
Member

MarkKoz commented Mar 22, 2023

Hi, thanks for the feature request, and the offer to implement it! I'm not familiar with what the security implications are for allowing access to its own process. I have several questions regarding this request.

First, what uses cases are we supporting by allowing access to this? If we're going to be making the default sandbox less strict, we need to have good reason to take on the potential risk. We're aiming to provide a sandbox with sane, secure defaults. Bear in mind that it is possible to use a custom config for one's own snekbox instance.

Second, how can we be reasonably confident that this change poses no or minimal security risk?

Finally, how can we be reasonably confident the implementation is correct?

@ghost
Copy link
Author

ghost commented Mar 24, 2023

First, what uses cases are we supporting by allowing access to this? If we're going to be making the default sandbox less strict, we need to have good reason to take on the potential risk. We're aiming to provide a sandbox with sane, secure defaults. Bear in mind that it is possible to use a custom config for one's own snekbox instance.

The main use case of this (and really all that it would allow it to do) would be to allow a process to dynamically access/analyze information about itself.

Second, how can we be reasonably confident that this change poses no or minimal security risk?

To my knowledge, there are no security risks involved with a process accessing its own process information. However, the access could be limited to only certain parts of the /proc/[pid]/ directory if we want to be absolutely sure of safety/security. (We might only allow access to /proc/[pid]/fd/* and /proc/[pid]/fdinfo/*, for example.)

Finally, how can we be reasonably confident the implementation is correct?

We'd check if the process can access its /proc/[pid]/ directory (and also /proc/self/). I can write some tests that check for that (among other things) to go along with the implementation.

Let me know how what you think! (I might not get back to you immediately as things are quite busy here, but I will get back to you within the next few days.) Thanks for taking the time to respond to my idea.

@MarkKoz
Copy link
Member

MarkKoz commented Apr 1, 2023

The main use case of this (and really all that it would allow it to do) would be to allow a process to dynamically access/analyze information about itself.

Do you have any specific use cases in mind? How is a process being able to access information about itself useful?

the access could be limited to only certain parts of the /proc/[pid]/ directory if we want to be absolutely sure of safety/security

I'm more comfortable starting with this, and leaving the door open to expanding it in the future. There are some things that seem pretty benign and straight forward (like cmdline) and others that naïvely sound more "scary" (e.g. mem). Which directories did you have in mind?

@ghost
Copy link
Author

ghost commented Apr 5, 2023

Do you have any specific use cases in mind? How is a process being able to access information about itself useful?

It's not the most useful thing, in that almost everything you could do using it you could do via similar already allowed means, but there are some things like /proc/[pid]/fd/ and /proc/[pid]/fdinfo/ which are not only benign but provide information which is hard to find elsewhere.

I'm more comfortable starting with this, and leaving the door open to expanding it in the future. There are some things that seem pretty benign and straight forward (like cmdline) and others that naïvely sound more "scary" (e.g. mem). Which directories did you have in mind?

The ones that I originally wanted were /proc/self/, /proc/[pid]/fd/ and /proc/[pid]/fdinfo/, but upon having thought about it more, /proc/[pid]/cmdline, /proc/[pid]/environ, /proc/[pid]/cwd/, and /proc/[pid]/status would all probably be fine.

While we're at it, /dev/fd/, /dev/stdout, /dev/stdin, /dev/stderr, /dev/null, /dev/full, and /dev/zero would be nice additions as well.

@MarkKoz
Copy link
Member

MarkKoz commented Apr 7, 2023

While we're at it, /dev/fd/, /dev/stdout, /dev/stdin, /dev/stderr, /dev/null, /dev/full, and /dev/zero would be nice additions as well.

These all sound fine. And /dev/fd/ is really just /proc/self/fd/, right?

/proc/[pid]/fd/ and /proc/[pid]/fdinfo/

On second thought, wouldn't this allow one eval to access another eval's file descriptors? We do create a PID namespace with nsjail. I wonder if when we mount /proc, we can do it in a way such that it only gives access to the PIDs in that namespace. If we could, then we could just give access to all of /proc without worry. Maybe it does that out of the box already. It sounds like it depends on when the mount is performed according to this.

@ghost
Copy link
Author

ghost commented Apr 10, 2023

These all sound fine. And /dev/fd/ is really just /proc/self/fd/, right?

That is correct. It's just a convenient shortcut that we might as well include if we're going to include /proc/self/fd/.

On second thought, wouldn't this allow one eval to access another eval's file descriptors?

As long as each eval is a separate process, (and as I understand it, that's true), that wouldn't be an issue. Each process would see /proc/ as the following:

proc/
    [pid]/
    self/
``` Each process can only access itself, and not other processes. It's possible that there are other things in `/proc/` which we might want to include in the future, but for now I think that those are all that we'd need.

I must confess that I don't know too much about PID namespaces, so I can't accurately speak on how that would interact with what I'm proposing here. However, over the next few days I'll try to do some research into that, and get back to you with more information on the subject.

@MarkKoz
Copy link
Member

MarkKoz commented Apr 10, 2023

Yeah, each eval invokes a separate nsjail process which IIRC uses execve to replace itself with the Python process after it has set up the namespacing and whatnot. With PID namespacing enabled in nsjail, I'm not sure if mounting /proc would just mount the host system's /proc or if it would somehow be restricted by the namespacing.

Each process would see /proc/ as the following

How would we accomplish that? Would we have to know the PID of the process ahead of time? Mount options have to be passed to nsjail before it creates the Python process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: nsjail Related to NsJail and its configuration type: feature New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant