Bugzilla

Reporter

Comment 2

•

2 years ago

Attached patch bz_1817553.patch — Details — Splinter Review

Attachment #9318533 - Flags: review?(djackson)

Reporter

Comment 3

•

2 years ago

This is my first patch - it is a simple one line fix. I am not sure how or if I even have access to commit something as a new user. Hopefully an experienced maintainer can review and implement this?

Reporter

Comment 4

•

2 years ago

Should I have written this against NSS instead? It is an NSS issue, part of Firefox source code, that impacts the ability to select a Signing and Encryption certificate under Thunderbird.

Kai Engert (:KaiE:)

Comment 5

•

2 years ago

Ken, thanks for reporting and working on this issue. Yes, this is a NSS issue, I will change the bug component.

I will ask Bob to review your patch, who is our expert for this code.

Assignee: nobody → nobody

Status: UNCONFIRMED → NEW

Component: Security: PSM → Libraries

Ever confirmed: true

Product: Core → NSS

Version: Firefox 110 → other

Kai Engert (:KaiE:)

Updated

•

2 years ago

Attachment #9318533 - Flags: review?(rrelyea)

Updated

•

2 years ago

Attachment #9318533 - Flags: review?(rrelyea) → review-

Comment 6

•

2 years ago

Comment on attachment 9318533 [details] [diff] [review]
bz_1817553.patch

The comment is wrong (While it might appear to be correct in the current case). The nickname is the conatenation of the token name and the CKA_LABEL. The CKA_LABEL is often (but not always) the CN name. It looks like in the paritcular case, some PKCS #11 module is creating tokens with the token name the same as the CN. This is problematic if the CN includes a :.

In any case the current code is preferred because a token name with a ':' is more problematic than a CKA_LABEL with a ':'.

More importantly, 'fixing' this will cause failures in other cases where that cert is imported into a token that already works correctly.

We probably need more information about exactly what is failing, and what token is being used. Hopefully it's one that can be adjusted to be more friendly;).

Reporter

Comment 7

•

2 years ago

My apology on the comment which is wrong about the CN. I didn't want to update this bugzilla until someone replied. I've included the PKCS11 info below.

You are correct, for this particular routine the 'nickname' is the concatenation of the TOKEN LABEL and the object LABEL, not the CN. However, I don't understand the logic behind preferring the existing code, it is incorrect. Whoever wrote that seemed to be concatenating the TOKEN LABEL and Object LABEL and for some reason they chose to use a ':' as a delimiter between those two args. It seems like they were trying to avoid passing in two arguments by using this programming shortcut. Internally they are trying to 'undo' that concatenation using the wrong logic since they assumed the colon they added was the only one. Using PORT_Strchr, they are not reversing what they did in the concatenation.

Alternatively, maybe a different delimiter should be used instead - something that is illegal to use in the PKCS11 standard?

I rebuilt NSS with this fix and it is working fine (for me). But i understand what you are saying if there are existing Object labels with colons in them.

These are smartcards issued by the DoD from IdenTrust. See below.

$ pkcs11-tool -L
Available slots:
Slot 0 (0x0): HID Global OMNIKEY 3x21 Smart Card Reader [OMNIKEY 3x21 Smart...
token label : XXXXXXX X XXXX:A0XXXXXXXXXXXX...
token manufacturer : Common Access Card
token model : PKCS#15 emulated
token flags : login required, rng, token initialized, PIN initialized
hardware version : 0.0
firmware version : 0.0
serial num : 00000000
pin min/max : 4/8

$ pkcs11-tool -O
Using slot 0 with a present token (0x0)
Public Key Object; RSA 2048 bits
label: CAC Cert 7
ID: 0007
Usage: encrypt
Access: none
Certificate Object; type = X.509 cert
label: CAC Cert 7
subject: DN: C=US, O=U.S. Government, OU=ECA, OU=IdenTrust, OU=XXXXXXX X XXXX, CN=XXXXXXX X XXXX:A0XXXXXXXXXXXXXXXXXXXXXXXXXXXXX
ID: 0007
Public Key Object; RSA 2048 bits
label: CAC Cert 9
ID: 0009
Usage: verify
Access: none
Certificate Object; type = X.509 cert
label: CAC Cert 9
subject: DN: C=US, O=U.S. Government, OU=ECA, OU=IdenTrust, OU=XXXXXXX X XXXX, CN=XXXXXXX X XXXX:A0XXXXXXXXXXXXXXXXXXXXXXXXXXXXX
ID: 0009
Profile object 2070099664
profile_id: '4'

Reporter

Comment 8

•

2 years ago

Bob, after some more thought based on your comment, I think I have another idea to fix this properly.

The 'Nickname' used in the code is the concatenation of the CK_TOKEN_INFO label field and the CKA_LABEL attribute related to the storage object.

The OASIS PKCS #11 standard and the NSS source code REQUIRES the following:

typedef struct CK_TOKEN_INFO {
  CK_UTF8CHAR   label[32];           /* blank padded */
  // ... snip
} CK_TOKEN_INFO

Note above that the label MUST be 32 characters, blank padded, and the spec says non-null terminated. That is why my Smartcard label is:

$ pkcs11-tool -L
 token label : XXXXXXX X XXXX:A0XXXXXXXXXXXX...

Based on this observation and to allow existing OBJECT CKA_LABEL attributes containing colons to work, the fix would be to start the strchr search after the 32nd character position. That would guarantee the ':' delimiter is not being searched within the CK_TOKEN_INFO label. This also raises the question as to why a search is even need at all to undo the concatenation? Due to the fixed 32 char nature of the label, shouldn't the delimiter, if it is used, always follow the 32nd character?

Flags: needinfo?(rrelyea)

Comment 9

•

2 years ago

That won't work because NSS truncates token names by removing blank padding at the end, so internally it's a null terminated string with at most 32 characters.

Can you say what PKCS #11 module is presenting the token name as the CN? If it's something like opensc, we might be able to change it. The choice of token name is in the PKCS #11 module and not the actual hardware, usually.

This is the first token I've run into with a colon in the token name (this code is about 3 decades old, and was created to seemlessly add PKCS #11 support to NSS before there was a PKCS #11).

bob

Flags: needinfo?(rrelyea)

Reporter

Comment 10

•

2 years ago

Darn, there goes that suggestion.

Yes, it is Opensc under Ubuntu 22 LTS and I believe p11-kit (which uses opensc as a module?) under RHEL8.

This token 'works' under Thunderbird Windows because the vendor provides its own windows DLL for PKSC11. Under Linux, it is readable with opensc. I have a previous version of the smartcard which required some software called 'Safenet' by Gemalto. While that version of the card also contains colons in the CN it is not readable by opensc. I believe in the past few years they have changed their card programming to be more compatible.

I am pretty sure since there are millions of people with these cards, all issued by IdenTrust for the DoD ECA program, they need to a way to disambiguate people with the same names. Their policy states :

IdenTrust appends a disambiguating number after the colon character in the subject:CommonName field. The
disambiguating number can be generated either by IdenTrust or provided by the Subscribing Organization. The
disambiguating number is also commonly known as and interchangeable with the term “globally unique identifier”
(GUID).

There are only two companies authorized as the issuer of these cards for the DoD ECA program - IdenTrust and Widepoint. I believe Widepoint uses 'periods' rather than colons to disambiguate.

https://github.com/OpenSC/OpenSC/blob/16fdd70a7e02bc87c14d363ca21777b293cdcf2c/src/pkcs11/framework-pkcs15.c#L1203-L1209

Reporter

Comment 11

•

2 years ago

Bob, not sure if this helps... I did some gdb debugging of opensc - below is where the label is being set. It would appear the card is being detected as a Common Access Card (CAC). That detection sets the label of the 'Auth' pin object to 'PIN'. The result of that setting executes the code path below (an auth label of "PIN" is for some reason referred to in the code as 'useless')

} else {
	/* PIN label is empty or just says useless "PIN",
	 * print only token label */
	strcpy_bp(slot->token_info.label,
			p15card->tokeninfo ? p15card->tokeninfo->label : "",
			32);
}

Comment 12

•

2 years ago

I've just cc'd Jakub on this bug. He's the red hat opensc guy. Jakub, we're running into an issue where openSC is using the CN as the token name, but the given CN has a ':' in the name, which breaks NSS if the token name has a ':' (NSS can't reference a cert by nickname in that case). I wonder if you could open an upsream opensc issue on this?

Reporter

Comment 13

•

2 years ago

One possible fix (hack?) is the following:

Right after the strcpy_bp() in framework-pkcs15.c (the code I pointed out above), I added the following to replace any colons with hyphens since NSS cannot reference a nickname that contains a colon.:

               for(int idx=0; idx<32; idx++) {
                    if (slot->token_info.label[idx] == ':') {
                        slot->token_info.label[idx] = '-';
                    }
                }

This code works too (without any NSS modification).

Comment 14

•

2 years ago

jakub Jelen can you look at comment 12 and comment 13 ? Thanks.

Comment 15

•

2 years ago

Jakub Jelinek, sorry for the noise, you can remove yourself from this bug if you want:)

Comment 16

•

2 years ago

The CN of the certficate issued is of the following format:
Fullname MiddleInitial Lastname:X0000000000000...

Interesting. The CAC cards I have seen always had the CN in the format Fullname.Lastname.NNNNNN without any colons. But these were quite old cards. Is it some newly issued card?

I am not much fan of modifying the token label from the CN. I think it can make some other applications fail which will depend on the token label being the CN.

We briefly discussed this with Bob last week, but it looks like this will have to be handled with either some configuration option or quirk specific for NSS so it won't choke on colons in token label. I filled an issue in OpenSC:
https://github.com/OpenSC/OpenSC/issues/2725

The other option would be that NSS would "sanitize" these fields before storing and using them. The strstr() is fragile on any user-data which might contain colons.

Reporter

Comment 17

•

2 years ago

Jakub, just to be clear. These cards are issued for the DoD by IdenTrust for what is called the DoD ECA (External Certificate Authority) program. I believe they used to be unreadable without special software because they were not formatted properly. In the past few years, newer versions of the cards have been formatted differently and can be read by OpenSC. While these are issued on behalf of the DoD, these are not technically Common Access Cards, even though OpenSC detects it as such and the card itself labels the cert objects are CAC Cert 7 and CAC Cert 9 for signing and encryption. The use of colons in the CN name is based on their official CA issuance policy which I quoted in Comment 10.

I do agree the NSS code is fragile. It seems its goal was to concatenate strings together with a colon, but when undoing that concatenation the code fails to handle an embedded colon in the CN.

Background on DoD ECA: https://www.identrust.com/digital-certificates/dod-eca-programs
There is an second company that issues these DoD ECA tokens, Widepoint, and their CA policy just so happens to use periods in their CN names.

Comment 18

•

2 years ago

Where does the nickname come from? I think we should make a step away from the place where we are trying to fix the stuff now. Place where you propose a patch already parses nickname that is no longer unambiguously parsable. So my proposal would be to have a look into the place where these nicknames are created from pkcs11 token structures and later stored. We should sanitize the token name there to make sure it does not contain any colons to create unambiguous nicknames.

When we will parse the nickname here, we can unambiguously split the parts and convert the same way the real token name we will be comparing the read values. Bob, what do you think, would something like this work?

Indeed, we can workaround this inside of OpenSC, but I see any replacement of colons in the token label on the OpenSC side only as a workaround to the NSS issue. And there are dozens of other PKCS#11 drivers which might sooner or later come with the same issue

BugBot [:suhaib / :marco/ :calixte]

Comment 19

•

2 years ago

The applications/users paste the token name and the label together. It's a semantic NSS added in the late 90's to allow you to reference a certificate on a token through existing interfaces (which used to just take the label on the cert in the database, actually it still does). If there is a colon, it take first part as a tokenname and the second part as the label. If the token doesn't exist, it takes the whole label. In may tokens, the CN is used for the label (rather than the whole token), since there are often many certs on the token that don't share the same CN.

In any case the actual semantic of tokenname:label is part of NSS for over 20 years and not something that can be changed. In that time this is the first instance we have run into of a token having a ':' in the token name.

I'm fine with an option that says 'NSS_HACK_NO_COLONS_IN_TOKEN_NAME' as the config option, though...

bob

Comment 20

•

2 years ago

The severity field is not set for this bug.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(bbeurdouche)

https://github.com/OpenSC/OpenSC/pulls

Comment 21

•

2 years ago

Ken, do you want to open a PR for this change in OpenSC? Looking into this further, you solution is probably the best for now

Benjamin Beurdouche [:beurdouche]

Comment 22

•

2 years ago

Opened an new PR for OpenSC basically with your patch, but using underscores instead of dashes: https://github.com/OpenSC/OpenSC/pull/2760
Waiting for reviews now.

I did this change uniformly for now. If it will turn out problematic in some other applications we can try to limit it only to some NSS applications later.

Comment 23

•

1 year ago

Bob, could you please set priority and severity for this ? : ) Thanks !

Flags: needinfo?(bbeurdouche) → needinfo?(rrelyea)

Comment 24

•

1 year ago

My take would by probably to close this as this was resolved in OpenSC (should be in 0.24.0 due in coming weeks) and there should not be any action needed for NSS to take (except it might make sense to document this limitation of NSS somewhere so next time somebody using different pkcs11 module will not have to go through this bug, code and bob's knowledge to figure this out.