Not sure about this one, but it could be a logic error

[hugmyndakassi]

Inside uthenticode::read_certs() you add to offset while traversing WIN_CERTIFICATE structures which may exist 8-byte-aligned in the PE security directory. You then enter a while loop and add to it based on the WIN_CERTIFICATE members.

Overall in one loop iteration you do:

offset += sizeof(dwLength) + sizeof(wRevision) + sizeof(wCertificateType) + round(dwLength, 8);

I think with the last one you overshoot (well, technically you'd simply land inside the subsequent WIN_CERTIFICATE::bCertificate; but that would likely lead to all sorts of undesired effects).

The reason being that sizeof(bCertificate) is dwLength - 8 (i.e. sizeof(bCertificate) == dwLength - offsetof(WIN_CERTIFICATE, bCertificate)).

Or in other words: to find the next WIN_CERTIFICATE you have to take the original offset from when you entered the loop, add round(dwLength, 8) to that and then look at the next one.

This particular case doesn't seem to be covered by your test suite (for a lack of samples, I reckon?), which would explain why this went unnoticed. However, there's a good chance I simply have a wrinkle in my brain and am just blabbering some garbage 😉

[woodruffw]

Hmm, I think you're right.

Specifically, this line:

    std::vector<std::uint8_t> cert_data(raw_cert_table.data() + offset,
                                        raw_cert_table.data() + length);

should probably be:

    std::vector<std::uint8_t> cert_data(raw_cert_table.data() + offset,
                                        raw_cert_table.data() + length - 8);

...right? I'll do some testing and see whether that breaks anything.

(Also, if you know of a good way to generate a PE with multiple WIN_CERTIFICATE blobs, let me know! We're definitely missing good coverage for that.)

[woodruffw]

Ugh, I remember why we don't have coverage for this -- I tried to create a PE with multiple sequential signatures, but signtool.exe creates them with nested signatures instead.

If you know a good way to get signtool.exe or the Set-AuthenticodeSignature cmdlet to embed multiple certificates as separate WIN_CERTIFICATEs, let me know.

[hugmyndakassi]

I haven't seen any binary multiple (non-nested) certificates either, to be honest.

I didn't mean line 400 but rather line 407 where it proceeds to the next WIN_CERTIFICATE structure. But you are right - and I missed that - line 400 is also affected by the same issue.

That said, I think there's a rather simple solution to this. The amount of offsetof(WIN_CERTIFICATE, bCertificate) (== 8) can be subtracted from length on line 399 or earlier and then everything falls in place, even for the issue on line 407, which I originally meant.

[woodruffw]

Yep, that sounds good to me. I'll create a fix PR later today.

[hugmyndakassi]

I'm on the tohex thingamy anyway, so I can take this one as well, if you want.

[woodruffw]

Sure, if you don't mind! Separate PRs are preferred 🙂

[hugmyndakassi]

If you know a good way to get signtool.exe or the Set-AuthenticodeSignature cmdlet to embed multiple certificates as separate WIN_CERTIFICATEs, let me know.

ClamAV cites a number of samples which have curious signatures, but I don't know how to convince signtool to append to the security directory instead of nesting. I have a hunch where this may be used, though (ELAM).

Negative serial doesn't seem to be such a big deal, I've seen those when using makecert for test-signing.