Hi Juan,
I need some advice.
Attached is a patch to fix bug 34388, where an app that tries to verify its code signature fails because Wine, while parsing the certificates embedded within the signature, encounters an item in the CMS signer info (tag 0x31, i.e. ASN_UNIVERSAL | ASN_CONSTRUCTOR | 0x11), right before the HashEncryptionAlgorithm sequence item, that it doesn't yet know how to decode. This patch (which just skips the item in question) allows that app to successfully verify its code signature and run, but...
My testing on Windows (cf. job 2037 on newtestbot) shows some interesting behavior. Windows will indeed accept this item, but only if the AuthAttrs item is also present and immediately precedes it in the sequence. (The other test bails out with CRYPT_E_ASN_BADTAG.) I don't quite know what to make of this. The odd thing is that the certificate in question doesn't have this optional AuthAttrs item, and yet (in most cases, at least) most people who run this particular app on Windows do not have this problem. I can't seem to find anything about this from reading the relevant RFCs. Is there something I'm missing?
Thanks.
Chip
Hiya Chip,
I don't have an answer off the top of my head. But let's see if we can unpack that ASN.1 a bit.
First, with the type, notice that crypt32_private.h defines a few useful types that Microsoft omitted for some reason from their public header, including: #define ASN_SETOF (ASN_UNIVERSAL | ASN_PRIMITIVE | 0x11) Okay, so that's what we're dealing with.
Next, to help sort it out: write each of the blobs to a binary, and interpret it with "openssl asn1parse -dump -i -inform DER". Here's the output from PKCSSignerWithUnknownItem31:
0:d=0 hl=4 l= 146 cons: SEQUENCE 4:d=1 hl=2 l= 1 prim: INTEGER :00 7:d=1 hl=2 l= 25 cons: SEQUENCE 9:d=2 hl=2 l= 20 cons: SEQUENCE 11:d=3 hl=2 l= 18 cons: SET 13:d=4 hl=2 l= 16 cons: SEQUENCE 15:d=5 hl=2 l= 3 prim: OBJECT :commonName 20:d=5 hl=2 l= 9 prim: PRINTABLESTRING :Juan Lang 31:d=2 hl=2 l= 1 prim: INTEGER :01 34:d=1 hl=2 l= 6 cons: SEQUENCE 36:d=2 hl=2 l= 2 prim: OBJECT :1.2.3 40:d=2 hl=2 l= 0 prim: NULL 42:d=1 hl=2 l= 96 cons: SET 44:d=2 hl=2 l= 0 prim: EOC (a bunch of these omitted) 140:d=1 hl=2 l= 6 cons: SEQUENCE 142:d=2 hl=2 l= 2 prim: OBJECT :1.5.6 146:d=2 hl=2 l= 0 prim: NULL 148:d=1 hl=2 l= 0 prim: OCTET STRING
And here's the output from PKCSSignerWithAuthAttrAndUnknownItem31: 0:d=0 hl=4 l= 196 cons: SEQUENCE 4:d=1 hl=2 l= 1 prim: INTEGER :00 7:d=1 hl=2 l= 25 cons: SEQUENCE 9:d=2 hl=2 l= 20 cons: SEQUENCE 11:d=3 hl=2 l= 18 cons: SET 13:d=4 hl=2 l= 16 cons: SEQUENCE 15:d=5 hl=2 l= 3 prim: OBJECT :commonName 20:d=5 hl=2 l= 9 prim: PRINTABLESTRING :Juan Lang 31:d=2 hl=2 l= 1 prim: INTEGER :01 34:d=1 hl=2 l= 6 cons: SEQUENCE 36:d=2 hl=2 l= 2 prim: OBJECT :1.2.3 40:d=2 hl=2 l= 0 prim: NULL 42:d=1 hl=2 l= 32 cons: cont [ 0 ] 44:d=2 hl=2 l= 30 cons: SEQUENCE 46:d=3 hl=2 l= 3 prim: OBJECT :commonName 51:d=3 hl=2 l= 23 cons: SET 53:d=4 hl=2 l= 21 cons: SEQUENCE 55:d=5 hl=2 l= 19 cons: SET 57:d=6 hl=2 l= 17 cons: SEQUENCE 59:d=7 hl=2 l= 3 prim: OBJECT :commonName 64:d=7 hl=2 l= 10 prim: PRINTABLESTRING :Juan Lang 76:d=1 hl=2 l= 96 cons: SET 78:d=2 hl=2 l= 0 prim: EOC (again, a bunch of these omitted) 174:d=1 hl=2 l= 6 cons: SEQUENCE 176:d=2 hl=2 l= 2 prim: OBJECT :1.5.6 180:d=2 hl=2 l= 0 prim: NULL 182:d=1 hl=2 l= 16 prim: OCTET STRING 0000 - 00 01 02 03 04 05 06 07-08 09 0a 0b 0c 0d 0e 0f ................
You can see that in the latter case, you've got a context-specific tag of 0 wrapping a sequence containing a non-empty set. That context-specific [0] looks an awful lot like "signedAttrs [0] IMPLICIT SignedAttributes OPTIONAL," no? And SignedAttributes is defined as "SignedAttributes ::= SET SIZE (1..MAX) OF Attribute".
Anyway, in the latter case, the signed attributes precede the set, and in the former case they're missing.
In addition to that, it's not entirely clear from your test results that it's expected to succeed in Windows in either case:
+ ret = pCryptDecodeObjectEx(dwEncoding, PKCS7_SIGNER_INFO, + PKCSSignerWithAuthAttrAndUnknownItem31, + sizeof(PKCSSignerWithAuthAttrAndUnknownItem31), + CRYPT_DECODE_ALLOC_FLAG, NULL, &buf, &size); + if (ret)
You don't have an ok(ret) here. I can't blame you too much for that, though: it looks like I omitted a bunch of ok(ret)s in my tests too. Sigh. Sorry about that. Could you add that and see what that turns up? --Juan
On Thu, Sep 5, 2013 at 11:57 PM, Charles Davis cdavis5x@gmail.com wrote:
Hi Juan,
I need some advice.
Attached is a patch to fix bug 34388, where an app that tries to verify its code signature fails because Wine, while parsing the certificates embedded within the signature, encounters an item in the CMS signer info (tag 0x31, i.e. ASN_UNIVERSAL | ASN_CONSTRUCTOR | 0x11), right before the HashEncryptionAlgorithm sequence item, that it doesn't yet know how to decode. This patch (which just skips the item in question) allows that app to successfully verify its code signature and run, but...
My testing on Windows (cf. job 2037 on newtestbot) shows some interesting behavior. Windows will indeed accept this item, but only if the AuthAttrs item is also present and immediately precedes it in the sequence. (The other test bails out with CRYPT_E_ASN_BADTAG.) I don't quite know what to make of this. The odd thing is that the certificate in question doesn't have this optional AuthAttrs item, and yet (in most cases, at least) most people who run this particular app on Windows do not have this problem. I can't seem to find anything about this from reading the relevant RFCs. Is there something I'm missing?
Thanks.
Chip
On Sep 6, 2013, at 1:10 PM, Juan Lang wrote:
Hiya Chip,
I don't have an answer off the top of my head. But let's see if we can unpack that ASN.1 a bit.
First, with the type, notice that crypt32_private.h defines a few useful types that Microsoft omitted for some reason from their public header, including: #define ASN_SETOF (ASN_UNIVERSAL | ASN_PRIMITIVE | 0x11) Okay, so that's what we're dealing with.
I guess that makes sense... even though the tag in question is an ASN_CONSTRUCTOR instead of an ASN_PRIMITIVE. I must've overlooked that because it didn't have the ASN_CONSTRUCTOR bit set.
Next, to help sort it out: write each of the blobs to a binary, and interpret it with "openssl asn1parse -dump -i -inform DER". Here's the output from PKCSSignerWithUnknownItem31:
0:d=0 hl=4 l= 146 cons: SEQUENCE 4:d=1 hl=2 l= 1 prim: INTEGER :00 7:d=1 hl=2 l= 25 cons: SEQUENCE 9:d=2 hl=2 l= 20 cons: SEQUENCE11:d=3 hl=2 l= 18 cons: SET 13:d=4 hl=2 l= 16 cons: SEQUENCE 15:d=5 hl=2 l= 3 prim: OBJECT :commonName 20:d=5 hl=2 l= 9 prim: PRINTABLESTRING :Juan Lang 31:d=2 hl=2 l= 1 prim: INTEGER :01 34:d=1 hl=2 l= 6 cons: SEQUENCE 36:d=2 hl=2 l= 2 prim: OBJECT :1.2.3 40:d=2 hl=2 l= 0 prim: NULL 42:d=1 hl=2 l= 96 cons: SET 44:d=2 hl=2 l= 0 prim: EOC (a bunch of these omitted) 140:d=1 hl=2 l= 6 cons: SEQUENCE 142:d=2 hl=2 l= 2 prim: OBJECT :1.5.6 146:d=2 hl=2 l= 0 prim: NULL 148:d=1 hl=2 l= 0 prim: OCTET STRING
And here's the output from PKCSSignerWithAuthAttrAndUnknownItem31: 0:d=0 hl=4 l= 196 cons: SEQUENCE 4:d=1 hl=2 l= 1 prim: INTEGER :00 7:d=1 hl=2 l= 25 cons: SEQUENCE 9:d=2 hl=2 l= 20 cons: SEQUENCE 11:d=3 hl=2 l= 18 cons: SET 13:d=4 hl=2 l= 16 cons: SEQUENCE 15:d=5 hl=2 l= 3 prim: OBJECT :commonName 20:d=5 hl=2 l= 9 prim: PRINTABLESTRING :Juan Lang 31:d=2 hl=2 l= 1 prim: INTEGER :01 34:d=1 hl=2 l= 6 cons: SEQUENCE 36:d=2 hl=2 l= 2 prim: OBJECT :1.2.3 40:d=2 hl=2 l= 0 prim: NULL 42:d=1 hl=2 l= 32 cons: cont [ 0 ] 44:d=2 hl=2 l= 30 cons: SEQUENCE 46:d=3 hl=2 l= 3 prim: OBJECT :commonName 51:d=3 hl=2 l= 23 cons: SET 53:d=4 hl=2 l= 21 cons: SEQUENCE 55:d=5 hl=2 l= 19 cons: SET 57:d=6 hl=2 l= 17 cons: SEQUENCE 59:d=7 hl=2 l= 3 prim: OBJECT :commonName 64:d=7 hl=2 l= 10 prim: PRINTABLESTRING :Juan Lang 76:d=1 hl=2 l= 96 cons: SET 78:d=2 hl=2 l= 0 prim: EOC (again, a bunch of these omitted) 174:d=1 hl=2 l= 6 cons: SEQUENCE 176:d=2 hl=2 l= 2 prim: OBJECT :1.5.6 180:d=2 hl=2 l= 0 prim: NULL 182:d=1 hl=2 l= 16 prim: OCTET STRING 0000 - 00 01 02 03 04 05 06 07-08 09 0a 0b 0c 0d 0e 0f ................
You can see that in the latter case, you've got a context-specific tag of 0 wrapping a sequence containing a non-empty set. That context-specific [0] looks an awful lot like "signedAttrs [0] IMPLICIT SignedAttributes OPTIONAL," no? And SignedAttributes is defined as "SignedAttributes ::= SET SIZE (1..MAX) OF Attribute".
Yep, I remember reading that in the specs, but I was thrown off by Wine expecting some custom tag (ASN_CONTEXT | ASN_CONSTRUCTOR | 0) for that.
Anyway, in the latter case, the signed attributes precede the set, and in the former case they're missing.
In addition to that, it's not entirely clear from your test results that it's expected to succeed in Windows in either case:
- ret = pCryptDecodeObjectEx(dwEncoding, PKCS7_SIGNER_INFO,
PKCSSignerWithAuthAttrAndUnknownItem31,sizeof(PKCSSignerWithAuthAttrAndUnknownItem31),CRYPT_DECODE_ALLOC_FLAG, NULL, &buf, &size);- if (ret)
You don't have an ok(ret) here. I can't blame you too much for that, though: it looks like I omitted a bunch of ok(ret)s in my tests too. Sigh. Sorry about that. Could you add that and see what that turns up?
I did what you said, and it turns out that my initial analysis (if you can call it that) was wrong. Windows Crypto doesn't accept the SET either way (cf. job 2056 on newtestbot).
In the "AuthAttrsAndUnknownItem" case, it might be because it already encountered the AuthAttrs set and didn't expect to find another one. But I suspect that in the other case (just with the "UnknownItem"), it's because Crypto was expecting to find something in the set, but didn't. (I did, after all, fill the set with zeroes.)
Maybe then the real fix is to make Wine accept either a constructor SET or the custom tag (ASN_CONTEXT | ASN_CONSTRUCTOR) it currently accepts, for either attribute set. I should come up with a test case first, though, to see if that's what Windows does. I'll get back to you on that.
Chip
--Juan
On Thu, Sep 5, 2013 at 11:57 PM, Charles Davis cdavis5x@gmail.com wrote: Hi Juan,
I need some advice.
Attached is a patch to fix bug 34388, where an app that tries to verify its code signature fails because Wine, while parsing the certificates embedded within the signature, encounters an item in the CMS signer info (tag 0x31, i.e. ASN_UNIVERSAL | ASN_CONSTRUCTOR | 0x11), right before the HashEncryptionAlgorithm sequence item, that it doesn't yet know how to decode. This patch (which just skips the item in question) allows that app to successfully verify its code signature and run, but...
My testing on Windows (cf. job 2037 on newtestbot) shows some interesting behavior. Windows will indeed accept this item, but only if the AuthAttrs item is also present and immediately precedes it in the sequence. (The other test bails out with CRYPT_E_ASN_BADTAG.) I don't quite know what to make of this. The odd thing is that the certificate in question doesn't have this optional AuthAttrs item, and yet (in most cases, at least) most people who run this particular app on Windows do not have this problem. I can't seem to find anything about this from reading the relevant RFCs. Is there something I'm missing?
Thanks.
Chip
On Fri, Sep 6, 2013 at 3:54 PM, Charles Davis cdavis5x@gmail.com wrote:
Maybe then the real fix is to make Wine accept either a constructor SET or the custom tag (ASN_CONTEXT | ASN_CONSTRUCTOR) it currently accepts, for either attribute set. I should come up with a test case first, though, to see if that's what Windows does. I'll get back to you on that.
Yeah, that seems plausible, as either some sort of BER/DER thing or just two alternate encodings for the same value. I'm not certain, but tests will definitely help. --Juan
On Sep 6, 2013, at 5:01 PM, Juan Lang wrote:
On Fri, Sep 6, 2013 at 3:54 PM, Charles Davis cdavis5x@gmail.com wrote: Maybe then the real fix is to make Wine accept either a constructor SET or the custom tag (ASN_CONTEXT | ASN_CONSTRUCTOR) it currently accepts, for either attribute set. I should come up with a test case first, though, to see if that's what Windows does. I'll get back to you on that.
Yeah, that seems plausible, as either some sort of BER/DER thing or just two alternate encodings for the same value. I'm not certain, but tests will definitely help.
So much for that theory.
I tried twice to replace the CONSTRUCTOR | CONTEXT tag with the generic CONSTRUCTOR | SET tag (jobs 2057 and 2058 on newtestbot). Both times, Crypto bailed out. I don't think Windows will accept a CONSTRUCTOR | SET tag there under any circumstances. I think we're seriously screwing up somewhere reading the code signature. Trouble is, I don't know where, or if this is even a problem in Wine at all--it might be peculiar to my system. (The other Wine users who reported being unable to run the Star Citizen Launcher because of this were missing a root CA.)
I don't know why, but strangely, the problem with the Launcher that prompted my patch seems to have gone away (at least, on my system) with the update they just released. Maybe it's related to the problems a few people were having with this on Windows. Maybe the signature really was malformed.
Chip
On Fri, Sep 6, 2013 at 9:17 PM, Charles Davis cdavis5x@gmail.com wrote:
On Sep 6, 2013, at 5:01 PM, Juan Lang wrote:
On Fri, Sep 6, 2013 at 3:54 PM, Charles Davis cdavis5x@gmail.com wrote:
Maybe then the real fix is to make Wine accept either a constructor SET or the custom tag (ASN_CONTEXT | ASN_CONSTRUCTOR) it currently accepts, for either attribute set. I should come up with a test case first, though, to see if that's what Windows does. I'll get back to you on that.
Yeah, that seems plausible, as either some sort of BER/DER thing or just two alternate encodings for the same value. I'm not certain, but tests will definitely help.
So much for that theory.
I tried twice to replace the CONSTRUCTOR | CONTEXT tag with the generic CONSTRUCTOR | SET tag (jobs 2057 and 2058 on newtestbot). Both times, Crypto bailed out. I don't think Windows will accept a CONSTRUCTOR | SET tag there under any circumstances. I think we're seriously screwing up somewhere reading the code signature. Trouble is, I don't know where, or if this is even a problem in Wine at all--it might be peculiar to my system. (The other Wine users who reported being unable to run the Star Citizen Launcher because of this were missing a root CA.)
If you've got the signature that was failing, please keep it handy, and we can make it into a test case. If not, keep an eye on it to see if it recurs. What I've done in the past is to modify crypt32 to write a failing cert, sig, whatever to /tmp, and made a test case out of it.
I don't know why, but strangely, the problem with the Launcher that
prompted my patch seems to have gone away (at least, on my system) with the update they just released. Maybe it's related to the problems a few people were having with this on Windows. Maybe the signature really was malformed.
That wouldn't be unheard of. I've forgotten which game it was, but at least once some game server was sending a malformed signature that happened to work on some broken Windows systems. IIRC, they ended up fixing it once a Windows service pack started rejecting it. --Juan
-
Charles Davis -
Juan Lang