Addressing issue #49, offset calculation (#52)

- In the previous implementation there were two issues which this fixes:
  1. While the offset would have increased across loop runs, the end
     iterator was always stuck at the start of the security directory
     plus the length of the current certificate. There was no correction
     for further certificates, as the comments suggested.
  2. The offset after populating the vector was already at value 8 and
     so the calculation of the offset to the next WIN_CERTIFICATE struct
     would end up 8 bytes beyond where it should have.
- Added some additional bounds checking and commented on it in the code
- Adding a doctored PE file based on pegoat-authenticode.exe by merely
  appending the whole WIN_CERTIFICATE once again and doubling the size
  of the security directory given in the optional header. While this
  isn't a validly signed PE file anymore, it provides a contrived test
  case for uthenticode::read_certs() nevertheless.

There still no good test cases for this, other than the possibility to
doctor files with multiple WIN_CERTIFICATE structs in their security
directory, thereby invalidating the actual signature (since we need to
adjust the size of the security directory in the optional header, too).

Author: hugmyndakassi

src/uthenticode.cpp

@@ -389,25 +389,30 @@ std::vector<WinCert> read_certs(peparse::parsed_pe *pe) {
    * bCertificate (uint8_t[dwLength - 8]): the raw certificate data in this entry
    */
   std::vector<WinCert> certs;
-  size_t offset = 0;
-  while (offset < raw_cert_table.size()) {
-    std::uint32_t length = *reinterpret_cast<std::uint32_t *>(raw_cert_table.data() + offset);
+  auto *current_wincert = raw_cert_table.data();
+  auto const *const past_secdir = raw_cert_table.data() + raw_cert_table.size();
+  // Let us tread carefully, we expect all members of WIN_CERTIFICATE up to bCertificate
+  while (current_wincert + 8 < past_secdir) {
+    size_t offset = 0;
+    std::uint32_t length = *reinterpret_cast<std::uint32_t *>(current_wincert + offset);
     offset += sizeof(length);
 
-    std::uint16_t revision = *reinterpret_cast<std::uint16_t *>(raw_cert_table.data() + offset);
+    std::uint16_t revision = *reinterpret_cast<std::uint16_t *>(current_wincert + offset);
     offset += sizeof(revision);
 
-    std::uint16_t type = *reinterpret_cast<std::uint16_t *>(raw_cert_table.data() + offset);
+    std::uint16_t type = *reinterpret_cast<std::uint16_t *>(current_wincert + offset);
     offset += sizeof(type);
 
-    std::vector<std::uint8_t> cert_data(raw_cert_table.data() + offset,
-                                        raw_cert_table.data() + length);
+    // Continue only we can satisfy the length
+    if (current_wincert + length <= past_secdir) {
+      std::vector<std::uint8_t> cert_data(current_wincert + offset, current_wincert + length);
 
-    certs.emplace_back(static_cast<certificate_revision>(revision),
-                       static_cast<certificate_type>(type),
-                       cert_data);
+      certs.emplace_back(static_cast<certificate_revision>(revision),
+                         static_cast<certificate_type>(type),
+                         cert_data);
+    }
 
-    offset += round(length, 8);
+    current_wincert += round(length, 8);
   }
 
   return certs;

test/assets/32/pegoat-authenticode_duplicated.exe

@@ -32,6 +32,22 @@ class Auth32Test : public ::testing::Test {
   peparse::parsed_pe *pe{nullptr};
 };
 
+class Auth32DupeTest : public ::testing::Test {
+ protected:
+  void SetUp() override {
+    auto *file = UTHENTICODE_TEST_ASSETS "/32/pegoat-authenticode_duplicated.exe";
+
+    pe = peparse::ParsePEFromFile(file);
+    ASSERT_TRUE(pe != nullptr);
+  }
+
+  void TearDown() override {
+    peparse::DestructParsedPE(pe);
+  }
+
+  peparse::parsed_pe *pe{nullptr};
+};
+
 class Auth32PlusTest : public ::testing::Test {
  protected:
   void SetUp() override {

test/helpers.h

@@ -25,6 +25,30 @@ TEST_F(Auth32Test, SignedData_properties) {
   ASSERT_EQ(signed_data->get_nested_signed_data(), std::nullopt);
 }
 
+TEST_F(Auth32DupeTest, SignedData_properties) {
+  auto certs = uthenticode::read_certs(pe);
+  EXPECT_EQ(certs.size(), 2);
+
+  for (size_t index = 0; index < certs.size(); index++) {
+    SCOPED_TRACE("certificate # = " + std::to_string(index));
+    auto signed_data = certs[index].as_signed_data();
+
+    ASSERT_TRUE(signed_data.has_value());
+
+    ASSERT_EQ(signed_data->get_signers().size(), 1);
+    ASSERT_EQ(signed_data->get_certificates().size(), 1);
+    ASSERT_TRUE(signed_data->verify_signature());
+
+    auto checksum = signed_data->get_checksum();
+    ASSERT_EQ(std::get<uthenticode::checksum_kind>(checksum), uthenticode::checksum_kind::SHA1);
+    auto const checksumstr = std::get<std::string>(checksum);
+    ASSERT_EQ(checksumstr.size(), 40);
+    ASSERT_STRCASEEQ(checksumstr.c_str(), "6663dd7c24fa84fce7f16e0b02689952c06cfa22");
+
+    ASSERT_EQ(signed_data->get_nested_signed_data(), std::nullopt);
+  }
+}
+
 TEST_F(Auth32PlusTest, SignedData_properties) {
   auto certs = uthenticode::read_certs(pe);
   auto signed_data = certs[0].as_signed_data();

test/signeddata-test.cpp

@@ -25,6 +25,16 @@ TEST_F(Auth32Test, read_certs) {
   EXPECT_TRUE(certs[0].as_signed_data().has_value());
 }
 
+TEST_F(Auth32DupeTest, read_certs) {
+  auto certs = uthenticode::read_certs(pe);
+
+  EXPECT_EQ(certs.size(), 2);
+  for (size_t index = 0; index < certs.size(); index++) {
+    SCOPED_TRACE("certificate # = " + std::to_string(index));
+    EXPECT_TRUE(certs[index].as_signed_data().has_value());
+  }
+}
+
 TEST_F(Auth32PlusTest, read_certs) {
   auto certs = uthenticode::read_certs(pe);
 
@@ -45,6 +55,17 @@ TEST_F(Auth32Test, get_checksums) {
   EXPECT_EQ(std::get<uthenticode::checksum_kind>(checksums[0]), uthenticode::checksum_kind::SHA1);
 }
 
+TEST_F(Auth32DupeTest, get_checksums) {
+  auto checksums = uthenticode::get_checksums(pe);
+
+  EXPECT_EQ(checksums.size(), 2);
+  for (size_t index = 0; index < checksums.size(); index++) {
+    SCOPED_TRACE("checksum # = " + std::to_string(index));
+    EXPECT_EQ(std::get<uthenticode::checksum_kind>(checksums[index]),
+              uthenticode::checksum_kind::SHA1);
+  }
+}
+
 TEST_F(Auth32PlusTest, get_checksums) {
   auto checksums = uthenticode::get_checksums(pe);
 
@@ -88,6 +109,24 @@ TEST_F(Auth32Test, calculate_checksum) {
                    "ea013992f99840f76dcac225dd1262edcec3254511b250a6d1e98d99fc48f815");
 }
 
+TEST_F(Auth32DupeTest, calculate_checksum) {
+  auto unk = uthenticode::calculate_checksum(pe, uthenticode::checksum_kind::UNKNOWN);
+  EXPECT_TRUE(unk.empty());
+
+  auto md5 = uthenticode::calculate_checksum(pe, uthenticode::checksum_kind::MD5);
+  EXPECT_EQ(md5.size(), 32);
+  EXPECT_STRCASEEQ(md5.c_str(), "64c29391b57679b2973ac562cf64685d");
+
+  auto sha1 = uthenticode::calculate_checksum(pe, uthenticode::checksum_kind::SHA1);
+  EXPECT_EQ(sha1.size(), 40);
+  EXPECT_STRCASEEQ(sha1.c_str(), "6663dd7c24fa84fce7f16e0b02689952c06cfa22");
+
+  auto sha256 = uthenticode::calculate_checksum(pe, uthenticode::checksum_kind::SHA256);
+  EXPECT_EQ(sha256.size(), 64);
+  EXPECT_STRCASEEQ(sha256.c_str(),
+                   "ea013992f99840f76dcac225dd1262edcec3254511b250a6d1e98d99fc48f815");
+}
+
 TEST_F(Auth32PlusTest, calculate_checksum) {
   auto unk = uthenticode::calculate_checksum(pe, uthenticode::checksum_kind::UNKNOWN);
   EXPECT_TRUE(unk.empty());
@@ -118,6 +157,13 @@ TEST_F(Auth32Test, verify) {
   EXPECT_TRUE(uthenticode::verify(pe));
 }
 
+TEST_F(Auth32DupeTest, verify) {
+  // File is doctored and therefore signatures (and PE checksum) are invalid, but
+  // this isn't checked here. Instead this checks the signature, but not _against_
+  // the file hash (minus checksum, minus security data directory).
+  EXPECT_TRUE(uthenticode::verify(pe));
+}
+
 TEST_F(Auth32PlusTest, verify) {
   EXPECT_TRUE(uthenticode::verify(pe));
 }

test/uthenticode-test.cpp

@@ -14,6 +14,18 @@ TEST_F(Auth32Test, WinCert_properties) {
   ASSERT_EQ(certs[0].get_type(), uthenticode::certificate_type::CERT_TYPE_PKCS_SIGNED_DATA);
 }
 
+TEST_F(Auth32DupeTest, WinCert_properties) {
+  auto certs = uthenticode::read_certs(pe);
+
+  ASSERT_EQ(certs.size(), 2);
+  for (size_t index = 0; index < certs.size(); index++) {
+    SCOPED_TRACE("certificate # = " + std::to_string(index));
+    ASSERT_TRUE(certs[index].get_length() > 0);
+    ASSERT_EQ(certs[index].get_revision(), uthenticode::certificate_revision::CERT_REVISION_2_0);
+    ASSERT_EQ(certs[index].get_type(), uthenticode::certificate_type::CERT_TYPE_PKCS_SIGNED_DATA);
+  }
+}
+
 TEST_F(Auth32PlusTest, WinCert_properties) {
   auto certs = uthenticode::read_certs(pe);