SafeTarfile: Update relative symlinks instead of dropping#770
SafeTarfile: Update relative symlinks instead of dropping#770AndrewFasano wants to merge 2 commits intoonekey-sec:mainfrom
Conversation
AndrewFasano
force-pushed
the
safe_tar_symlinks
branch
from
1847a35 to
20f156f
Compare
AndrewFasano
force-pushed
the
safe_tar_symlinks
branch
from
20f156f to
ddaa826
Compare
|
Updated to hopefully fix the pyright check. I can't for the life of me figure out how to get pytest to work locally though. |
e3krisztian
left a comment
e3krisztian
left a comment
There was a problem hiding this comment.
Have looked at and tried to understand what is going on here (as this is quite a big change) - I have made some comments.
However in the end I have ended up with an alternative PR #775 based on your work here (added you as co-author to the fix-commits if you do not mind), but with smaller changes.
@AndrewFasano may I ask you to look at #775 and validate the code/run some tests with fw-s to check if that indeed fixes the symlink extraction problems?
| rel_target = (extract_root / rel_target.relative_to("/")).relative_to( | ||
| extract_root | ||
| ) |
There was a problem hiding this comment.
| rel_target = (extract_root / rel_target.relative_to("/")).relative_to( | |
| extract_root | |
| ) | |
| rel_target = rel_target.relative_to("/")) |
| symlink_depth = len(tarinfo.name.split("/")) | ||
| target_depth = len(rel_target.parts) |
There was a problem hiding this comment.
A stored absolute path can contain relative parts, like /bin/../etc/./shadow, these depth calculations would not work with a path like this, and could lead to the link pointing somewhere else.
| resolved_path = (extract_root / tarinfo.name).parent / rel_target | ||
|
|
||
| # If the resolved path points outside of extract_root, we need to fix it! | ||
| if not is_safe_path(extract_root, resolved_path): |
There was a problem hiding this comment.
This is the real fix, that saves the busybox symlinks.
I am not sure the fix attempt of violating symlinks is needed below.
Looking at the file $ tar tvf DCS-6517B1_FW_v2.00.03.zip_extract/DCS-6517B1_FW_v2.00.03.pkg_extract/44-17853067.gzip_extract/gzip.uncompressed | rg ubifs_app
lrwxrwxrwx richard/richard 0 2017-05-24 10:52 mnt/ubifs_app -> mtd
lrwxrwxrwx richard/richard 0 2017-05-24 10:52 mnt/mtd/ubifs_app -> ubifs_app
lrwxrwxrwx richard/richard 0 2017-07-11 14:17 www/classes/panoviewer_lib.exe -> /mnt/ubifs_app/panoviewer_lib.exe
lrwxrwxrwx richard/richard 0 2017-07-11 14:17 www/classes/panoviewer.dmg -> /mnt/ubifs_app/panoviewer.dmgIt looks bad at the archive level. |
|
#775 got merged, which was inspired by and has the same goal as this PR. @AndrewFasano thank you! |
2 participants
This is a partial fix for #769. With this change, 172 of the 173 missing symlinks are correctly extracted
If I rebase this on top of #768 then the final symlink is extracted correctly (
/mnt/mtd/ubifs_app -> ubifs_app).While this fixes the bug, I'm not positive my path handling is correct/secure for all possible symlinks.