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

Add some validation for remove_job_data in the executor server #468

Merged
merged 2 commits into from
Nov 1, 2022
Merged

Add some validation for remove_job_data in the executor server #468

merged 2 commits into from
Nov 1, 2022

Conversation

yahoNanJing
Copy link
Contributor

Which issue does this PR close?

Closes #467.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@yahoNanJing yahoNanJing requested a review from andygrove October 28, 2022 02:44
@yahoNanJing
Copy link
Contributor Author

yahoNanJing commented Oct 28, 2022 •
edited
Loading

Hi @andygrove @avantgardnerio, could you help review this PR? It's part of #466.

@yahoNanJing yahoNanJing mentioned this pull request Oct 28, 2022
Open
9 tasks
@mingmwang
Copy link
Contributor

Why we return errors in this RPC?

@yahoNanJing
Copy link
Contributor Author

Although currently we do nothing for the result, it's better to know the detailed error which may be helpful for the scheduler to take future actions.

avantgardnerio
avantgardnerio requested changes Oct 29, 2022
View reviewed changes
Copy link
Contributor

@avantgardnerio avantgardnerio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this has a critical vulnerability of allowing deletion of paths outside of the workdir.

ballista/executor/src/executor_server.rs Outdated
info!("Remove data for job {:?}", job_id);

// Verify whether it's a legal job id
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the redundant scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unnecessary. Just for making the verification steps more clearly

ballista/executor/src/executor_server.rs Outdated
if path.is_dir() {
info!("Remove data for job {:?}", job_id);
std::fs::remove_dir_all(&path)?;
path.push(job_id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the work_dir subdirectory check... that's the critical vulnerability here.

ballista/executor/src/executor_server.rs Outdated
"Job id should not be empty!!!".to_string(),
));
}
if job_id.contains('.') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to prevent files with extensions? Or parent directories?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for prevent deleting parent directory. Generally, a directory should not contain '.'

ballista/executor/src/executor_server.rs
}

std::fs::remove_dir_all(&path)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the docs we have a vulnerability here:

if self has a verbatim prefix (e.g. \\?\C:\windows) and path is not empty, the new path is normalized: all references to . and .. are removed.

I can bypass the . string check on Windows by doing a \\?. It is impossible to validate directories at a string level. The missing check is to fully normalize this directory, then check if the normalized directory is a subdirectory of the work_dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. I'll refine it to use a more standard and robust way for the subdirectory check.

@avantgardnerio
Copy link
Contributor

avantgardnerio commented Oct 29, 2022 •
edited
Loading

This (downvoted) solution is correct:

jdahlstrom
Dec '21

Please note that starts_with is purely lexical and does not necessarily reflect whether the paths are actually 
nested in the filesystem because symlinks are a thing. 
You might want to call [canonicalize 3](https://doc.rust-lang.org/std/path/struct.Path.html#method.canonicalize) 
first on both paths (although that does require both paths to actually exist).

@yahoNanJing
Copy link
Contributor Author

Hi @avantgardnerio, could you help review again for the refined commit?

@avantgardnerio
Copy link
Contributor

help review again

Looks great, thank you @yahoNanJing !

@yahoNanJing
Copy link
Contributor Author

Thanks @avantgardnerio. Then I'll merge it.

@yahoNanJing yahoNanJing merged commit 68d513d into apache:master Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avantgardnerio avantgardnerio avantgardnerio requested changes

@andygrove andygrove Awaiting requested review from andygrove

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add some validation for the remove_job_data grpc service
4 participants
@yahoNanJing @mingmwang @avantgardnerio @kyotoYaho