code clean ups and simplifications

[fabled]

This is initial clean ups and simplifications for the code base. I'm planning to work on implementing a more extensive pkcs15 secret-key support.

[dengert]

Have any developers ever seen any cards with p15 objects that could not be parsed by these changes?

If so how were these objects created, OpenSC? Some national ID card, other card issuer or other software?

In other words does PKCS#15 ansi follow X.680 30.6c?
It looks like it does as "PKCS#15 v1.1 (June 2000)"

"F.2 Explicit tagging of parameterized types"

"When a parameterized type, such as the PKCS15Object, contains a context tag, that tag will
be encoded as an EXPLICIT tag regardless of the tagging policy of the module in which the
parameterized type occurs. See further the section on tagging in [19]."

"[19] ISO/IEC 8824-1:1998 | ITU-T Recommendation X.680 (1997),..."

(The cards I have, all use emulation, i.e. don't have p15 objects on the cards, so I don't use pkcs15init or have to deal with p15 asn1 or have any way to test these changes.)

[fabled]

Yes, PKCS#15 follows X.680 on this. This is also evident on the existing code base from the fact that explicit tagging was implemented properly for all other object types via the extra sequence.

I have not seen any cards with these object types. But as OpenSC does not implement (yet) secret-key creation, it would have to be mistaken proprietary implementation generating them if any exists. For the data objects, I believe it is possible that OpenSC has been used to create incorrect encodings, and those might be in the wild. Though, I have no idea on their usage in general.

[dengert]

When creating your Secret Key changes, keep in mind that PKCS#11 supports session objects that may or may not reside on the card. In addition key derivation, especially ECDH, generates a secret key to be which may or may not be stored on the card. The current code in OpenSC for secret key objects was added to support the ECDH derivation where the card generates a GENERIC secret key that is returned as part of the ECDH operation and stored in a session based secret key in memory and is later retrieved by the application by as a PKCS#11 attribute.

The current code is messy and may need to be changed. But it must allow for the secret key be in memory as a PKCS#11 session object.

[fabled]

@dengert Ack. Thanks. I wrote the original pkcs11 code a decade ago, but it seems to have had major changes since... That explains the extra variable in sc_pkcs15_key_info.

The changes I have in mind are:

• libopensc - Add skdf encoding; implement aes key encoding/decoding per pkcs15v1.1 (I have ISO7816-15:2016 but it's not explicit how to do it, and I hope to get clarifications on it and do that later on); extend the algorithm handling for symmetric algorithms
• pkcs15init (library + tool) - Add generic secret-key object support, and secret-key generation API and implement it for myeid

Neither of this limits the use of in-memory objects... It's just bolting in the code to writing and reading objects in skdf.

I am also aware of issue #627, but seems it was a bit off from what and how things should be done.

[viktorTarasov]

It has to be rather:
+ return sc_compare_path(&((struct sc_pkcs15_skey_info *) data)->path, path);

[fabled]

fixed. added few more clean ups as well.

[viktorTarasov]

With this PR there is an error when parsing the privateRSAKey for Gemalto IAS/ECC card.
asn1_decode_entry: decoding 'subjectName' -- 310B3009060355040A0C026F74310B300906...
SubjectName SEQUENCE enveloppe is skipped and data starts from the first RDN's SET .

To compare with the same for current master:
asn1_decode_entry: decoding 'subjectName' -- 303C310B3009060355040A0C026F74310B300906...

With this PR applied the skip-asn1-tag has probably be removed . But now I cannot estimate how deep could be the consequence.

I do not tried to encode privateRSAKey.

[fabled]

I tested this only with EC keys. Do you have example encoding, or full logs of what is happening? I think the asn1_skip_tag thingy is part of how the asn1 decoded and should not be touched. Probably I made a mistake in the systematic conversion of removing the explicit sequence tags.

[fabled]

It looks like c_asn1_com_prkey_attr needs to have the SC_ASN1_TAG_SEQUENCE removed and be handled as raw OCTET SEQUENCE instead. I'll go through the other class attributes to see if such fix up is needed else where. The problem is that the explicit tagging was not understood and the removal of the extra sequence tag was done in all sub structures.

[fabled]

Would it be possible to get opensc-explorer asn1 dump? It maybe that the encoding on the card is incorrect. Was it created with opensc? It seems to me that the opensc's encoding (and decoding) of the private and public key subclass common attributes has been incorrect (missing the explicit sequence tag like it is/was for secretkey/datacontainerobject type attributes).

[viktorTarasov]

@fabled

I think the asn1_skip_tag thingy is part of how the asn1 decoded and should not be touched

Yes, something like this.
OpenSC ASN.1 module as presented by sc_asn1_entry... data types do not thoroughly represent the standard. The differences are corrected by procedures. One of such cases is rightly the asn1_skip_tag -- when decoding, the explicit tag is removed by code to fit the existing ASN.1 module description.

There is the content of PrKDF with one key:

00000000  30 81 ad 30 34 0c 17 54  65 73 74 20 49 41 53 2f
00000010  45 43 43 20 45 6e 63 72  20 32 30 34 38 44 03 02  
00000020  06 c0 04 01 c1 30 12 30  08 03 03 06 60 40 04 01
00000030  c1 30 06 03 02 07 80 05  00 30 27 04 14 30 2d 50
00000040  03 dd 32 e4 42 7a 03 3e  19 5c 8f 19 a6 8e 25 bc
00000050  8f 03 02 04 30 03 02 04  b0 02 02 00 81 a1 03 02
00000060  01 05 a0 40 30 3e 30 3c  31 0b 30 09 06 03 55 04
00000070  0a 0c 02 6f 74 31 0b 30  09 06 03 55 04 0b 0c 02
00000080  73 63 31 20 30 1e 06 03  55 04 03 0c 17 54 65 73
00000090  74 20 49 41 53 2f 45 43  43 20 45 6e 63 72 20 32
000000a0  30 34 38 44 a1 0a 30 08  30 02 04 00 02 02 08 00

The same dumped with dumpasn1:

  0 173: SEQUENCE {
  3  52:   SEQUENCE {                                   <--- CommonObjectAttrs
  5  23:     UTF8String 'Test IAS/ECC Encr 2048D'
 30   2:     BIT STRING 6 unused bits
       :       '11'B
 34   1:     OCTET STRING C1
 37  18:     SEQUENCE {
 39   8:       SEQUENCE {
 41   3:         BIT STRING 6 unused bits
       :           '1000000110'B
 46   1:         OCTET STRING C1
       :         }
 49   6:       SEQUENCE {
 51   2:         BIT STRING 7 unused bits
       :           '1'B (bit 0)
 55   0:         NULL
       :         }
       :       }
       :     }
 57  39:   SEQUENCE {                                   <--- CommonKeyAttrs
 59  20:     OCTET STRING 30 2D 50 03 DD 32 E4 42 7A 03 3E 19 5C 8F 19 A6 8E 25 BC 8F
 81   2:     BIT STRING 4 unused bits
       :       '1100'B
 85   2:     BIT STRING 4 unused bits
       :       '1101'B
 89   2:     INTEGER 129
 93   3:     [1] {
 95   1:       INTEGER 5
       :       }
       :     }
 98  64:   [0] {                                        <--- Explicit Tag
100  62:     SEQUENCE {                                 <--- CommonPrivateKeyAttrs
102  60:       SEQUENCE {                               <--- Subject
104  11:         SET {
106   9:           SEQUENCE {
108   3:             OBJECT IDENTIFIER organizationName (2 5 4 10)
113   2:             UTF8String 'ot'
       :             }
       :           }
117  11:         SET {
119   9:           SEQUENCE {
121   3:             OBJECT IDENTIFIER organizationalUnitName (2 5 4 11)
126   2:             UTF8String 'sc'
       :             }
       :           }
130  32:         SET {
132  30:           SEQUENCE {
134   3:             OBJECT IDENTIFIER commonName (2 5 4 3)
139  23:             UTF8String 'Test IAS/ECC Encr 2048D'
       :             }
       :           }
       :         }
       :       }
       :     }
164  10:   [1] {                                        <--- Explicit Tag
166   8:     SEQUENCE {                                 <--- RSAPrivateKeyAttrs
168   2:       SEQUENCE {
170   0:         OCTET STRING
       :           Error: Object has zero length.
       :         }
172   2:       INTEGER 2048
       :       }
       :     }
       :   }

@fabled

Would it be possible to get opensc-explorer asn1 dump?

opensc-explorer has asn1 option to print decoded ASN1 data of EF:

[vtarasov@sophora opensc-OpenSC (master *)]$ ./build/bin/opensc-explorer 
OpenSC Explorer version 0.16.0
Using reader with a card: OMNIKEY AG CardMan 3121 00 00
OpenSC [3F00]> cd aid:E828BD080FD25047656E65726963
OpenSC [E828/BD08/0FD2/5047/656E/6572/6963]> asn1 7001
Printing tags for buffer of length 100
30 Univ: tag 0x10, length  79: SEQUENCE
  30 Univ: tag 0x10, length  48: SEQUENCE
...

Personally, I prefer to get the binary content of EF with get option and use dumpasn1.

[fabled]

I think the asn1_skip_tag thingy is part of how the asn1 decoded and should not be touched

Yes, something like this. OpenSC ASN.1 module as presented by sc_asn1_entry... data types do not thoroughly represent the standard. The differences are corrected by procedures. One of such cases is rightly the asn1_skip_tag -- when decoding, the explicit tag is removed by code to fit the existing ASN.1 module description.

Looking asn1_skip_tag() it reads exactly one tag, and matches it against expected tag value. If matched, it returns the contents. Otherwise it just returns null object. It never removes an inner explicit tag. Thus there's no problem there.

I just realized the problem. The subjectName is read to / written from an user controlled buffer. And it's expected to be the full presentation of it, including the encapsulating SEQUENCE tag. However, asn1_decode_entry has always and should strip the containing tag. The code only worked because there's two SEQUENCE tags. And currently if CommonPrivateKeyAttributes has additional members present those would be returned in the subjectName DER field which is clearly wrong.

The proper way to solve this is either to add a new SC_ASN1_RAW (or _DER) encoding option that expects the caller to ship DER presentation of the field including the containing tag (verify it matches the asn1_entry's tag type). Other option is to change the "struct sc_pkcs15_der subject" to not include the outer SEQUENCE tag, but this might have bigger implications.

It seems this is a wider problem in the code base. E.g. pkcs15-cert.c supports decoding 'direct' encoded certificate. It is matched using the Context tag. Because the value is of DummyReference type it gets explicit tagged, and again the caller visible "struct sc_pkcs15_der cert" gets the containing SEQUENCE tag. Same issue in pkcs15-pubkey.c.

[viktorTarasov]

@fabled

I just realized the problem. The subjectName is read to / written from an user controlled buffer. And it's expected to be the full presentation of it, including the encapsulating SEQUENCE tag. However, asn1_decode_entry has always and should strip the containing tag. The code only worked because there's two SEQUENCE tags. And currently if CommonPrivateKeyAttributes has additional members present those would be returned in the subjectName DER field which is clearly wrong.

Afais, the skipped tag is the explicit context tag subClassAttributes.
The CommonPrivateKeyAttributes SEQUENCE is kept, and so, beside the subjectName, other attributes can be encoded/decoded.

[fabled]

@viktorTarasov

98  64:   [0] {                                        <--- Outer tag. Matches https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/asn1.c#L1151
100  62:     SEQUENCE {                                 <--- EXPLICIT inner tag. Matches currently incorrectly at https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/pkcs15-prkey.c#L82
102  60:       SEQUENCE {                               <--- Not matching anything. This should match the above asn1 definition.
104  11:         SET {
106   9:           SEQUENCE {
108   3:             OBJECT IDENTIFIER organizationName (2 5 4 10)
113   2:             UTF8String 'ot'
       :             }

The sc_asn1_skip_tag() is ill-named. Maybe it's function changed throughout time. But currently it should be better named as "sc_asn1_read_and_verify_tag()". It does not do (and there's no way it could even do) any explicit tag processing because the rule when explicit tag is present is not universal, but it depends on the asn1 definition and context.

You can verify this from the fact that all private key attributes have definitions to process one more SEQUENCE tag via https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/pkcs15-prkey.c#L96 or similar match.

So as comparison the private key matches are as follows:

164  10:   [1] {                                        <--- Outer tag, matches https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/asn1.c#L1152
166   8:     SEQUENCE {                         <--- EXPLICIT tag, matches https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/pkcs15-prkey.c#L97
168   2:       SEQUENCE {                      <--- Path value, matches https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/pkcs15-prkey.c#L89
170   0:         OCTET STRING
       :           Error: Object has zero length.
       :         }
172   2:       INTEGER 2048              <--- modulusLength, matches https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/pkcs15-prkey.c#L90

As you see, for each tag in there is a struct sc_asn1_entry match. For private key attributes they are done right. For the subclassAttributes it's done incorrectly for the reasons explained above, but it sort of works currently because the contents are just passed to/from caller which is expected to include the containing tag. This is also the reason why encoding of certain objects (e.g. data containers) is currently non-conforming to the the PKCS#15 ASN.1 definition.

[mouse07410]

I have a naive question: how come that other cards (like PIV) work correctly now, without this PR? And cards (I've only PIV experience) with objects written by OpenSC interoperate with other software (like PKard)?

[fabled]

@mouse07410 Because the problem occurs only on specific object types and on specific object fields. The current code is right for many parts. And in some cases there's two bugs that "cancel out" each other in most of the current use cases. The bug really affects only the pkcs15 'data container' objects and certain key attributes which in most cases are not stored.

The changes I did uncovered these issues, and it would be good to fix all of it to be standard compliant.

[viktorTarasov]

@fabled
I will return to your comments later, for a while few moments that I've seen:

102 60: SEQUENCE { <--- Not matching anything. This should match the above asn1 definition.

This SEQUENCE matches the subjectName itself:
Acording to PKCS#15 the subjectName is a Name.

Name ::= CHOICE { -- only one possibility for now --rdnSequence  RDNSequence }
RDNSequence ::= SEQUENCE OF RelativeDistinguishedName
RelativeDistinguishedName ::=  SET SIZE (1..MAX) OF AttributeTypeAndDistinguishedValue
AttributeTypeAndDistinguishedValue ::= SEQUENCE {
  type                  ATTRIBUTE.&id({SupportedAttributes}),
  value                 ATTRIBUTE.&Type({SupportedAttributes}{@type}),
  ....

The bug really affects only the pkcs15 'data container' objects and certain key attributes which in most cases are not stored.

Please, show here the invalid content encoded by OpenSC, or correct content that OpenSC do not arrives to decode.

[fabled]

@viktorTarasov

This SEQUENCE matches the subjectName itself:

No. Currently it matches the EXPLICIT tag as marked in the previous comment. And yes, the code clearly shows that it is intended to match the subjectName itself. To be clear here, I'm talking about the OpenSC sc_asn1_entry definition matches; not the ASN1 specification. That's why it is obviously a bug and OpenSC current sc_asn1_entry definitions are mismatched against the ASN1 specification of the corresponding types. The only reason why it works is that the subjectName field is parsed to struct sc_pkcs15_der which is expected to include the containing tag of Name (a SEQUENCE tag). Thus these two bugs cancel out each other in cases when there's no additional attributes in the CommonPrivateKeyAttributes.

I can generate a data object encoding. All those should be incorrect. I'll post one soon.

[fabled]

EF.DCOD after pkcs15-init -W foo.txt is as follows:

OpenSC [3F00/5015]> asn1 4406
Printing tags for buffer of length 1530
30 Univ: tag 0x10, length  33: SEQUENCE
  30 Univ: tag 0x10, length   4: SEQUENCE
    03 Univ: tag 0x03, length   2: BIT STRING [10]
  30 Univ: tag 0x10, length  13: SEQUENCE
    0C Univ: tag 0x0C, length  11: UTF8STRING [pkcs15-init]
  A1 Cntx: tag 0x01, length  10: 
    30 Univ: tag 0x10, length   8: SEQUENCE
      04 Univ: tag 0x04, length   6: OCTET STRING [3F0050154601]
Zero tag, finishing

It is missing the explicit SEQUENCE tag between the A1 Context and the 30 Univ (SEQ) tag. It should look like:

  A1 Cntx: tag 0x01, length  12:        --> Tag for [1] TypeAttributes
    30 Univ: tag 0x10, length   10: SEQUENCE        --> EXPLICIT tag (SEQUENCE tag of OpaqueDOAttributes ::= ObjectValue which is a sequence)
      30 Univ: tag 0x10, length   8: SEQUENCE        --> SEQUENCE tag of Path
        04 Univ: tag 0x04, length   6: OCTET STRING [3F0050154601]        --> indirect presentations' efidOrPath OCTET STRING

[viktorTarasov]

@fabled

... OpaqueDOAttributes ::= ObjectValue which is a sequence ...

From PKCS #15 v1.1 ch. 6.1.6 "ObjectValue"

ObjectValue { Type } ::= CHOICE {
  indirect ReferencedValue {Type},
  direct [0] Type,
...

[fabled]

@viktorTarasov Oh, right. I was confusing my self with ISO7816-15 OidDOAttributes content for DataContainerObjectChoice which is a SEQUENCE. Seems the Data objects are proper. I will update my patch accordingly.

However, the reason why "Name subject" changed is still valid. I will probably fix this in my patch by adding a new SC_ASN1_RAW_DER encoding format which copies expect struct sc_pkcs15_der and it to have the containing tag also.

[viktorTarasov]

@fabled

.. fix this in my patch by adding a new SC_ASN1_RAW_DER encoding format ..

Agree with your analyse -- in the current ASN1 framework the CommonPrivateKeyAttributes, extended with the other attributes, cannot be properly encoded.

[fabled]

Maybe let's drop the Explicit clean up from this pull request, and handle that commit separately. I'd prefer to get the first two commits in first if there's no objection for those.

[viktorTarasov]

Agree, beforehand commit the first ones (I'll do it),
then, I think, rename the pkcs15-xx data members to show which of them hold its data in DER encoded form (in attempt to avoid the double encoding, as it's now the case for subject.). Then implement RAW_DER tag or EXPLICIT tag, together with, for example, implementing of CredentialIdentifier, to extend the CommonPrivateKeyAttributes with keyIdentifiers, to be able to test the updated ASN1 framework ... We will think it over .

[frankmorgner]

@fabled, @viktorTarasov, the progress on this PR seems stalled. How are you going to proceed?

[fabled]

I removed the work-in-progress commit, and rebased the existing ones. Care to review/pull?

[frankmorgner]

thanks, looks good! I'll leave this up for review for some more of days.