Addressing issue #49, offset/length calculation

[hugmyndakassi]

• The size of the certificate data WIN_CERTIFICATE::bCertificate is the size dwLength minus the offset of the bCertificate in the struct.

[hugmyndakassi]

Actually I didn't get the unit tests to run as of yet. The compiler gave me quite a few errors when trying with CMake and the -DBUILD_TESTS=1 which was suggested in the README. I need to review how to get this to build successfully so I can run the test suite locally, too.

Either during, but more likely after the weekend.

[woodruffw]

ASan doesn't like the changes, JFYI:

[  FAILED  ] Auth32Test.read_certs (0 ms)
[ RUN      ] Auth32Test.get_checksums
AddressSanitizer:DEADLYSIGNAL
/home/runner/work/uthenticode/uthenticode/test/uthenticode-test.cpp:44: Failure
=================================================================
==2786==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000020 (pc 0x5630413ec3d8 bp 0x7fffc9dd9050 sp 0x7fffc9dd9010 T0)
==2786==The signal is caused by a READ memory access.
==2786==Hint: address points to the zero page.

[woodruffw]

Re: https://github.com/trailofbits/uthenticode/pull/53#issuecomment-1024640447: these changes are causing memory corruption, so it's highly likely that your interpretation of the PE's certificate table structure is wrong.

Here's the relevant docs from Microsoft: https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#the-attribute-certificate-table-image-only

Those docs imply that your code is right, but it wouldn't be the first time Microsoft's docs have been wrong.

Here's how WINE does it: https://github.com/wine-mirror/wine/blob/master/dlls/imagehlp/integrity.c#L483-L546

[hugmyndakassi]

Looking into this with ASan on my end. For the purpose of testing this it doesn't really matter what comes after the first certificate, as long as the next one is yet again a half-way valid WIN_CERTIFICATE struct.

[hugmyndakassi]

Okay, so it's definitely wrong to read length bytes into the vector at (now) line 403. There are no two ways about it. So on line 404 there should be the - 8 after the length as you pointed out in your comment. But then it gets strange. Because obviously the offset has at that point already been changed compared to when the loop was entered.

And indeed when single-stepping through the Debug-configured Auth32Test.Certificate_properties test case into uthenticode::read_certs(), without the above I end up with a strange result from round().

Since 1432 (the value of WIN_CERTIFICATE::dwLength) is already 8-byte aligned, so by the end of the loop we should either way end up with 1432. But we get 1440.

And now it dawns on me that the main reason for this is that the WINE implementation you referred to does not change offset in the course of the loop, but in uthenticode::read_certs() offset is bumped up to 8 prior to adding WIN_CERTIFICATE::dwLength.

Now what I do not yet fully get is why the adjustment causes an out of bounds error; or rather why that didn't happen before when the full amount of length was requested at offset. Because if I look at the file I can see the whole security directory is 1432 bytes. So correcting for the offset (which was done correctly before), we have 1424 bytes left to read. But since we request length which is 1432 at this point it should already fail right there. I don't quite get why this doesn't happen. Still debugging it with the Debug config on Linux.

[hugmyndakassi]

Ah, my bad. Of course during the initialization of the vector we give the begin and end iterator. So we won't at this point overstep and so correcting the length value before this point will wreak havoc.

However, after that point we need to correct for the offset being 8 already. Essentially the offset needs to be reset to the value when entering a given iteration.

Edit: Here's what I am going to do. I'll doctor a PE file with a mock WIN_CERTIFICATE and then it should become more evident.

[hugmyndakassi]

And one more. the end iterator when filling the vector has to be based on the start offset of the current certificate + length added. But currently it takes the beginning of the security directory instead of the current certificate struct.

[hugmyndakassi]

I'll amend my change again as to provide a test case with a doctored file.

[hugmyndakassi]

Added another bounds check in the latest commit. From my perspective it concludes this pull request.

[woodruffw]

LGTM! Thanks especially for adding a doctored test to exercise the codepath.