-
Notifications
You must be signed in to change notification settings - Fork 762
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
Added error text to warning when untaring with bsdtar #1609
Added error text to warning when untaring with bsdtar #1609
Conversation
@mmatuska This MR seems suspicious, the error message that's printed is almost identical before and after, but calls to safe_fprintf were replaced with calls to the unsafe fprintf. The diff doesn't make this obvious due to the removal of a newline in a parameter list. Given the recent uncovering of @JiaT75's backdoor inserted into XZ, can you double check that switching out safe fprintf with unsafe fprintf isn't introducing a vulnerability here? It appears that the unsafe fprintf calls introduced by this MR are still in the source code, unchanged: https://github.com/libarchive/libarchive/blob/master/tar/read.c#L374-L375 |
@mortie Looks to me like |
They did also add more error printing using |
You're right, I noticed that too upon closer inspection of the diff. And yeah, after reading up on both what Sorry for the ping. |
(Hey Colin, LTNS! 👋 ) What about calls like this: libarchive/libarchive/archive_write_disk_posix.c Line 2256 in 2fb7b0c
or libarchive/libarchive/archive_write_disk_posix.c Line 1101 in 2fb7b0c
I guess the concern would be that they could use terminal escape sequences to obfuscate the contents of archives? |
FWIW, I noticed it's using |
@taviso Yeah, you're right. I forgot that libarchive put path names into there. Terminal escape sequences in pathnames could do all sorts of nasty things, including exploiting bugs in terminal emulators (I know there have been a few over the years). This should probably be switched back to safe_fprintf. And also use archive_errno as @obfusk notes. |
@cperciva would it be helpful to offer a PR? Or is one already in the works |
A PR would be appreciated. |
I'm not sure if there are potentially problematic |
Not sure how easy that would be to exploit, but I think this call could be a source of the error being printed:
|
That commit definitely looks fishy. |
Thanks to @emaste we have merged a fix to the issues observed above. |
I come here after knowing about this. I fear the fixes may not be enough. There are two issues with the current code:
Normally I wouldn't pay this much attention but given that this code was deliberately put by a hostile actor; I suggest the strerror be removed, or replaced by a thread-safe variant and check for nullptrs. |
I think this needs its own CVE. |
I don't see any evidence that either problem is relevant. bsdtar is not multithreaded. The errno values are from system calls, so if your libc doesn't do something sane in that case, it is already broken. |
If someone who has experience with CVE processes wants to drive this, please contact [email protected] first so we can ensure that we don't get a bunch of duplicate filings. |
@anarazel Sorry, that I am mentioning this here, but the clone of the compromised repository is set read-only. I wonder, if fionn/xz-backdoored@4430e07 also belongs to the exploit, because -O2 makes it much easier to inject code than -O3 |
The commit still uses |
ref: libarchive/libarchive#1609 (comment) (cherry picked from commit 71d684e)
ref: libarchive/libarchive#1609 (comment) (cherry picked from commit 71d684e)
ref: libarchive/libarchive#1609 (comment) (cherry picked from commit 71d684e)
ref: libarchive/libarchive#1609 (comment) (cherry picked from commit 71d684e)
[ commit 71d684e85c10ef13ef790a7195a1cf74722e9b52 ] ref: libarchive/libarchive#1609 (comment)
The issue is that the pathname can also end up in the error string.
Note though that there are other cases where |
Windows 11 bundles libarchive, but I don’t know if they ever call this part. |
This is part of tar's archive reader; it is likely that it is used in Windows' tar.exe. 🙂 EDIT: I misread which part of the code this was; corrected above. |
way too many arm chair "researchers" in this thread I have a bigger question, if the switch from safe fprintf is actually a concern, why did maintainers of this project approve the merge. Are commits actually being reviewed or just blindly approved cuz you recognize the PRs name attached to it? |
Patch the libarchive formula with a fix to make tar error reporting more robust and use correct errno. For more context, see libarchive/libarchive#1609 and libarchive/libarchive#2103. jh it seems prudent to (at least) review again commits by the same author.tkFollowing the xz backdoor (https://www.cve.org/CVERecord?id=CVE-2024-3094), the libarchive team reviewed commits by the same author. This
Patch the libarchive formula with a fix to make tar error reporting more robust and use correct errno. For more context, see libarchive/libarchive#1609 and libarchive/libarchive#2103.
I don't know much about C++ programming syntax, but shouldn't there be stronger code-formatting rules that pull requests must meet before they are reviewed let alone accepted? The intent of the changes may be obvious to those with C++ experience but looking at the diff it looks to me like there was something off about it. I think software should be designed and implemented in such a way that those with limited domain expertise but good programming experience can spot irregularities or stuff which looks "off". Hopefully this won't cause much of a digression from the main topic. |
CIA vibes |
Patch the libarchive formula with a fix to make tar error reporting more robust and use correct errno. For more context, see libarchive/libarchive#1609 and libarchive/libarchive#2103.
We should prefer the naming convention printf() and unsafe_printf(), no? |
@TheDirigible fprintf is from the C standard library, safe_fprintf is a function internal to libarchive which does extra filtering. There's nothing libarchive can reasonably do about fprintf. |
I suggest looking closely at those who approved said PRs, and other PRs they approved. |
A group of very experienced engineers has been working for a couple of days on this issue. We've audited all changes made by the one account that is known to have made malicious changes to We will continue to examine libarchive for any evidence of similar problems and will address any that we find. Thanks for your patience and assistance. If you know of any other problems with libarchive, please:
Most importantly, please keep this in mind: We're all victims here. |
Good idea! I'll do so. |
Added the error text when printing out warning and errors in bsdtar when untaring. Previously, there were cryptic error messages when, for example in issue #1561, the user tries to untar an archive in a location they do not have write access to.
closes #1561