Skip to content

Commit

Permalink
Merge pull request #1609 from JiaT75/added_error_message_to_warning_b…
Browse files Browse the repository at this point in the history 
…sdtar_1561

Added error text to warning when untaring with bsdtar
  • Loading branch information
@mmatuska
mmatuska committed Nov 15, 2021
2 parents 86c9361 + f27c173 commit e37efc1
Showing 1 changed file with 3 additions and 4 deletions.
7 changes: 3 additions & 4 deletions tar/read.c
View file
Expand Up @@ -371,10 +371,9 @@ read_archive(struct bsdtar *bsdtar, char mode, struct archive *writer)
r = archive_read_extract2(a, entry, writer);
if (r != ARCHIVE_OK) {
if (!bsdtar->verbose)
safe_fprintf(stderr, "%s",
archive_entry_pathname(entry));
safe_fprintf(stderr, ": %s",
archive_error_string(a));
safe_fprintf(stderr, "%s", archive_entry_pathname(entry));
fprintf(stderr, ": %s: ", archive_error_string(a));
fprintf(stderr, "%s", strerror(errno));
if (!bsdtar->verbose)
fprintf(stderr, "\n");
bsdtar->return_value = 1;
Expand Down

4 comments on commit e37efc1

@thygrrr
Copy link

@thygrrr thygrrr commented on e37efc1 Mar 29, 2024 •
edited

Choose a reason for hiding this comment

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

@mmatuska Note that the author/github account of this PR is implicated in a security incident regarding xz. Seeing how one of the lines was possibly downgraded from safe_fprintf to just fprintf, there might be some exploit angles/scenarios at play here as well. It looks a bit weird at first glance.

@ihexon
Copy link

@ihexon ihexon commented on e37efc1 Mar 29, 2024

Choose a reason for hiding this comment

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

https://owasp.org/www-community/attacks/Format_string_attack
https://cs155.stanford.edu/papers/formatstring-1.2.pdf

@kivikakk
Copy link

Choose a reason for hiding this comment

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

@ihexon This is not a format string attack, since the format strings themselves are still plain/valid (": %s: " and "%s"). The difference between safe_fprintf and fprintf is how they handle non-printable characters in their output, and not in their handling of the format strings themselves (fmt goes straight into the va_args calls without modification).

Note that safe_fprintf is still used with archive_entry_pathname(entry); the concern would be if something could be done with non-printable characters from archive_error_string(a) or strerror(errno) hitting stderr (which is newly added).

@kientzle
Copy link
Contributor

Choose a reason for hiding this comment

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

This has been discussed over in #1609 and fixed in #2101.

Please sign in to comment.