Skip to content
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

Userland: Fix tar/unzip directory traversal vulnerability #5713

Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Prev
Userland: Fix tar/unzip directory traversal vulnerability
This change validates the filenames within a tar/zip archive during
extraction. If the filename within a the archive is outside of the
current working directory the file will be skipped and not extracted
onto the host system.

Closes #3991
Closes #3992
  • Loading branch information
Baron Lenardson committed Mar 18, 2021
commit 3844e8569689dd476064a0759d704bc64fb3ca2c
48 changes: 47 additions & 1 deletion Userland/Utilities/tar.cpp
Expand Up @@ -59,6 +59,18 @@ int main(int argc, char** argv)
args_parser.add_positional_argument(paths, "Paths", "PATHS", Core::ArgsParser::Required::No);
args_parser.parse(argc, argv);

// Currently tar doesn't support extracting to a specified location. If that is implemented the target path
// should be unveiled with rwc instead of the current directory.
StringBuilder target_path_raw;
target_path_raw.append(realpath(".", nullptr));
target_path_raw.append("/");
LexicalPath target_path(target_path_raw.to_string());
auto target_path_string = target_path.string();
if (unveil(target_path_string.characters(), "rwc") < 0) {
perror("unveil");
return 1;
}

if (create + extract + list != 1) {
warnln("exactly one of -c, -x, and -t can be used");
return 1;
Expand All @@ -68,6 +80,16 @@ int main(int argc, char** argv)
auto file = Core::File::standard_input();

if (archive_file) {
// make sure we can read the archive path
auto archive_file_path = realpath(archive_file, nullptr);
if (unveil(archive_file_path, "r") < 0) {
perror("unveil");
return 1;
}
if (unveil(nullptr, nullptr) < 0) {
perror("unveil");
return 1;
}
auto maybe_file = Core::File::open(archive_file, Core::IODevice::OpenMode::ReadOnly);
if (maybe_file.is_error()) {
warnln("Core::File::open: {}", maybe_file.error());
Expand All @@ -86,14 +108,38 @@ int main(int argc, char** argv)
warnln("the provided file is not a well-formatted ustar file");
return 1;
}

for (; !tar_stream.finished(); tar_stream.advance()) {
auto file_name = tar_stream.header().file_name();

if (list || verbose)
outln("{}", tar_stream.header().file_name());
outln("{}", file_name);

if (extract) {
Tar::TarFileStream file_stream = tar_stream.file_contents();

const Tar::Header& header = tar_stream.header();

// determine where the archive intends to write a file
StringBuilder destination_path_raw;
if (file_name[0] != '/') {
destination_path_raw.append(target_path_string);
destination_path_raw.append("/");
}
destination_path_raw.append(file_name);
LexicalPath destination_path(destination_path_raw.to_string());

if (!(destination_path.string().starts_with(target_path_string))) {
fprintf(stderr, "File %s path is outside of current working directory, skipping\n", file_name.to_string().characters());
return false;
}

// Ignore tars containing target path.
// This will prevent mkdir from failing later when it probably shouldn't
if (destination_path.string() == target_path_string) {
continue;
}

switch (header.type_flag()) {
case Tar::NormalFile:
case Tar::AlternateNormalFile: {
Expand Down
48 changes: 46 additions & 2 deletions Userland/Utilities/unzip.cpp
Expand Up @@ -24,12 +24,14 @@
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/

#include <AK/LexicalPath.h>
#include <AK/MappedFile.h>
#include <AK/NumberFormat.h>
#include <LibCore/ArgsParser.h>
#include <LibCore/File.h>
#include <string.h>
#include <sys/stat.h>
#include <unistd.h>

static const u8 central_directory_file_header_sig[] = "\x50\x4b\x01\x02";

Expand Down Expand Up @@ -62,7 +64,7 @@ static bool find_next_central_directory(off_t file_size, const MappedFile& file,
return false;
}

static bool unpack_file_for_central_directory_index(off_t central_directory_index, const MappedFile& file)
static bool unpack_file_for_central_directory_index(off_t central_directory_index, const MappedFile& file, const AK::String target_path)
{
enum CentralFileDirectoryHeaderOffsets {
CFDHCompressionMethodOffset = 10,
Expand Down Expand Up @@ -120,6 +122,25 @@ static bool unpack_file_for_central_directory_index(off_t central_directory_inde
return false;
file_name[file_name_length] = '\0';

// determine where the archive intends to write a file
StringBuilder destination_path_raw;
if (file_name[0] != '/') {
destination_path_raw.append(target_path);
destination_path_raw.append("/");
}
destination_path_raw.append(file_name);
LexicalPath destination_path(destination_path_raw.to_string());
if (!(destination_path.string().starts_with(target_path))) {
fprintf(stderr, "File %s path is outside of current working directory, skipping\n", file_name);
return false;
}

// Ignore zips containing target path.
// This will prevent mkdir from failing later when it probably shouldn't
if (destination_path.string() == target_path) {
return true;
}

if (file_name[file_name_length - 1] == '/') {
if (mkdir(file_name, 0755) < 0) {
perror("mkdir");
Expand Down Expand Up @@ -162,6 +183,29 @@ int main(int argc, char** argv)
args_parser.add_positional_argument(path, "File to unzip", "path", Core::ArgsParser::Required::Yes);
args_parser.parse(argc, argv);

StringBuilder target_path_raw;
// Currently unzip doesn't support extracting to a specified location. If that is implemented the target_path
// should be replaced with the target path from the command line arguements instead of "."
target_path_raw.append(realpath(".", nullptr));
target_path_raw.append("/");
LexicalPath target_path(target_path_raw.to_string());
auto target_path_string = target_path.string();
if (unveil(target_path_string.characters(), "rwc") < 0) {
perror("unveil");
return 1;
}

auto zip_path = realpath(path, nullptr);
if (unveil(zip_path, "r") < 0) {
perror("unveil");
return 1;
}

if (unveil(nullptr, nullptr) < 0) {
perror("unveil");
return 1;
}

bblenard marked this conversation as resolved.
Show resolved Hide resolved
String zip_file_path { path };

struct stat st;
Expand Down Expand Up @@ -192,7 +236,7 @@ int main(int argc, char** argv)

off_t index = 0;
while (find_next_central_directory(st.st_size, mapped_file, index, index)) {
bool success = unpack_file_for_central_directory_index(index, mapped_file);
bool success = unpack_file_for_central_directory_index(index, mapped_file, target_path_string);
if (!success) {
printf("Could not find local file header for a file.\n");
return 4;
Expand Down