Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Different result from Signtool #102

Open
zeze-zeze opened this issue Jul 26, 2024 · 5 comments
Open

Different result from Signtool #102

zeze-zeze opened this issue Jul 26, 2024 · 5 comments
Labels
bug Something isn't working C:uthenticode The core uthenticode library

Comments

@zeze-zeze
Copy link

zeze-zeze commented Jul 26, 2024

nc.exe from int0x33/nc.exe is signed and it is verified by signtool.

>signtool verify /pa nc.exe
File: nc.exe
Index  Algorithm  Timestamp
========================================
0      sha1       Authenticode

Successfully verified: nc.exe

But it is not verified by uthenticode.

>svcli nc.exe
This PE is NOT verified!

nc.exe has 1 certificate entries

Calculated checksums:
   MD5: 93013015944D906D98AC97C32274D8E7
  SHA1: 612E98A6DABA999F46EE8CE82176E10B77B60C87
SHA256: F2CB0E58C668B2C289D7CBAD47030E3FB82126180270EE99E69076AFDE997F8E

SignedData entry:
        Embedded checksum: 612E98A6DABA999F46EE8CE82176E10B77B60C87

        Signers:
                Subject: /C=SI/CN=Jernej Simoncic
                Issuer: /C=BE/O=GlobalSign nv-sa/OU=ObjectSign CA/CN=GlobalSign ObjectSign CA
                Serial: 010000000001307A27872D

        Certificates:
                Subject: /C=BE/O=GlobalSign nv-sa/OU=Primary Object Publishing CA/CN=GlobalSign Primary Object Publishing CA
                Issuer: /C=BE/O=GlobalSign nv-sa/OU=Root CA/CN=GlobalSign Root CA
                Serial: 040000000001239E0FACB3

                Subject: /OU=Timestamping CA/O=GlobalSign/CN=GlobalSign Timestamping CA
                Issuer: /C=BE/O=GlobalSign nv-sa/OU=Root CA/CN=GlobalSign Root CA
                Serial: 0400000000012019C19066

                Subject: /C=BE/O=GlobalSign NV/CN=GlobalSign Time Stamping Authority
                Issuer: /OU=Timestamping CA/O=GlobalSign/CN=GlobalSign Timestamping CA
                Serial: 01000000000125B0B4CC01

                Subject: /C=SI/CN=Jernej Simoncic
                Issuer: /C=BE/O=GlobalSign nv-sa/OU=ObjectSign CA/CN=GlobalSign ObjectSign CA
                Serial: 010000000001307A27872D

                Subject: /C=BE/O=GlobalSign nv-sa/OU=ObjectSign CA/CN=GlobalSign ObjectSign CA
                Issuer: /C=BE/O=GlobalSign nv-sa/OU=Primary Object Publishing CA/CN=GlobalSign Primary Object Publishing CA
                Serial: 040000000001239E0FAF24

        This SignedData is invalid!

Some other software has the same problem, while some of them are normal. I understand there are caveats that uthenticode may behave differently to Wintrust API because of not accessing Trusted Publishers store which causes the uthenticode-verified software can't run on some Windows environments. But this situation is that the signature can't be cryptographically verified, which I have no idea why it would happen.

The following are the versions of the dependencies I used:

  • openssl 3.3.1
  • pe-parse 2.1.1
  • uthenticode 2.0.1
@woodruffw
Copy link
Member

But this situation is that the signature can't be cryptographically verified, which I have no idea why it would happen.

We've had some bugs in the past around uthenticode's section/data hashing, producing false negatives where other implementations (like signcode) don't. So that's possibly what's happening here, although in your case it looks like the calculated SHA-1 and embedded SHA-1 do indeed match (612E98A6DABA999F46EE8CE82176E10B77B60C87).

If you have the ability, I suggest stepping through verify_signature and seeing if any of the pre-signature checks fail:

bool SignedData::verify_signature() const {
STACK_OF(X509) *certs = nullptr;
switch (OBJ_obj2nid(p7_->type)) {
case NID_pkcs7_signed: {
certs = p7_->d.sign->cert;
break;
}
/* NOTE(ww): I'm pretty sure Authenticode signatures are always SignedData and never
* SignedAndEnvelopedData, but it doesn't hurt us to handle the latter as well.
*/
case NID_pkcs7_signedAndEnveloped: {
certs = p7_->d.signed_and_enveloped->cert;
break;
}
}
if (certs == nullptr) {
return false;
}
auto *signers_stack_ptr = PKCS7_get0_signers(p7_, nullptr, 0);
if (signers_stack_ptr == nullptr) {
return false;
}
auto signers_stack = impl::STACK_OF_X509_ptr(signers_stack_ptr, impl::SK_X509_free);
/* NOTE(ww): Authenticode specification, page 13: the signer must have the
* codeSigning EKU, **or** no member of the signer's chain may have it.
*
* The check below is more strict than that: **every** signer and embedded
* intermediate cert must have the codeSigning EKU.
*/
/* Check all signing certificates. */
for (auto i = 0; i < sk_X509_num(signers_stack.get()); ++i) {
auto *signer = sk_X509_value(signers_stack.get(), i);
/* NOTE(ww): Ths should really be X509_check_purpose with
* X509_PURPOSE_CODE_SIGN, but this is inexplicably not present
* in even the latest releases of OpenSSL as of 2023-05.
*/
auto xku_flags = X509_get_extended_key_usage(signer);
if (!(xku_flags & XKU_CODE_SIGN)) {
return false;
}
}
/* Check all embedded intermediates. */
for (auto i = 0; i < sk_X509_num(certs); ++i) {
auto *cert = sk_X509_value(certs, i);
auto xku_flags = X509_get_extended_key_usage(cert);
if (!(xku_flags & XKU_CODE_SIGN)) {
return false;
}
}
/* NOTE(ww): What happens below is a bit dumb: we convert our SpcIndirectDataContent back
* into DER form so that we can unwrap its ASN.1 sequence and pass the underlying data
* to PKCS7_verify for verification. This displays our intent a little more clearly than
* our previous approach, which was to walk the PKCS#7 structure manually.
*/
std::uint8_t *indirect_data_buf = nullptr;
auto buf_size = impl::i2d_Authenticode_SpcIndirectDataContent(indirect_data_, &indirect_data_buf);
if (buf_size < 0 || indirect_data_buf == nullptr) {
return false;
}
auto indirect_data_ptr =
impl::OpenSSL_ptr(reinterpret_cast<char *>(indirect_data_buf), impl::OpenSSL_free);
const auto *signed_data_seq = reinterpret_cast<std::uint8_t *>(indirect_data_ptr.get());
long length = 0;
int tag = 0, tag_class = 0;
ASN1_get_object(&signed_data_seq, &length, &tag, &tag_class, buf_size);
if (tag != V_ASN1_SEQUENCE) {
return false;
}
auto *signed_data_ptr = BIO_new_mem_buf(signed_data_seq, length);
if (signed_data_ptr == nullptr) {
return false;
}
impl::BIO_ptr signed_data(signed_data_ptr, BIO_free);
/* Our actual verification happens here.
*
* We pass `certs` explicitly, but (experimentally) we don't have to -- the function correctly
* extracts then from the SignedData in `p7_`.
*
* We pass `nullptr` for the X509_STORE, since we don't do full-chain verification
* (we can't, since we don't have access to Windows's Trusted Publishers store on non-Windows).
*/
auto status = PKCS7_verify(p7_, certs, nullptr, signed_data.get(), nullptr, PKCS7_NOVERIFY);
return status == 1;
}

If not, I can attempt to debug this in a few days.

@woodruffw woodruffw added bug Something isn't working C:uthenticode The core uthenticode library labels Jul 26, 2024
@zeze-zeze
Copy link
Author

zeze-zeze commented Jul 27, 2024

Thanks for the recommendation. After stepping through verify_signature, I found that checking the xku_flags from the second X509_get_extended_key_usage (https://github.com/trailofbits/uthenticode/blob/master/src/uthenticode.cpp#L244) makes the verification fails.

Possible xdu_flags are as follows:

XKU_SSL_SERVER: 1, XKU_SSL_CLIENT: 2, XKU_SMIME: 4, XKU_CODE_SIGN: 8, XKU_OCSP_SIGN: 32, XKU_TIMESTAMP: 64, XKU_DVCS : 128, XKU_ANYEKU: 256

Currently, uthenticode returns false if it doesn't contain XKU_CODE_SIGN (8). However, for all of my false negative test cases, they get only XKU_TIMESTAMP (64) from the second X509_get_extended_key_usage, and that's why verify_signature fails.

From my understanding, XKU_TIMESTAMP is used to verify a file exists and is not modified at a specified timestamp. I'm not quite sure if there is any side effect if we change the line if (!(xku_flags & XKU_CODE_SIGN)) to if (!(xku_flags & (XKU_CODE_SIGN | XKU_TIMESTAMP))).

@woodruffw
Copy link
Member

Thanks for debugging that!

Hmm -- I don't think we can unconditionally widen this to XKU_CODE_SIGN | XKU_TIMESTAMP -- Authenticode follows the rule that all members of the signing chain must either have the codeSigning EKU set or no EKU at all. We currently don't support the "no EKU" case though, since in practice no Authenticode-issuing CAs appear to do that.

That being said, I think I see the underlying problem here: it looks like nc.exe's PKCS#7 blob contains not just normal Authenticode code-signing certs, but also timestamping certs that aren't part of the signing chain. So we should just be ignoring those, since they're not actually part of our verified chain.

To do that, I think uthenticode needs to do one of two things:

  1. Use the correct OpenSSL API to specify EKU behavior during chain verification, rather than checking the certs before actually verifying them, or;
  2. Check the verified chain after building to confirm that every member has the codeSigning EKU (or no EKUs at all)

(1) is preferred since it's strictly more correct from a path validation perspective, but (2) might be easier.

TL;DR: You've hit a bug in uthenticode, but unfortunately I think the patch in #103 is too broad -- we'll need to approach this in a way that doesn't involve uthenticode handling any EKUs besides codeSigning. But thank you for triaging this!

@zeze-zeze
Copy link
Author

@woodruffw, is this issue planned to be fixed in a short time?

  1. Use the correct OpenSSL API to specify EKU behavior during chain verification, rather than checking the certs before actually verifying them, or;
  2. Check the verified chain after building to confirm that every member has the codeSigning EKU (or no EKUs at all)

Can you provide more details about the two methods?

@woodruffw
Copy link
Member

@woodruffw, is this issue planned to be fixed in a short time?

I don't have an immediate timeline for fixing it myself. If someone sends a functional patch (with tests) that doesn't regress the correctness of our other EKU checks, then I'll do my best to review and merge it in a timely manner.

Can you provide more details about the two methods?

Option (1) basically means someone needs to go through OpenSSL's APIs and figure out how to tell it to constrain the EKU during validation, rather than the pre-check we currently do.

Option (2) basically means that we should move the current EKU check from before validation to after it. This is less correct from a chain building perspective, but it's a lot simpler than (1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C:uthenticode The core uthenticode library
Projects
None yet
Development

No branches or pull requests

2 participants