crash-reporter: Avoid writing through symlinks.
BUG=7987
TEST=bvts
Change-Id: I875adeb5073936e790beb93f6a223a1642131cbd
Review URL: http://codereview.chromium.org/4603001
diff --git a/crash_reporter/crash_collector.cc b/crash_reporter/crash_collector.cc
index 1e2cd1a..7abe2a5 100644
--- a/crash_reporter/crash_collector.cc
+++ b/crash_reporter/crash_collector.cc
@@ -5,11 +5,15 @@
#include "crash-reporter/crash_collector.h"
#include <dirent.h>
+#include <fcntl.h> // For file creation modes.
#include <pwd.h> // For struct passwd.
#include <sys/types.h> // for mode_t.
+#include <sys/wait.h> // For waitpid.
+#include <unistd.h> // For execv and fork.
#include <set>
+#include "base/eintr_wrapper.h"
#include "base/file_util.h"
#include "base/logging.h"
#include "base/string_util.h"
@@ -60,6 +64,76 @@
logger_ = logger;
}
+int CrashCollector::WriteNewFile(const FilePath &filename,
+ const char *data,
+ int size) {
+ int fd = HANDLE_EINTR(open(filename.value().c_str(),
+ O_CREAT | O_WRONLY | O_TRUNC | O_EXCL, 0666));
+ if (fd < 0) {
+ return -1;
+ }
+
+ int rv = file_util::WriteFileDescriptor(fd, data, size);
+ HANDLE_EINTR(close(fd));
+ return rv;
+}
+
+int CrashCollector::ForkExecAndPipe(std::vector<const char *> &arguments,
+ const char *output_file) {
+ // Copy off a writeable version of arguments.
+ scoped_array<char*> argv(new char *[arguments.size() + 1]);
+ int total_args_size = 0;
+ for (size_t i = 0; i < arguments.size(); ++i) {
+ if (arguments[i] == NULL) {
+ logger_->LogError("Bad parameter");
+ return -1;
+ }
+ total_args_size += strlen(arguments[i]) + 1;
+ }
+ scoped_array<char> buffer(new char[total_args_size]);
+ char *buffer_pointer = &buffer[0];
+
+ for (size_t i = 0; i < arguments.size(); ++i) {
+ argv[i] = buffer_pointer;
+ strcpy(buffer_pointer, arguments[i]);
+ buffer_pointer += strlen(arguments[i]);
+ *buffer_pointer = '\0';
+ ++buffer_pointer;
+ }
+ argv[arguments.size()] = NULL;
+
+ int pid = fork();
+ if (pid < 0) {
+ logger_->LogError("Fork failed: %d", errno);
+ return -1;
+ }
+
+ if (pid == 0) {
+ int output_handle = HANDLE_EINTR(creat(output_file, 0700));
+ if (output_handle < 0) {
+ logger_->LogError("Could not create %s: %d", output_file, errno);
+ // Avoid exit() to avoid atexit handlers from parent.
+ _exit(127);
+ }
+ dup2(output_handle, 1);
+ dup2(output_handle, 2);
+ execv(argv[0], &argv[0]);
+ logger_->LogError("Exec failed: %d", errno);
+ _exit(127);
+ }
+
+ int status = 0;
+ if (HANDLE_EINTR(waitpid(pid, &status, 0)) < 0) {
+ logger_->LogError("Problem waiting for pid: %d", errno);
+ return -1;
+ }
+ if (!WIFEXITED(status)) {
+ logger_->LogError("Process did not exit normally: %d", status);
+ return -1;
+ }
+ return WEXITSTATUS(status);
+}
+
std::string CrashCollector::Sanitize(const std::string &name) {
std::string result = name;
for (size_t i = 0; i < name.size(); ++i) {
@@ -291,7 +365,10 @@
version.c_str(),
payload_path.c_str(),
payload_size);
- if (!file_util::WriteFile(meta_path, meta_data.c_str(), meta_data.size())) {
+ // We must use WriteNewFile instead of file_util::WriteFile as we
+ // do not want to write with root access to a symlink that an attacker
+ // might have created.
+ if (WriteNewFile(meta_path, meta_data.c_str(), meta_data.size()) < 0) {
logger_->LogError("Unable to write %s", meta_path.value().c_str());
}
}
diff --git a/crash_reporter/crash_collector.h b/crash_reporter/crash_collector.h
index e5eddcd..219c8af 100644
--- a/crash_reporter/crash_collector.h
+++ b/crash_reporter/crash_collector.h
@@ -39,15 +39,25 @@
FRIEND_TEST(CrashCollectorTest, CheckHasCapacityUsual);
FRIEND_TEST(CrashCollectorTest, GetCrashDirectoryInfo);
FRIEND_TEST(CrashCollectorTest, GetCrashPath);
+ FRIEND_TEST(CrashCollectorTest, ForkExecAndPipe);
FRIEND_TEST(CrashCollectorTest, FormatDumpBasename);
FRIEND_TEST(CrashCollectorTest, Initialize);
FRIEND_TEST(CrashCollectorTest, MetaData);
FRIEND_TEST(CrashCollectorTest, ReadKeyValueFile);
FRIEND_TEST(CrashCollectorTest, Sanitize);
+ FRIEND_TEST(CrashCollectorTest, WriteNewFile);
// Set maximum enqueued crashes in a crash directory.
static const int kMaxCrashDirectorySize;
+ // Writes |data| of |size| to |filename|, which must be a new file.
+ // If the file already exists or writing fails, return a negative value.
+ // Otherwise returns the number of bytes written.
+ int WriteNewFile(const FilePath &filename, const char *data, int size);
+
+ int ForkExecAndPipe(std::vector<const char *> &arguments,
+ const char *output_file);
+
// Return a filename that has only [a-z0-1_] characters by mapping
// all others into '_'.
std::string Sanitize(const std::string &name);
diff --git a/crash_reporter/crash_collector_test.cc b/crash_reporter/crash_collector_test.cc
index 859653b..bf3a9dd 100644
--- a/crash_reporter/crash_collector_test.cc
+++ b/crash_reporter/crash_collector_test.cc
@@ -8,9 +8,16 @@
#include "base/string_util.h"
#include "crash-reporter/crash_collector.h"
#include "crash-reporter/system_logging_mock.h"
+#include "crash-reporter/test_helpers.h"
#include "gflags/gflags.h"
#include "gtest/gtest.h"
+// This test assumes the following standard binaries are installed.
+static const char kBinBash[] = "/bin/bash";
+static const char kBinCp[] = "/bin/cp";
+static const char kBinEcho[] = "/bin/echo";
+static const char kBinFalse[] = "/bin/false";
+
void CountCrash() {
ADD_FAILURE();
}
@@ -48,6 +55,81 @@
ASSERT_TRUE(&logging_ == collector_.logger_);
}
+TEST_F(CrashCollectorTest, WriteNewFile) {
+ FilePath test_file = test_dir_.Append("test_new");
+ const char kBuffer[] = "buffer";
+ EXPECT_EQ(strlen(kBuffer),
+ collector_.WriteNewFile(test_file,
+ kBuffer,
+ strlen(kBuffer)));
+ EXPECT_LT(collector_.WriteNewFile(test_file,
+ kBuffer,
+ strlen(kBuffer)), 0);
+}
+
+TEST_F(CrashCollectorTest, ForkExecAndPipe) {
+ std::vector<const char *> args;
+ char output_file[] = "test/fork_out";
+
+ // Test basic call with stdout.
+ args.clear();
+ args.push_back(kBinEcho);
+ args.push_back("hello world");
+ EXPECT_EQ(0, collector_.ForkExecAndPipe(args, output_file));
+ ExpectFileEquals("hello world\n", output_file);
+ EXPECT_EQ("", logging_.log());
+
+ // Test non-zero return value
+ logging_.clear();
+ args.clear();
+ args.push_back(kBinFalse);
+ EXPECT_EQ(1, collector_.ForkExecAndPipe(args, output_file));
+ ExpectFileEquals("", output_file);
+ EXPECT_EQ("", logging_.log());
+
+ // Test bad output_file.
+ EXPECT_EQ(127, collector_.ForkExecAndPipe(args, "/bad/path"));
+
+ // Test bad executable.
+ logging_.clear();
+ args.clear();
+ args.push_back("false");
+ EXPECT_EQ(127, collector_.ForkExecAndPipe(args, output_file));
+
+ // Test stderr captured.
+ std::string contents;
+ logging_.clear();
+ args.clear();
+ args.push_back(kBinCp);
+ EXPECT_EQ(1, collector_.ForkExecAndPipe(args, output_file));
+ EXPECT_TRUE(file_util::ReadFileToString(FilePath(output_file),
+ &contents));
+ EXPECT_NE(std::string::npos, contents.find("missing file operand"));
+ EXPECT_EQ("", logging_.log());
+
+ // NULL parameter.
+ logging_.clear();
+ args.clear();
+ args.push_back(NULL);
+ EXPECT_EQ(-1, collector_.ForkExecAndPipe(args, output_file));
+ EXPECT_NE(std::string::npos,
+ logging_.log().find("Bad parameter"));
+
+ // No parameters.
+ args.clear();
+ EXPECT_EQ(127, collector_.ForkExecAndPipe(args, output_file));
+
+ // Segmentation faulting process.
+ logging_.clear();
+ args.clear();
+ args.push_back(kBinBash);
+ args.push_back("-c");
+ args.push_back("kill -SEGV $$");
+ EXPECT_EQ(-1, collector_.ForkExecAndPipe(args, output_file));
+ EXPECT_NE(std::string::npos,
+ logging_.log().find("Process did not exit normally"));
+}
+
TEST_F(CrashCollectorTest, Sanitize) {
EXPECT_EQ("chrome", collector_.Sanitize("chrome"));
EXPECT_EQ("CHROME", collector_.Sanitize("CHROME"));
@@ -231,7 +313,8 @@
}
TEST_F(CrashCollectorTest, MetaData) {
- FilePath meta_file = test_dir_.Append("generated.meta");
+ const char kMetaFileBasename[] = "generated.meta";
+ FilePath meta_file = test_dir_.Append(kMetaFileBasename);
FilePath lsb_release = test_dir_.Append("lsb-release");
FilePath payload_file = test_dir_.Append("payload-file");
std::string contents;
@@ -247,12 +330,43 @@
collector_.AddCrashMetaData("foo", "bar");
collector_.WriteCrashMetaData(meta_file, "kernel", payload_file.value());
EXPECT_TRUE(file_util::ReadFileToString(meta_file, &contents));
- EXPECT_EQ("foo=bar\n"
- "exec_name=kernel\n"
- "ver=version\n"
- "payload=test/payload-file\n"
- "payload_size=3\n"
- "done=1\n", contents);
+ const char kExpectedMeta[] =
+ "foo=bar\n"
+ "exec_name=kernel\n"
+ "ver=version\n"
+ "payload=test/payload-file\n"
+ "payload_size=3\n"
+ "done=1\n";
+ EXPECT_EQ(kExpectedMeta, contents);
+
+ // Test target of symlink is not overwritten.
+ payload_file = test_dir_.Append("payload2-file");
+ ASSERT_TRUE(
+ file_util::WriteFile(payload_file,
+ kPayload, strlen(kPayload)));
+ FilePath meta_symlink_path = test_dir_.Append("symlink.meta");
+ ASSERT_EQ(0,
+ symlink(kMetaFileBasename,
+ meta_symlink_path.value().c_str()));
+ ASSERT_TRUE(file_util::PathExists(meta_symlink_path));
+ logging_.clear();
+ collector_.WriteCrashMetaData(meta_symlink_path,
+ "kernel",
+ payload_file.value());
+ // Target metadata contents sould have stayed the same.
+ contents.clear();
+ EXPECT_TRUE(file_util::ReadFileToString(meta_file, &contents));
+ EXPECT_EQ(kExpectedMeta, contents);
+ EXPECT_NE(std::string::npos, logging_.log().find("Unable to write"));
+
+ // Test target of dangling symlink is not created.
+ file_util::Delete(meta_file, false);
+ ASSERT_FALSE(file_util::PathExists(meta_file));
+ logging_.clear();
+ collector_.WriteCrashMetaData(meta_symlink_path, "kernel",
+ payload_file.value());
+ EXPECT_FALSE(file_util::PathExists(meta_file));
+ EXPECT_NE(std::string::npos, logging_.log().find("Unable to write"));
}
int main(int argc, char **argv) {
diff --git a/crash_reporter/kernel_collector.cc b/crash_reporter/kernel_collector.cc
index c958733..4fa51e2 100644
--- a/crash_reporter/kernel_collector.cc
+++ b/crash_reporter/kernel_collector.cc
@@ -277,9 +277,12 @@
FilePath kernel_crash_path = root_crash_directory.Append(
StringPrintf("%s.kcrash", dump_basename.c_str()));
- if (file_util::WriteFile(kernel_crash_path,
- kernel_dump.data(),
- kernel_dump.length()) !=
+ // We must use WriteNewFile instead of file_util::WriteFile as we
+ // do not want to write with root access to a symlink that an attacker
+ // might have created.
+ if (WriteNewFile(kernel_crash_path,
+ kernel_dump.data(),
+ kernel_dump.length()) !=
static_cast<int>(kernel_dump.length())) {
logger_->LogInfo("Failed to write kernel dump to %s",
kernel_crash_path.value().c_str());
diff --git a/crash_reporter/user_collector.cc b/crash_reporter/user_collector.cc
index 02025c2..6fa368d 100644
--- a/crash_reporter/user_collector.cc
+++ b/crash_reporter/user_collector.cc
@@ -4,17 +4,13 @@
#include "crash-reporter/user_collector.h"
-#include <fcntl.h> // For creat.
#include <grp.h> // For struct group.
#include <pwd.h> // For struct passwd.
#include <sys/types.h> // For getpwuid_r, getgrnam_r, WEXITSTATUS.
-#include <sys/wait.h> // For waitpid.
-#include <unistd.h> // For execv and fork.
#include <string>
#include <vector>
-#include "base/eintr_wrapper.h"
#include "base/file_util.h"
#include "base/logging.h"
#include "base/string_util.h"
@@ -176,9 +172,10 @@
std::string dump_basename = FormatDumpBasename(exec, time(NULL), pid);
FilePath log_path = GetCrashPath(crash_path, dump_basename, "log");
FilePath meta_path = GetCrashPath(crash_path, dump_basename, "meta");
- file_util::WriteFile(log_path,
- error_log_.data(),
- error_log_.length());
+ // We must use WriteNewFile instead of file_util::WriteFile as we do
+ // not want to write with root access to a symlink that an attacker
+ // might have created.
+ WriteNewFile(log_path, error_log_.data(), error_log_.length());
AddCrashMetaData("sig", kCollectionErrorSignature);
WriteCrashMetaData(meta_path, exec, log_path.value());
}
@@ -255,62 +252,6 @@
return false;
}
-int UserCollector::ForkExecAndPipe(std::vector<const char *> &arguments,
- const char *output_file) {
- // Copy off a writeable version of arguments.
- scoped_array<char*> argv(new char *[arguments.size() + 1]);
- int total_args_size = 0;
- for (size_t i = 0; i < arguments.size(); ++i) {
- if (arguments[i] == NULL) {
- logger_->LogError("Bad parameter");
- return -1;
- }
- total_args_size += strlen(arguments[i]) + 1;
- }
- scoped_array<char> buffer(new char[total_args_size]);
- char *buffer_pointer = &buffer[0];
-
- for (size_t i = 0; i < arguments.size(); ++i) {
- argv[i] = buffer_pointer;
- strcpy(buffer_pointer, arguments[i]);
- buffer_pointer += strlen(arguments[i]);
- *buffer_pointer = '\0';
- ++buffer_pointer;
- }
- argv[arguments.size()] = NULL;
-
- int pid = fork();
- if (pid < 0) {
- logger_->LogError("Fork failed: %d", errno);
- return -1;
- }
-
- if (pid == 0) {
- int output_handle = creat(output_file, 0700);
- if (output_handle < 0) {
- logger_->LogError("Could not create %s: %d", output_file, errno);
- // Avoid exit() to avoid atexit handlers from parent.
- _exit(127);
- }
- dup2(output_handle, 1);
- dup2(output_handle, 2);
- execv(argv[0], &argv[0]);
- logger_->LogError("Exec failed: %d", errno);
- _exit(127);
- }
-
- int status = 0;
- if (HANDLE_EINTR(waitpid(pid, &status, 0)) < 0) {
- logger_->LogError("Problem waiting for pid: %d", errno);
- return -1;
- }
- if (!WIFEXITED(status)) {
- logger_->LogError("Process did not exit normally: %d", status);
- return -1;
- }
- return WEXITSTATUS(status);
-}
-
bool UserCollector::RunCoreToMinidump(const FilePath &core_path,
const FilePath &procfs_directory,
const FilePath &minidump_path,
diff --git a/crash_reporter/user_collector.h b/crash_reporter/user_collector.h
index 2ed8e6f..109bf0c 100644
--- a/crash_reporter/user_collector.h
+++ b/crash_reporter/user_collector.h
@@ -52,7 +52,6 @@
FRIEND_TEST(UserCollectorTest, CopyOffProcFilesBadPath);
FRIEND_TEST(UserCollectorTest, CopyOffProcFilesBadPid);
FRIEND_TEST(UserCollectorTest, CopyOffProcFilesOK);
- FRIEND_TEST(UserCollectorTest, ForkExecAndPipe);
FRIEND_TEST(UserCollectorTest, GetIdFromStatus);
FRIEND_TEST(UserCollectorTest, GetProcessPath);
FRIEND_TEST(UserCollectorTest, GetSymlinkTarget);
@@ -95,8 +94,6 @@
FilePath *crash_file_path,
bool *out_of_capacity);
bool CopyStdinToCoreFile(const FilePath &core_path);
- int ForkExecAndPipe(std::vector<const char *> &arguments,
- const char *output_file);
bool RunCoreToMinidump(const FilePath &core_path,
const FilePath &procfs_directory,
const FilePath &minidump_path,
diff --git a/crash_reporter/user_collector_test.cc b/crash_reporter/user_collector_test.cc
index db707f5..06ba9b8 100644
--- a/crash_reporter/user_collector_test.cc
+++ b/crash_reporter/user_collector_test.cc
@@ -7,6 +7,7 @@
#include "base/file_util.h"
#include "crash-reporter/system_logging_mock.h"
#include "crash-reporter/user_collector.h"
+#include "crash-reporter/test_helpers.h"
#include "gflags/gflags.h"
#include "gtest/gtest.h"
@@ -15,12 +16,6 @@
static const char kFilePath[] = "/my/path";
-// This test assumes the following standard binaries are installed.
-static const char kBinBash[] = "/bin/bash";
-static const char kBinCp[] = "/bin/cp";
-static const char kBinEcho[] = "/bin/echo";
-static const char kBinFalse[] = "/bin/false";
-
void CountCrash() {
++s_crashes;
}
@@ -91,69 +86,6 @@
std::string::npos);
}
-TEST_F(UserCollectorTest, ForkExecAndPipe) {
- std::vector<const char *> args;
- char output_file[] = "test/fork_out";
-
- // Test basic call with stdout.
- args.clear();
- args.push_back(kBinEcho);
- args.push_back("hello world");
- EXPECT_EQ(0, collector_.ForkExecAndPipe(args, output_file));
- ExpectFileEquals("hello world\n", output_file);
- EXPECT_EQ("", logging_.log());
-
- // Test non-zero return value
- logging_.clear();
- args.clear();
- args.push_back(kBinFalse);
- EXPECT_EQ(1, collector_.ForkExecAndPipe(args, output_file));
- ExpectFileEquals("", output_file);
- EXPECT_EQ("", logging_.log());
-
- // Test bad output_file.
- EXPECT_EQ(127, collector_.ForkExecAndPipe(args, "/bad/path"));
-
- // Test bad executable.
- logging_.clear();
- args.clear();
- args.push_back("false");
- EXPECT_EQ(127, collector_.ForkExecAndPipe(args, output_file));
-
- // Test stderr captured.
- std::string contents;
- logging_.clear();
- args.clear();
- args.push_back(kBinCp);
- EXPECT_EQ(1, collector_.ForkExecAndPipe(args, output_file));
- EXPECT_TRUE(file_util::ReadFileToString(FilePath(output_file),
- &contents));
- EXPECT_NE(std::string::npos, contents.find("missing file operand"));
- EXPECT_EQ("", logging_.log());
-
- // NULL parameter.
- logging_.clear();
- args.clear();
- args.push_back(NULL);
- EXPECT_EQ(-1, collector_.ForkExecAndPipe(args, output_file));
- EXPECT_NE(std::string::npos,
- logging_.log().find("Bad parameter"));
-
- // No parameters.
- args.clear();
- EXPECT_EQ(127, collector_.ForkExecAndPipe(args, output_file));
-
- // Segmentation faulting process.
- logging_.clear();
- args.clear();
- args.push_back(kBinBash);
- args.push_back("-c");
- args.push_back("kill -SEGV $$");
- EXPECT_EQ(-1, collector_.ForkExecAndPipe(args, output_file));
- EXPECT_NE(std::string::npos,
- logging_.log().find("Process did not exit normally"));
-}
-
TEST_F(UserCollectorTest, HandleCrashWithoutMetrics) {
s_metrics = false;
collector_.HandleCrash(10, 20, "foobar");