Added error text to warning when untaring with bsdtar

[JiaT75]

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

[mortie]

@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

[cperciva]

@mortie Looks to me like fprintf(stderr, "%s", strerror(errno)); is new? I'm not sure if the safe_fprintf matters since the strings here (error strings from libarchive and libc) should be 7-bit-safe, but I wouldn't mind switching that back.

[wgaylord]

@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

They did also add more error printing using fprintf(stderr, "%s", strerror(errno)); Which was not in the original. I do agree, strange to change from safe fprint to normal fprint tho.

[mortie]

You're right, I noticed that too upon closer inspection of the diff. And yeah, after reading up on both what archive_error_string returns and what's "safe" about safe_fprintf, replacing safe_fprintf with fprintf in this circumstance shouldn't change anything... it's just weird.

Sorry for the ping.

[taviso]

(Hey Colin, LTNS! 👋 )

What about calls like this:

libarchive/libarchive/archive_write_disk_posix.c

Line 2256 in 2fb7b0c

archive_set_error(&a->archive, en, "Can't create '%s'",

or

libarchive/libarchive/archive_write_disk_posix.c

Line 1101 in 2fb7b0c

archive_set_error(&a->archive, errno,

I guess the concern would be that they could use terminal escape sequences to obfuscate the contents of archives?

[obfusk]

FWIW, I noticed it's using errno directly instead of archive_errno(a), which might give the wrong error.

[cperciva]

@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.

[sgammon]

@cperciva would it be helpful to offer a PR? Or is one already in the works

[kientzle]

A PR would be appreciated.

[emaste]

What about calls like this:

libarchive/libarchive/archive_write_disk_posix.c

Line 2256 in 2fb7b0c

archive_set_error(&a->archive, en, "Can't create '%s'",

I'm not sure if there are potentially problematic archive_set_error calls in the read path, but no matter - there's no reason to avoid using safe_fprintf. I have a change in my local tree that I'll push to a PR.

[obfusk]

I'm not sure if there are potentially problematic archive_set_error calls in the read path

Not sure how easy that would be to exploit, but I think this call could be a source of the error being printed:

libarchive/libarchive/archive_read_disk_posix.c

Line 747 in 2fb7b0c

"Couldn't open %s", tree_current_path(t));

[DanielRuf]

That commit definitely looks fishy.

[kientzle]

Thanks to @emaste we have merged a fix to the issues observed above.

[darksylinc]

I come here after knowing about this.

I fear the fixes may not be enough.

There are two issues with the current code:

1. strerror is not thread safe which means it can be potentially exploited by having another thread manipulate strerror's internal buffer so that the null terminator is ignored when it's evaluated by printf/safe_printf (due to race conditions).
   • I speculate this could be used to leak secrets. Since printf would continue printing beyond the original null terminator, until it encounters a 0x00 in the stream.
2. On certain systems it is known for strerror to return nullptr on certain conditions; which may cause a crash.

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.

[davispw]

I think this needs its own CVE.

[jsonn]

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.

[kientzle]

I think this needs its own CVE.

If someone who has experience with CVE processes wants to drive this, please contact security@libarchive.de first so we can ensure that we don't get a bunch of duplicate filings.

[spixi]

@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

[rpigott]

I have some examples of exploits against busybox and OpenBSD tar in https://dgl.cx/2023/09/ansi-terminal-security#vulnerabilities-using-known-replies I suspect those could apply nearly unmodified here.

The commit still uses safe_printf for the pathnames, though. I don't think those are applicable here at all.

[emaste]

The commit still uses safe_printf for the pathnames, though. I don't think those are applicable here at all.

The issue is that the pathname can also end up in the error string.

$ touch $(printf '\033[31mred\033[0m')
$ tar -c -f escape.tar *red*
$ (cd /var/empty && tar -xf ~/escape.tar)
\033[31mred\033[0m: Can't create 'red': Operation not permitted
tar: Error exit delayed from previous errors.

Note though that there are other cases where archive_error_string() might be printed unescaped (e.g., when passed to lafe_warnc() or lafe_errc()), so this should be addressed in a more comprehensive manner.

[GossiTheDog]

Windows 11 bundles libarchive, but I don’t know if they ever call this part.

[DHowett]

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.

[juraisa]

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?

[vonHabsi]

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.

[Speculate7348]

CIA vibes

[TheDirigible]

We should prefer the naming convention printf() and unsafe_printf(), no?

[mortie]

@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.

[gerrywastaken]

I suggest looking closely at those who approved said PRs, and other PRs they approved.
The review process is the only protection we have and if it is compromised that is a bigger issue.

[kientzle]

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 xz. This change to error printing is the only one that has any concerns. It has been fixed and we'll be coordinating announcement and updates to libarchive through the appropriate channels.

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:

• If it's a security-critical issue, please email security@libarchive.de
• If it's a non-security-critical bug, please file an Issue in this github repository.

Most importantly, please keep this in mind: We're all victims here.

[emaste]

@kientzle perhaps we should close this to new comments? The specific case here has been addressed via #2101, I'm opening an issue for @jsonn's suggestion above of escaping pathnames in error strings, and anything else should be addressed via one of the two paths in your preceding comment.

[kientzle]

Good idea! I'll do so.