onekey-sec / unblob

Extract files from any kind of container formats
https://unblob.org
Other
2.16k stars 80 forks source link

Sanitize symlink target #768

Open e3krisztian opened 7 months ago

e3krisztian commented 7 months ago

Split off of #763 . There are still problems to solve here, see https://github.com/onekey-sec/unblob/commit/954c1cd5a06bcdb52048fc23da44661c73c94f31#commitcomment-138623089 but tests should run with the exception of 2 failures.

https://github.com/onekey-sec/unblob/commit/954c1cd5a06bcdb52048fc23da44661c73c94f31 rewrites the logic to sanitize symlinks to be relative and kept within the extraction directory. This is done using the os module instead of Pathlib as Pathlib.resolve would fail if a symlink target was missing (which doesn't prevent us from safely converting it to a relative link). With this change I no longer see false positives around MaliciousSymlinks, instead symlinks are created safely within the extraction directory. If a relative symlink originally tried accessing a directory above its own root (i.e., ./bin/sh -> ../../../../../bin/bash), we update the link so it remains within the extraction directory.

AndrewFasano commented 7 months ago

I believe there is still a bug in b11fe469a8716e7c00e70f4daa99e291a0277422: when extracting in a (host) directory within /tmp/, a symlink of /var pointing to /tmp/var will be sanitize to point to var creating a symlink loop. This seems to be caused by the use of os.path.commonpath finding a similarity between the extraction directory and the absolute symlink target.

e3krisztian commented 7 months ago

@qkaiser I wanted to make this PR just to have a place to discuss. It was extracted from a larger PR, and wanted to see CI test results, which our code checks (ruff) prevented, to push through the commit I have made some hacky fixes to be thrown away in the final version.

This definitely needs a rewrite, so should have made it draft initially.

I do not plan working on this soon, especially as the tar fix covering these problems are merged.