Commit 2e5a8ccb authored by Dylan Yudaken's avatar Dylan Yudaken Committed by Facebook Github Bot

Fix potential buffer overflow in use of strncat

Summary: There is a bug in ElfFile using strncat which could cause buffer overflows when a previous file open failed. strncat only limits based on the src length, and so if strlen(dest) > 0 this could have overflowed.

Reviewed By: anakryiko

Differential Revision: D13398108

fbshipit-source-id: 793c35884f1e639f36758f36046158858fd5976a
parent e919bad4
......@@ -65,6 +65,10 @@ int ElfFile::openNoThrow(
bool readOnly,
const char** msg) noexcept {
FOLLY_SAFE_CHECK(fd_ == -1, "File already open");
// Always close fd and unmap in case of failure along the way to avoid
// check failure above if we leave fd != -1 and the object is recycled
// like it is inside SignalSafeElfCache
auto guard = makeGuard([&] { reset(); });
strncat(filepath_, name, kFilepathMaxLen - 1);
fd_ = ::open(name, readOnly ? O_RDONLY : O_RDWR);
if (fd_ == -1) {
......@@ -73,10 +77,6 @@ int ElfFile::openNoThrow(
}
return kSystemError;
}
// Always close fd and unmap in case of failure along the way to avoid
// check failure above if we leave fd != -1 and the object is recycled
// like it is inside SignalSafeElfCache
auto guard = makeGuard([&] { reset(); });
struct stat st;
int r = fstat(fd_, &st);
if (r == -1) {
......
......@@ -25,10 +25,10 @@ using folly::symbolizer::ElfFile;
// signatures here to prevent name mangling
uint64_t kIntegerValue = 1234567890UL;
const char* kStringValue = "coconuts";
const char* const kDefaultElf = "/proc/self/exe";
class ElfTest : public ::testing::Test {
protected:
ElfFile elfFile_{"/proc/self/exe"};
ElfFile elfFile_{kDefaultElf};
};
TEST_F(ElfTest, IntegerValue) {
......@@ -76,3 +76,17 @@ TEST_F(ElfTest, NonElfScript) {
EXPECT_EQ(ElfFile::kInvalidElfFile, res);
EXPECT_STREQ("invalid ELF magic", msg);
}
TEST_F(ElfTest, FailToOpenLargeFilename) {
// ElfFile used to segfault if it failed to open large filenames
folly::test::TemporaryDirectory tmpDir;
auto elfFile = std::make_unique<ElfFile>(); // on the heap so asan can see it
auto const largeNonExistingName = (tmpDir.path() / std::string(1000, 'x'));
ASSERT_EQ(
ElfFile::kSystemError,
elfFile->openNoThrow(largeNonExistingName.c_str()));
ASSERT_EQ(
ElfFile::kSystemError,
elfFile->openNoThrow(largeNonExistingName.c_str()));
EXPECT_EQ(ElfFile::kSuccess, elfFile->openNoThrow(kDefaultElf));
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment