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

Fix sc_pkcs15_get_bitstring_extension endianness #1141

Merged
merged 1 commit into from Sep 12, 2017

Conversation

Projects
None yet
3 participants
@nwf
Copy link
Contributor

commented Aug 31, 2017

Like asn1's decode_bit_field. However, because we're decoding an
unsigned long long here, not an unsigned int, we can't just
change the decoder to SC_ASN1_BIT_FIELD.

Checklist
  • Tested with the following card: YubiKey 4
    • tested PKCS#11

Tested on ar71xx MIPS box.

@frankmorgner

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

There are several uses of SC_ASN1_TAG_BIT_STRING, did you check the other locations?

Should we check the length of the bit string to match the length of unsigned long long?

@frankmorgner

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

Please fix CI's build error.

@nwf nwf force-pushed the nwf:master branch from feb31e5 to d1f31ef Aug 31, 2017

@nwf

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

Auditing the uses of SC_ASN1_TAG_BIT_STRING is challenging due to the pervasive use of sc_format_asn1_entry, which lexically separates the identifier SC_ASN1_TAG_BIT_STRING from the corresponding .parm and .arg fields of the struct sc_asn1_entry. However, all of them that are paired with SC_ASN1_BIT_FIELD are endian-correct because decode_bit_field does the right thing (assuming the callers are writing into a machine integer and not a u8[]!). That leaves these uses.

src/libopensc/card-gids.c:              { "keyType", SC_ASN1_INTEGER, SC_ASN1_TAG_BIT_STRING | SC_ASN1_CTX, 0, NULL, NULL },
src/libopensc/pkcs15-cert.c:            { "signatureValue",     SC_ASN1_BIT_STRING, SC_ASN1_TAG_BIT_STRING, 0, NULL, NULL },
src/libopensc/pkcs15-cert.c:            { "bitString", SC_ASN1_BIT_STRING, SC_ASN1_TAG_BIT_STRING, 0, value, &val_len },
src/libopensc/pkcs15-pubkey.c:          { "subjectPublicKey", SC_ASN1_BIT_STRING_NI, SC_ASN1_TAG_BIT_STRING, SC_ASN1_ALLOC, NULL, NULL},
src/libopensc/pkcs15-pubkey.c:          { "key",        SC_ASN1_BIT_STRING_NI, SC_ASN1_TAG_BIT_STRING, 0, NULL, NULL },

The second src/libopensc/pkcs15-cert.c match is the one in question. Let me look at the rest quickly.

  • "keyType" is given a .parm of a int * when it is set later by sc_format_asn1_entry, but sc_asn1_decode_integer is endian-correct. (Though its use of a machine-specific int type rather than something like uint32_t is highly suspect.)
  • "signatureValue" truly has a NULL .parm by the time we get to its sc_asn1_decode use, so no decode is done.
  • "subjectPublicKey" decodes into a struct sc_pkcs15_der's .value field, a u8 *, and so is fine.
  • "key" decodes into a struct sc_pkcs15_u8's .value field, also a u8 *, and so is fine.

However, back to the topic at hand: the only uses of sc_pkcs15_get_bitstring_extension are to decode OID 2.5.29.15 in pkcs15-cac.c and pkcs15-piv.c. This OID is defined by https://tools.ietf.org/html/rfc5280#section-4.2.1.3 to use only bits 0 through 8, and so does not justify an entire unsigned long long. I have kicked it over to using an unsigned int and the SC_ASN1_BIT_FIELD decoder instead of the fix I originally proposed. While a more sweeping change, it keeps the endianness glue entirely inside the asn1 decoder. If there's truly need for a 64-bit integer type decode later, it can be added as a SC_ASN1_BIT_FIELD64 or somesuch.

This commit is submitted for review and should be logically the same as the earlier fix. However, the only hardware I have to test upon is in production and I am loathe to take it out for another round.

sc_pkcs15_get_bitstring_extension: int, not long long
Use the ASN.1 decoder's SC_ASN1_BIT_FIELD decoder to properly decode
into a machine word.  As _bitstring_extension is used only for the OID
2.5.29.15 by all callers, which is at most 9 bits wide, this is a
reasonable thing to do.

@nwf nwf force-pushed the nwf:master branch from d1f31ef to 0560bee Aug 31, 2017

@nwf

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

One more revision to try to appease the build bot.

@nwf

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2017

Got a moment to test this patch on my MIPS target and it seemingly works fine; pkcs11-tool -O shows the correct key usage bits.

@frankmorgner

This comment has been minimized.

Copy link
Member

commented Sep 10, 2017

Thanks for the detailed analysis!

@vletoux could you look at card-gids.c?

@vletoux

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2017

I do not have any big endian computer to test on it.
the keyType just put one byte on it so I do not think that the endianness matters here.

You're free to adapt the code and change the asn1 encoding flag accordingly if you like
(when designing I copy paste existing code)

@frankmorgner

This comment has been minimized.

Copy link
Member

commented Sep 12, 2017

OK, thanks for the feedback!

@frankmorgner frankmorgner merged commit 00535f0 into OpenSC:master Sep 12, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.