977673 - prevent users from disabling the internal module

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Elio Maldonado]

Users sometimes want have an external cryptographic module be the preferred provider, opencryptoki on fedora and RHEL is an exemple. They have in the past as part of their setting up disabled the nss internal module. After a few years they reported an "nss segfaults with opencryptoki module" bug. They shouldn't have disabled softoken to begin, over the years they just were lucky. NSS should be changes to return an error when that attempt is made.

Comment 1

[Elio Maldonado]

These are the step to reproduce using cryptoki on Linux:

First, make sure that your pkcsslotd service is running and also that the user you are using is in pkcs11 group, see /usr/share/doc/opencryptoki-3.0/README

As root:
groupadd pkcs11
chmod -G pkcs11 test
/bin/systemctl start pkcsslotd.service

Now as the test (the user)
mkdir -p nssdb
echo 'redhat' > nssdb/nsspassword
certutil -N -d sql:nssdb -f nssdb/nsspassword
while [ 1 -eq 1 ]; do dd if=/dev/urandom bs=1 count=1; echo; sleep 0.014576; done 2>/dev/null | certutil -S -n cacert -s 'cn=CAcert' -x -t 'C,C,C' -m 1 -v 120 -d sql:nssdb -f nssdb/nsspassword
certutil -L -d sql:nssdb
modutil -force -dbdir sql:nssdb -nocertdb -add opencryptoki -libfile /usr/lib64/opencryptoki/PKCS11_API.so
modutil -force -dbdir sql:nssdb -nocertdb -disable 'NSS Internal PKCS #11 Module'
modutil -force -dbdir sql:nssdb -default opencryptoki -mechanisms  RSA:DSA:DH:RC2:RC4:RC5:AES:CAMELLIA:DES:MD2:MD5:SHA1:SHA256:SHA512:SSL:TLS:RANDOM
modutil -force -dbdir sql:nssdb -list
while [ 1 -eq 1 ]; do dd if=/dev/urandom bs=1 count=1; echo; sleep 0.015796; done 2>/dev/null | certutil -f nssdb/nsspassword -R -s 'CN=John Smith, O=Netscape, L=Mountain View, ST=California, C=US' -p '650-555-8888' -o mycert.req -d sql:nssdb -f nssdb/nsspassword

# this steps will fa
certutil -L -d sql:nssdb
Segmentation fault

Running in the debugger will show
-----------------------------------
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7630315 in nssToken_IsPresent (token=0x0) at devtoken.c:1441
1441	    return nssSlot_IsTokenPresent(token->slot);
(gdb) bt
#0  0x00007ffff7630315 in nssToken_IsPresent (token=0x0) at devtoken.c:1441
#1  0x00007ffff75d6dd1 in PK11_TraverseCertsInSlot (slot=0x6aa370, callback=0x7ffff75d7c4f <listCertsCallback>, arg=0x7fffffffda80)
    at pk11cert.c:2094
#2  0x00007ffff75d7e07 in PK11_ListCertsInSlot (slot=0x6aa370) at pk11cert.c:2624
#3  0x000000000040b2e7 in listCerts (handle=0x6a05c0, name=0x0, email=0x0, slot=0x6aa370, raw=0, ascii=0, outfile=0x630a70, 
    pwarg=0x7fffffffdd90) at certutil.c:528
#4  0x000000000040b489 in ListCerts (handle=0x6a05c0, nickname=0x0, email=0x0, slot=0x6aa370, raw=0, ascii=0, outfile=0x630a70, 
    pwdata=0x7fffffffdd90) at certutil.c:572
#5  0x0000000000411d32 in certutil_main (argc=4, argv=0x7fffffffdf38, initialize=1) at certutil.c:2954
#6  0x00000000004131ff in main (argc=4, argv=0x7fffffffdf38) at certutil.c:3426
(gdb) print token
$1 = (NSSToken *) 0x0 <-------- this what caused the segfault
(gdb) 
-----------------------------------------------------------------

Bob's comments:
1) When I look in the debugger I see that the internal slot was explicitly disabled. This shouldn't happen and is an application/test error. We probably should modify NSS to prevent explicitly disabling the internal token.
2) We shouldn't crash in this case, we should probably test for token == to NULL and fail.

The original bug was that the test was getting lucky. It shouldn't ever disable the internal token. Even old versions of NSS would fail at this point if the stack happened to be just right.

Comment 2

[Elio Maldonado]

Attachment: don't disable internal module

Comment 3

[Wan-Teh Chang]

Comment on attachment 8383177 [details] [diff] [review]
don't disable internal module

Review of attachment 8383177 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. But please remove devtoken.c from the patch. It seems
unrelated to this issue, and I am not sure if that change is
a good idea.

I also have suggested changes for pk11slot.c.

::: lib/dev/devtoken.c
@@ +1437,5 @@
>  nssToken_IsPresent (
>    NSSToken *token
>  )
>  {
> +    if (token == NULL) return PR_FALSE;

Is this change necessary? It seems unrelated to the subject
of this bug (the internal module). Also, passing a NULL token
may be a programming error, so we probably should not allow
that.

::: lib/pk11wrap/pk11slot.c
@@ +1500,5 @@
>  /* returns PR_TRUE if successfully disable the slot */
>  /* returns PR_FALSE otherwise */
>  PRBool PK11_UserDisableSlot(PK11SlotInfo *slot) {
>  
> +    if (slot->isInternal) return PR_FALSE;

1. Add a comment to say we want to prevent users from disabling
the internal module. I wonder if we should also set an error code.
I didn't find a suitable error code. I guess we can use
SEC_ERROR_INVALID_ARGS as a placeholder.

2. Nit: put the return statement on the next line?

Comment 4

[Robert Relyea]

Elio,

WTC is correct about the nssToken_IsPresent, it's a separate issue (though it was part of the same Red Hat Bug). Go ahead and create a separate bug for this issue and copy wtc's comments and this one to that bug.

WTC, if the user happens to disable the internal token (which is why we realized we had to prevent disabling the internal token), then NSS will get and internal slot without an attached NSS token. The NULL check handles this case (not that nssToken_IsPresent is currently only used internally in NSS). So either we make this NULL check here, or do it where ever we call nssToken_IsPresent(). The semantic of the token being NULL is that the slot is disabled, and therefore not present.

bob

Comment 5

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #3)
> 
> 1. Add a comment to say we want to prevent users from disabling
> the internal module. I wonder if we should also set an error code.
> I didn't find a suitable error code. I guess we can use
> SEC_ERROR_INVALID_ARGS as a placeholder.
> 
Yes, SEC_ERROR_INVALID_ARG is the best we can do at the time. If I don't set the error code Ales told me of seen some odd error code that didn't make any sense. I'll checkin the revised patch with your requests.

Comment 6

[Elio Maldonado]

Pushed: https://hg.mozilla.org/projects/nss/rev/a9c32f21da5c