Fix Zip path traversal vulnerability #2630
Conversation
|
@damagatchi retest this please |
4 similar comments
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
| @@ -285,6 +285,7 @@ mult.install.bad=The selected ZIP file is not valid. Please choose a valid zip f | |||
| mult.install.progress.baddest=Couldn't write multimedia to the local filesystem at: ${0} | |||
| mult.install.progress.badentry=There was a bad entry in the zip file: ${0} | |||
| mult.install.progress.errormoving=There was a problem copying the multimedia from the zip file, please try again. | |||
| mult.install.progress.invalid.entry=The path of the entry ${0} doesn't match the parent folder, review the content of the ZIP file and try again! | |||
There was a problem hiding this comment.
Probably just say Invalid CCZ selected. User would not necessary understand entry or zip here. So lets obscure the details because this issue would probably only surface when someone is deliberately trying to do something malicious.
There was a problem hiding this comment.
@avazirna this is still pending on this.
There was a problem hiding this comment.
I don't know what you mean @shubham1g5 😅. That's on me, I was waiting for the jobs to complete to push the new changes and then I forgot
| Logger.log(TAG, "Unzipped entry " + entry.getName()); | ||
|
|
||
| File entryOutput = new File(destination, entry.getName()); |
There was a problem hiding this comment.
nit: call it entryFile and path beloe entryPath ?
There was a problem hiding this comment.
@shubham1g5 At this point, it's unclear whether the zip entry is a directory or a file, so the idea was to use a generic term to apply the verification to both. How strongly do you feel about this? we can always put that logic in a helper method
There was a problem hiding this comment.
not strongly, ok to skip.
|
@damagatchi retest this please |
1 similar comment
|
@damagatchi retest this please |
|
@damagatchi retest this please |
| @@ -285,7 +285,7 @@ mult.install.bad=The selected ZIP file is not valid. Please choose a valid zip f | |||
| mult.install.progress.baddest=Couldn't write multimedia to the local filesystem at: ${0} | |||
| mult.install.progress.badentry=There was a bad entry in the zip file: ${0} | |||
| mult.install.progress.errormoving=There was a problem copying the multimedia from the zip file, please try again. | |||
| mult.install.progress.invalid.entry=The path of the entry ${0} doesn't match the parent folder, review the content of the ZIP file and try again! | |||
| mult.install.progress.invalid.ccz=The selected CCZ file is invalid, please review its content and try again! | |||
There was a problem hiding this comment.
nit: I like the extra messaging but we probably also don't want to hint that user is expected to be able to inspect content of the ccz - The selected CCZ file is invalid, please verify it and try again!
There was a problem hiding this comment.
True. I approached this with an attacker's persona in mind but let's keep things simple, updating it.
avazirna
force-pushed
the
zip-path-traversal-vulnerability-fix
branch
from
9f6fb4d
to
8dcda3b
Compare
|
@damagatchi retest this please |
|
@damagatchi retest this please |
Summary
This PR is to fix a vulnerability that can be exploited to gain unauthorized access to system or even private folders by using ZIP entries with path traversal characters, more info can be found here. The fix is basically to check whether each Zip entry path matches the destination folder, if it doesn't, then the decompression process is interrupted.
Safety Assurance