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

Do not upload files outside task sandbox #1777

Merged
merged 1 commit into from Apr 12, 2018

Conversation

Projects
None yet
3 participants
@pschoenfelder
Contributor

pschoenfelder commented Apr 4, 2018

No description provided.

@pschoenfelder pschoenfelder requested a review from ssalinas Apr 4, 2018

@@ -69,6 +69,12 @@ private boolean writeS3MetadataFileForRotatedFiles(boolean finished) {
for (SingularityS3UploaderFile additionalFile : taskDefinition.getExecutorData().getS3UploaderAdditionalFiles()) {
Path directory = additionalFile.getDirectory().isPresent() ? taskDefinition.getTaskDirectoryPath().resolve(additionalFile.getDirectory().get()) : taskDefinition.getTaskDirectoryPath();
if (!directory.toAbsolutePath().startsWith(taskDefinition.getTaskDirectoryPath())) {

This comment has been minimized.

@ssalinas

ssalinas Apr 5, 2018

Member

Do we want to && this with isCheckSubdirectories(). The main thing we're trying to protect against is the uploader going crazy checking all subdirectories of / or something like that. Debating if we should allow a way for folks to use this for things outside of the sandbox, within reason. @baconmania thoughts?

@ssalinas

ssalinas Apr 5, 2018

Member

Do we want to && this with isCheckSubdirectories(). The main thing we're trying to protect against is the uploader going crazy checking all subdirectories of / or something like that. Debating if we should allow a way for folks to use this for things outside of the sandbox, within reason. @baconmania thoughts?

This comment has been minimized.

@pschoenfelder

pschoenfelder Apr 5, 2018

Contributor

As far as I can tell the startsWith method of checking this should not start recursively checking directories so I don't think we have to worry about going crazy checking all subdirs of /.

@pschoenfelder

pschoenfelder Apr 5, 2018

Contributor

As far as I can tell the startsWith method of checking this should not start recursively checking directories so I don't think we have to worry about going crazy checking all subdirs of /.

This comment has been minimized.

@ssalinas

ssalinas Apr 5, 2018

Member

It's not the startsWith, that's just a string check, I'm worried about. It's the isCheckSubdirectories() value of the additionalFile. If it isn't checking subdirectories/not recursive, the most files the uploader will eventually scan is whatever is in the specified folder. If it is recursive, that's the problem we are trying to prevent. (e.g. isCheckSubdirectories() -> true with a folder of / essentially causes it to check every folder on the host

That said I could go either way on limiting this. It's basically a question of whether or not we expect a task to write to and want to upload something outside it's sandbox, and if it does we certainly need to impose a limit that the folder isn't checked recursively

@ssalinas

ssalinas Apr 5, 2018

Member

It's not the startsWith, that's just a string check, I'm worried about. It's the isCheckSubdirectories() value of the additionalFile. If it isn't checking subdirectories/not recursive, the most files the uploader will eventually scan is whatever is in the specified folder. If it is recursive, that's the problem we are trying to prevent. (e.g. isCheckSubdirectories() -> true with a folder of / essentially causes it to check every folder on the host

That said I could go either way on limiting this. It's basically a question of whether or not we expect a task to write to and want to upload something outside it's sandbox, and if it does we certainly need to impose a limit that the folder isn't checked recursively

This comment has been minimized.

@baconmania

baconmania Apr 6, 2018

Contributor

I could see potentially only doing this limitation based on isCheckSubdirectories() == true, but my vote here right now would be to default to limiting everything to the sandbox unless we see an ask where people want to be able to upload things outside the sandbox.

@baconmania

baconmania Apr 6, 2018

Contributor

I could see potentially only doing this limitation based on isCheckSubdirectories() == true, but my vote here right now would be to default to limiting everything to the sandbox unless we see an ask where people want to be able to upload things outside the sandbox.

This comment has been minimized.

@ssalinas

ssalinas Apr 6, 2018

Member

👍 fair enough, let's leave the check as-is for now then

@ssalinas

ssalinas Apr 6, 2018

Member

👍 fair enough, let's leave the check as-is for now then

@ssalinas ssalinas added this to the 0.20.0 milestone Apr 11, 2018

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Apr 11, 2018

Member

🚢

Member

ssalinas commented Apr 11, 2018

🚢

@ssalinas ssalinas merged commit 38548b8 into master Apr 12, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ssalinas ssalinas deleted the s3-guardrails branch Apr 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment