trailofbits / uthenticode

A cross-platform library for verifying Authenticode signatures
https://trailofbits.github.io/uthenticode/
MIT License
133 stars 33 forks source link

Fix hash calculation #62

Closed pkdawson closed 2 years ago

pkdawson commented 2 years ago

Resolves #59

The diff looks confusing because I've moved some things around, so I'll try to explain a bit.

There was a (pe_bits.size() <= cert_table_offset + 8) check near the end of the function which I don't think made sense there, so I moved it to before cert_table_offset is used as an index into pe_bits.

The main change is hashing everything past the header, which I'm doing without allocating another intermediate buffer, just feeding the appropriate data directly into EVP_DigestUpdate. That function cleanly returns an error if it's given 0 bytes, so no additional checks are necessary when hashing any leftovers at the end of the file.

I had to change the expected hashes in NoAuthTest because they were wrong. This can be verified by signing pegoat.exe with a test certificate, then checking it with signtool.

> signtool sign /fd sha256 pegoat.exe
Done Adding Additional Store
Successfully signed: pegoat.exe

> signtool verify /v pegoat.exe
Verifying: pegoat.exe

Signature Index: 0 (Primary Signature)
Hash of file (sha256): 6B7FA3E8298F33BC47F4ABB9C845930B1EACC0DAD96503CFA52D4EA18DDC89F0
woodruffw commented 2 years ago

Changes LGTM, CI is unhappy with some of the formatting:

diff --git a/src/uthenticode.cpp b/src/uthenticode.cpp
index 5a264d9..8a0a1aa [10](https://github.com/trailofbits/uthenticode/runs/6885050812?check_suite_focus=true#step:4:11)0644
--- a/src/uthenticode.cpp
+++ b/src/uthenticode.cpp
@@ -520,8 +520,7 @@ std::string calculate_checksum(peparse::parsed_pe *pe, checksum_kind kind) {
   EVP_DigestInit(md_ctx, md);
   EVP_DigestUpdate(md_ctx, pe_bits.data(), pe_bits.size());
-  if (security_dir.VirtualAddress > 0)
-  {
+  if (security_dir.VirtualAddress > 0) {
     /* If a certificate table exists, hash everything before and after it.
      */
     EVP_DigestUpdate(md_ctx,
@@ -534,14 +533,[11](https://github.com/trailofbits/uthenticode/runs/6885050812?check_suite_focus=true#step:4:12) @@ std::string calculate_checksum(peparse::parsed_pe *pe, checksum_kind kind) {
     EVP_DigestUpdate(md_ctx,
                      pe->fileBuffer->buf + security_dir.VirtualAddress + security_dir.Size,
                      pe->fileBuffer->bufLen - (security_dir.VirtualAddress + security_dir.Size));
-  }
-  else
-  {
+  } else {
     /* If there's no certificate table, just hash the rest of the file.
      */
-    EVP_DigestUpdate(md_ctx,
-                     pe->fileBuffer->buf + size_of_headers,
-                     pe->fileBuffer->bufLen - size_of_headers);
+    EVP_DigestUpdate(
+        md_ctx, pe->fileBuffer->buf + size_of_headers, pe->fileBuffer->bufLen - size_of_headers);
   }
pkdawson commented 2 years ago

Thought my editor would automatically find the .clang-format as usual, but it was silently failing. Oops.

woodruffw commented 2 years ago

Sorry for the delay here! This looks great, merging now. Thanks for all the work.