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.

Sign up for GitHub

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

Jump to bottom

ProcessManager Auth missing #1180

Closed
dckc opened this issue Aug 30, 2016 · 5 comments · Fixed by #1279
Closed

ProcessManager Auth missing #1180

dckc opened this issue Aug 30, 2016 · 5 comments · Fixed by #1279
Assignees
@SeanTAllen
Labels
triggers release Major issue that when fixed, results in an "emergency" release

Comments

@dckc
Copy link
Contributor

dckc commented Aug 30, 2016

The Process package doesn't require anything beyond FilePath access to create a subprocess. Is that by design?

If so, please documented it in the files package: if you pass a FilePath("/abc") to a library, that library can create sub-processes to any path under "/abc". The FileExec bit isn't even checked.
@SeanTAllen
Copy link
Member

Would you be up for PR'ing some of that @dckc ?

@dckc
Copy link
Contributor Author

dckc commented Aug 30, 2016

I'm willing, but my availability is limited.

But first, I would need to know the answer: is it by design?

@jemc
Copy link
Member

jemc commented Aug 30, 2016

The FileExec bit isn't even checked.

This part at least definitely sounds like a bug to me.

@SeanTAllen SeanTAllen added difficulty: 1 - easy triggers release Major issue that when fixed, results in an "emergency" release and removed needs discussion labels Aug 31, 2016
@sylvanc
Copy link
Contributor

sylvanc commented Aug 31, 2016

Certainly not checking FileExec is a bug, no question!

But it is also a bug that ProcessMonitor doesn't yet require some sub-authority generated from AmbientAuth. Just because you have FileExec access to some part of the file system doesn't necessarily mean you should be allowed to start processes.

Thanks @dckc !

@SeanTAllen SeanTAllen changed the title Every FilePath includes ProcessMonitor authority? ProcessManager Auth missing Sep 28, 2016
@SeanTAllen SeanTAllen self-assigned this Sep 30, 2016
SeanTAllen added a commit that referenced this issue Sep 30, 2016
@SeanTAllen
Require authorization to start processes
9e3bb74 
Add an auth token for being able to start a process.
Additionally, it verifies that the file you are attempting
to start is in fact executable.

Closes #1180
@SeanTAllen SeanTAllen mentioned this issue Sep 30, 2016
Merged
SeanTAllen added a commit that referenced this issue Sep 30, 2016
@SeanTAllen
Require authorization to start processes
b53ce75 
Add an auth token for being able to start a process.
Additionally, it verifies that the file you are attempting
to start is in fact executable.

This is a breaking API change.

Closes #1180
@SeanTAllen
Copy link
Member

I'm working on this. As a difficulty easy, I left it for a bit hoping someone from the community would pick up, but its been a month since this was opened and it needs to get fixed.

SeanTAllen added a commit that referenced this issue Sep 30, 2016
@SeanTAllen
Require authorization to start processes
35f1fec 
Add an auth token for being able to start a process.
Additionally, it verifies that the file you are attempting
to start is in fact executable.

This is a breaking API change.

Closes #1180
This was referenced Oct 8, 2016
Closed
Closed
SeanTAllen added a commit that referenced this issue Oct 8, 2016
@SeanTAllen
Require authorization to start processes
31a424a 
Add an auth token for being able to start a process.
Additionally, it verifies that the file you are attempting
to start is in fact executable.

This is a breaking API change.

Closes #1180
SeanTAllen added a commit that referenced this issue Oct 8, 2016
@SeanTAllen
Require authorization to start processes
4e8cba9 
Add an auth token for being able to start a process.
Additionally, it verifies that the file you are attempting
to start is in fact executable.

This is a breaking API change.

Closes #1180
SeanTAllen added a commit that referenced this issue Oct 8, 2016
@SeanTAllen
Require authorization to start processes
8e391df 
Add an auth token for being able to start a process.
Additionally, it verifies that the file you are attempting
to start is in fact executable.

This is a breaking API change.

Closes #1180
kelcecil added a commit to kelcecil/ponyc that referenced this issue Oct 8, 2016
@kelcecil
Removed unneeded errors from ProcessError
98f3d9b 
There are several errors in ProcessError that are unused in the
ProcessMonitor. This commit removed unused errors with the
exception of ExecveError which is being used in ponylang#1180.
@kelcecil kelcecil mentioned this issue Oct 8, 2016
Merged
kelcecil added a commit to kelcecil/ponyc that referenced this issue Oct 8, 2016
@kelcecil
Removed unneeded errors from ProcessError
3812189 
There are several errors in ProcessError that are unused in the
ProcessMonitor. This commit removed unused errors with the
exception of ExecveError which is being used in ponylang#1180.
Theodus pushed a commit that referenced this issue Oct 9, 2016
@SeanTAllen @Theodus
Require authorization to start processes (#1279)
2d4c2a4 
Add an auth token for being able to start a process.
Additionally, it verifies that the file you are attempting
to start is in fact executable.

This is a breaking API change.

Closes #1180
@Theodus Theodus mentioned this issue Oct 9, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@SeanTAllen SeanTAllen

Labels
triggers release Major issue that when fixed, results in an "emergency" release
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Require authorization to start processes ponylang/ponyc
4 participants
@dckc @SeanTAllen @sylvanc @jemc