Skip to content

Fix directory traversal in bsdcpio #110

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

Merged
merged 1 commit into from
Mar 5, 2015
Merged

Fix directory traversal in bsdcpio #110

merged 1 commit into from
Mar 5, 2015

Conversation

ghedo
Copy link
Contributor

@ghedo ghedo commented Mar 1, 2015

This fixes a directory traversal issue in the cpio tool that was originally reported by Alexander Cherepanov.

The patch adds a new option ARCHIVE_EXTRACT_SECURE_NOABSOLUTEPATHS that disables extraction of absolute paths. It also provides a test case and updates the documentation.

Note that I only implemented this for posix, because I don't have a windows machine to test this on.

Also, a CVE id was requested for this issue but none has been assigned yet.

archive_entry_copy_pathname(ae, "/tmp/abs");
archive_entry_set_mode(ae, S_IFREG | 0777);
assert(0 == archive_write_header(a, ae));
assert(0 == archive_write_finish_entry(a));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should include a check here to verify that /tmp/abs was actually created in this case. I think the following is sufficient:

assert_file_exists("/tmp/abs")

It would be better, of course, to generate a filename that does not exist before this test, then check that it does not exist after this test.

@kientzle
Copy link
Contributor

kientzle commented Mar 4, 2015

This is a nice start. The tests need a few more checks to verify that the security check really does prevent or not prevent the file creation. (Right now, you're just verifying that the security check returns an error.)

@ghedo
Copy link
Contributor Author

ghedo commented Mar 4, 2015

Should be fixed now. I use assertFileExists("/tmp/abs"), then unlink("/tmp/abs") in the test without the flag, and assertFileNotExists("/tmp/abs") in the one with the flag.

Not sure if using unlink() is ok though.

@kientzle
Copy link
Contributor

kientzle commented Mar 4, 2015

If we're going to use a fixed name, we need to use one that is reasonably certain to not interfere with any other user of /tmp. How about
/tmp/libarchive_test-test_write_disk_secure-absolute_path.tmp

This fixes a directory traversal in the cpio tool.
@ghedo
Copy link
Contributor Author

ghedo commented Mar 4, 2015

Sounds good to me. I just updated the PR.

kientzle added a commit that referenced this pull request Mar 5, 2015
Fix directory traversal in bsdcpio
@kientzle kientzle merged commit 180a8cb into libarchive:master Mar 5, 2015
@kientzle
Copy link
Contributor

kientzle commented Mar 5, 2015

Thank you. I'll merge this now; we will of course need to update this logic soon to identify absolute paths on Windows as well.

@attritionorg
Copy link

CVE-2015-2304 was assigned for this at some point.OSVDB 117148 corresponds to this entry as well.

@melvyn-sopacua
Copy link

I'm wondering if this fix is correct for the cpio -dumpl <abspath> case, used in quite a few scripts (FreeBSD ports's macro ${COPY_TREE_SHARE} as a prime example) to hardlink something from place a to b, falling back top copy if no hardlink is possible.

The error is thrown on the path that is the combination of the argument and the relative path provided by stdin. So if input path is verified to be relative and abspath argument is identical to it's realpath, there is no issue and the error thrown is a false positive. Please correct me if I'm missing something here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants