mtrojnar / osslsigncode

OpenSSL based Authenticode signing for PE/MSI/Java CAB files
Other
805 stars 131 forks source link

Wrong signing time at timestamp (v2.7) #304

Closed westyles closed 1 year ago

westyles commented 1 year ago

Wrong signing time at timestamp! The current time is set. cmd ..... -TSA-time 1514754000 -time 1514754000 g5EkhDM But time for timestamp ok.

Through signtool.exe and the local time server all dates are correct.

p.s. And for some reason the file is always different in the output, with the same signature parameters.

westyles commented 1 year ago

I think the output file is always different, with the same signing parameters, precisely because of the current signing time for timestamp!

olszomal commented 1 year ago

-TSA-certs, -TSA-key and -TSA-time options can be only used by the built-in TSA responder.

$ osslsigncode sign -in unsigned.exe -out signed.exe -certs cert.pem -key key.pem -time 1567296000 -TSA-certs TSA.pem -TSA-key TSA.key -TSA-time 1567296000 
$ osslsigncode verify -in signed.exe -CAfile CACert.pem -TSA-CAfile TSACA.pem
(...)
Authenticated attributes:
    Signing time: Sep  1 00:00:00 2019 GMT
    Microsoft Individual Code Signing purpose
    Message digest: ED2CF03C79C03BCB691002A2E8314493D97302AC6C90D8A472CB1D24B5AB364B 

The signature is timestamped: Sep  1 00:00:00 2019 GMT
Hash Algorithm: sha256
Timestamp Verified by:
        Issuer : /C=PL/O=osslsigncode/OU=Timestamp Authority Root CA/CN=TSA Root CA
        Serial : 1979B175128FB256E00640698C98D29C59C3506A
(...)
westyles commented 1 year ago

Hello. Thank you for your reply. Of course I used the built-in TSA responder

I use osslsigncode v2.7 Here are the full commands in the cmd console and screenshots of the signatures: 1). osslsigncode builtin tsa (Wrong Signing time for timestamp, but time for timestamp is OK): https://i.imgur.com/iNY9uei.png 2). osslsigncode + signtool + local server tsa (All OK) https://i.imgur.com/zNolk7z.png 3). osslsigncode + local server tsa (All OK): https://i.imgur.com/rg41WFS.png

p.s. Why it gives errors when checking the result, I don't know, maybe I specified something wrong. In fact, such signatures work! But the wrong time signature in the built-in tsa every time a timestamp is signed.

Also if the timestamp is done via singtool.exe, the checksum of the file is always the same!

If you do the timestamp signature via osslsigncode.exe, the checksum of the file is always different, regardless of the tsa method. This may not be important, but I wanted to let you know.

westyles commented 1 year ago

I can send these certificates to you for a test if needed.

westyles commented 1 year ago

-TSA-certs, -TSA-key and -TSA-time options can be only used by the built-in TSA responder. Answered you above.

olszomal commented 1 year ago

Thanks @westyles for your explanations. I didn't understand this problem, sorry. I'm working on this issue.

olszomal commented 1 year ago

RFC 3161 Timestamp token encapsulates a signed data content type. Encapsulated content includes genTime value. This is a time at which the time-stamp token has been created by the TSA. Only this time is used for verifying signatures.

Timestamp token is signed, so it also contains PKCS#9 signing time. This is a time at which the signer (TSA) performed the signing process. This signing time is informative and not checked because does not affect the signature verification process. Could you elaborate why this signing time would be important?

The following patch, although not suitable for submitting to OpenSSL, will force indicated time to also be set in a signer_info struct:

diff --git i/crypto/ts/ts_rsp_sign.c w/crypto/ts/ts_rsp_sign.c
index 79d3e67837..5f4e6ad7de 100644
--- i/crypto/ts/ts_rsp_sign.c
+++ w/crypto/ts/ts_rsp_sign.c
@@ -678,6 +678,8 @@ static int ts_RESP_sign(TS_RESP_CTX *ctx)
     BIO *p7bio = NULL;
     int i;
     EVP_MD *signer_md = NULL;
+    ASN1_GENERALIZEDTIME *time;
+    long sec, usec;

     if (!X509_check_private_key(ctx->signer_cert, ctx->signer_key)) {
         ERR_raise(ERR_LIB_TS, TS_R_PRIVATE_KEY_DOES_NOT_MATCH_CERTIFICATE);
@@ -723,6 +725,20 @@ static int ts_RESP_sign(TS_RESP_CTX *ctx)
         ERR_raise(ERR_LIB_TS, TS_R_PKCS7_ADD_SIGNED_ATTR_ERROR);
         goto err;
     }
+    if (ctx->time_cb(ctx, ctx->time_cb_data, &sec, &usec)) {
+        time = TS_RESP_set_genTime_with_precision(NULL, sec, usec,
+ ctx->clock_precision_digits);
+        if (time == NULL) {
+            ERR_raise(ERR_LIB_TS, TS_R_PKCS7_ADD_SIGNED_ATTR_ERROR);
+            goto err;
+        }
+        if (!PKCS7_get_signed_attribute(si, NID_pkcs9_signingTime)) {
+            if (!PKCS7_add0_attrib_signing_time(si, time)) {
+                ERR_raise(ERR_LIB_TS, TS_R_PKCS7_ADD_SIGNED_ATTR_ERROR);
+                goto err;
+            }
+        }
+    } 

I will improve the verification logs:

Authenticated attributes:
    Signing time: Jul  1 00:00:00 2023 GMT
    Microsoft Individual Code Signing purpose
    Message digest: ED2CF03C79C03BCB691002A2E8314493D97302AC6C90D8A472CB1D24B5AB364B 

Countersignatures:
    Timestamp time: Jul  1 00:00:00 2023 GMT
    Signing time: Oct  5 09:15:22 2023 GMT
    Hash Algorithm: sha256
    Issuer: /C=PL/O=osslsigncode/OU=Timestamp Authority Root CA/CN=TSA Root CA
    Serial: 73A8FD01E89A2E0A36D1878FA41F41ACADD79A
westyles commented 1 year ago

The following patch, although not suitable for submitting to OpenSSL, will force indicated time to also be set in a signer_info struct: I will improve the verification logs:

Hi. This timestamp signing time with a different value looks broken and incorrect. With other methods of this utility and other utilities this time is set correctly, and it should be correct. You should try to do everything correctly, and I would like osslsigncode to achieve the highest possible level of quality of work. Especially since this correction task is not difficult.

Good thing they added a caption to the timestamp signing time output, will be handy to watch! Only in your output, the timestamp signing time is not the same as the Timestamp time. All 3 time entries must match to be correct.

mtrojnar commented 1 year ago

All 3 time entries must match to be correct.

This is a very interesting idea. Which RFC section defines this requirement?

westyles commented 1 year ago

This is a very interesting idea. Which RFC section defines this requirement?

I am not a native English speaker and it is difficult for me to discuss in more detail and choose the right words to be understood correctly. And I see that not everything was clear in the sense I wanted it to be. RFC section 1 in the preface specifies word usage rules for describing the structure of signatures with different requirements or necessities:

The key words "MUST", "MUST NOT", "REQUIRED", "SHOULD", "SHOULD NOT", "SHALL", "RECOMMENDED", "MAY", and "OPTIONAL" in this document (in uppercase, as shown) are to be interpreted as described in [RFC2119].

Not all signature parameters are needed directly to fulfill their functions. But optionally can be used to maintain the same look and result.

I'm not forcing you, only suggesting that this point be finalized. If my arguments are not significant for you, you can do as you wish - leave it as it is now, and close this issue. I'll continue to use the local server instead of the built-in TSA then.

mtrojnar commented 1 year ago

Feel free to use the OpenSSL patch written by @olszomal if you feel that it's necessary for your particular use case or your sense of esthetics. There is nothing we can do in osslsigncode anyway, as there is no OpenSSL API for a custom timestamp signing time, as opposed to the timestamp time itself.

westyles commented 1 year ago

Feel free to use the OpenSSL patch written by @olszomal

Thanks. I would love to use your suggested code for the patch, but I don't know how to do it. And I couldn't compile osslsigncode from source via MSYS2 MinGW on Windows 10, something was wrong all the time. Spent a whole day following the advice on the net. Nothing worked.

mtrojnar commented 1 year ago

You don't need to build osslsigncode from source to use the OpenSSL patch. It's enough to build a patched OpenSSL and replace the DLLs.

westyles commented 1 year ago

You don't need to build osslsigncode from source to use the OpenSSL patch. It's enough to build a patched OpenSSL and replace the DLLs.

I fixed ts_rsp_sign.c file by olszomal code manually and build openssl-3.1.2 x64 and x86! The patch code is probably for a different version of openssl, because the file line numbers don't match. Replaced libcrypto-3-x64.dll from openssl x64 and signature time is the same everywhere now!!!! I haven't checked it on x86.

Immediately I realized that for built-in TSA there is not enough separate -h indication (e.g. -TSA-h) to specify a different algorithm if the certificate for the timestamp is different. In this case it is already important. I managed to get out of the situation in 2 passes:

osslsigncode sign -in vfd.sys      -out vfd_sign.sys -spc Client+Int.crt -key Client+Int.key -ac Cross.crt -nolegacy -h sha1 -time 1420059600 -verbose 
osslsigncode add  -in vfd_sign.sys -out vfd_sign+ts.sys -h sha256 -TSA-certs TSA_sha256.crt -TSA-key TSA_sha256.key -TSA-time 1420059600 -verbose

I hope this option is the right one. Create a new issue for the suggestion to add -TSA-h ? Or does it not fit the important terms? I just don't want to stress you out if such an addition is not important to you. I have a couple other suggestions, but I don't know if I should write them anymore, since it doesn't affect the working functionality.