TEZ-4699:Add Validation/Canonical checks to avoid path exploitation in CSVResult.java#471
Conversation
|
thanks @ramitg254 for this patch, can you please elaborate on how the patch solved this problem? before/after behaviour would be nice to see |
|
💔 -1 overall
This message was automatically generated. |
|
sure @abstractdog before: after: similarly the other preventive check ensures the output directory should exist: before: after: in the similar fashion other preventive checks works before opening stream for file writes as well |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| // check if the parent directory is writable | ||
| if (!parent.canWrite()) { | ||
| throw new IOException("No write permission on directory: " + parent); | ||
| } | ||
| return target; |
There was a problem hiding this comment.
Should we check this separately? If the directory isn’t writable by the user, the write will fail anyway, right?
There was a problem hiding this comment.
this was added for more descriptive exception, but it can be dropped as AccessDeniedException will take care of it. done.
| if (record.length != headers.length) { | ||
| continue; //LOG error msg? | ||
| } | ||
| private static File validateOutputFile(String fileName) throws IOException { |
There was a problem hiding this comment.
nit class formatting: private method ideally goes below the public one
| continue; //LOG error msg? | ||
| } | ||
| private static File validateOutputFile(String fileName) throws IOException { | ||
| //null or empty string check |
There was a problem hiding this comment.
the corresponding code is too obvious, we don't need this comment
| File target = new File(fileName).getCanonicalFile(); | ||
| File parent = target.getParentFile(); | ||
|
|
||
| // check if the parent directory exists |
There was a problem hiding this comment.
the corresponding code is too obvious, we don't need this comment
| throw new IllegalArgumentException("fileName must not be null or empty"); | ||
| } | ||
|
|
||
| // resolve relative path |
There was a problem hiding this comment.
the corresponding code is too obvious, we don't need this comment
| Path targetPath = target.toPath(); | ||
|
|
||
| try (BufferedWriter bw = new BufferedWriter( | ||
| // atomically create new file if it doesn't exist, and if exists, throw an exception |
There was a problem hiding this comment.
this comment doesn't add more value then looking at the javadoc of Files.newOutputStream, we can remove it I think
There was a problem hiding this comment.
thanks @ramitg254, left minor comments
also, can you please add simple unit test cases for what you implemented in validateOutputFile?
1 more thing to check: I think a path like |
done added |
This comment was marked as outdated.
This comment was marked as outdated.
I think it will be fine as it will be resolved to valid path by getCanonicalFile() before checks are run on it |
| result.dumpToFile(out.toString()); | ||
| Assert.fail("Expected FileAlreadyExistsException when output file already exists"); | ||
| } catch (FileAlreadyExistsException e) { | ||
| // CREATE_NEW must fail if path already exists (atomic exclusive create) |
There was a problem hiding this comment.
add an extra assertion here for clarity on exception message
yeah, it might be the case, but it should be validated with a unit test case also |
done added |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
🎊 +1 overall
This message was automatically generated. |
| } | ||
|
|
||
| @Test | ||
| public void testDumpToFileNoWritePermissionRelativePath() throws Exception { |
There was a problem hiding this comment.
I think this unit test mixes 2 different scenarios, which should be thought of separately:
- whether the user is able to define relative paths with
.. - whether the user can write to a folder without permissions
2) is clear, and I'm not even insisting on having a test case on that, because it's testing permission handling instead of any of the actual logic you just implemented, right? no matter how you implement validateOutputFile, this is going to pass always, so it doesn't add much value
regarding 1), it's more of a decision whether we want the user to be able to move upwards in the directory structure from the current working dir: I guess it's fine to completely disable that, like we did in TEZ-4659
There was a problem hiding this comment.
done added like if with use paths like ../ moves upward from the pwd from which process was launched it should throw IO exception
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
3 participants
validated using following command on cluster: