I was able to repeat the crash with curl:

$ cat key.pem 
-----BEGIN PRIVATE KEY-----
key
-----END PRIVATE KEY-----

$ curl -v --key key.pem https://koji.fedoraproject.org/koji/login --cacert ~/.fedora-upload-ca.cert --cert ~/.fedora.cert 
*   Trying 209.132.181.7...
* Connected to koji.fedoraproject.org (209.132.181.7) port 443 (#0)
* Initializing NSS with certpath: sql:/etc/pki/nssdb
*   CAfile: /home/kdudka/.fedora-upload-ca.cert
  CApath: none
* ALPN/NPN, server did not agree to a protocol
* SSL connection using TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
* Server certificate:
*       subject: E=buildsys,CN=koji.fedoraproject.org,OU=Fedora Builders,O=Fedora Project,ST=North Carolina,C=US
*       start date: Apr 15 18:26:14 2014 GMT
*       expire date: Apr 12 18:26:14 2024 GMT
*       common name: koji.fedoraproject.org
*       issuer: E=admin,CN=Fedora Project CA,OU=Fedora Project CA,O=Fedora Project,L=Raleigh,ST=North Carolina,C=US
> GET /koji/login HTTP/1.1
> Host: koji.fedoraproject.org
> User-Agent: curl/7.46.0
> Accept: */*
> 
* NSS: client certificate from file
*       subject: E=kdudka,CN=kdudka,OU=Fedora User Cert,O=Fedora Project,ST=North Carolina,C=US
*       start date: Nov 18 20:45:26 2015 GMT
*       expire date: May 16 20:45:26 2016 GMT
*       common name: kdudka
*       issuer: E=admin,CN=Fedora Project CA,OU=Fedora Project CA,O=Fedora Project,L=Raleigh,ST=North Carolina,C=US
zsh: segmentation fault (core dumped)  curl -v --key key.pem https://koji.fedoraproject.org/koji/login --cacert   

It crashes during a sing operation:

#0  PK11_GetPrivateModulusLen (key=<optimized out>) at pk11akey.c:814
#1  PK11_SignatureLen (key=key@entry=0x5555559339b0) at pk11obj.c:531
#2  SGN_Digest (privKey=0x5555559339b0, ...) at secsign.c:408
#3  ssl3_SignHashes (hash=0x7fffffffcd60, ...) at ssl3con.c:1020
#4  ssl3_SendCertificateVerify (ss=0x55555590fe80) at ssl3con.c:6355

However, the cause of the crash is not obvious from the backtrace because it happens much sooner -- the code of pem_getPrivateKey() silently ignores the failure of SEC_QuickDERDecodeItem():

209│     /* decode the private key and any algorithm parameters */
210├>    rv = SEC_QuickDERDecodeItem(arena, lpk, pem_RSAPrivateKeyTemplate,
211│                                 keysrc);
212│
213│     if (rv != SECSuccess) {
214│         goto done;
215│     }
216│
217│ done:
218│     return lpk;
219│ }

This bug seems to be introduced in commit 06c610cc:

https://git.fedorahosted.org/cgit/nss-pem.git/commit/?id=06c610cc

What should pem_getPrivateKey() do in this case at line 214?  Set pError to some code, free any allocated memory, and return NULL?

nice! glad there will be some fix; nice work!

cheers,
tehmasp

I think this would have blown up anyway. Even with the old code I don't think everything would have been allocated properly.

If this is solved the the key will just not work, they will get a MAC error as the server private key is not available. Not sure if that's enough for a user to look at the contents of the key file or not.

What needs to happen is in GetAttribute and GetAttributeSize the call to pem_FetchAttribute needs to include an error pointer so we can return an error if the key isn't loaded. This will short-circuit things in the upper PKCS#11 layers and is likely the most compliant. I think this means that pem_PopulateModulusExponent will need to return a CK_RV value and have that bubble up to get reasonable error messages returned. Or we could shortcut it and always return a fixed error to avoid a humongous, invasive change.

This is what the end of a request looks like when I configured mod_nss to use a pemnss token:

pem_mdToken_OpenSession
pem_CreateSession returning new session
Failed to decode, assuming raw RSA key
SEC_QuickDERDecodeItem failed
pem_getPrivateKey returned NULL
pem_mdToken_OpenSession
pem_CreateSession returning new session
pem_mdToken_OpenSession
pem_CreateSession returning new session
Failed to decode, assuming raw RSA key
SEC_QuickDERDecodeItem failed
pem_getPrivateKey returned NULL
pem_Finalize

Created attachment 1117015 [details]
Patch to catch failed ASN1 decoding of RSA keys

Comment on attachment 1117015 [details]
Patch to catch failed ASN1 decoding of RSA keys

nack https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/security/nss/lib/ckfw/pem/pfind.c_sec1
 
    CK_RV * pError = CKR_OK;

    b = pem_FetchAttribute(o, a->type, pError);

should be

    CK_RV pError = CKR_OK;

    b = pem_FetchAttribute(o, a->type, &pError);
    /* do something here with pError? */

https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/security/nss/lib/ckfw/pem/pobject.c_sec6

Is it ok to overwrite pError if ((const NSSItem *) NULL != b) ?

https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/security/nss/lib/ckfw/pem/prsa.c_sec1

this shouldn't be different?

https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/security/nss/lib/ckfw/pem/prsa.c_sec2

this shouldn't be different?

https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/security/nss/lib/ckfw/pem/prsa.c_sec3

second diff - this shouldn't be different?

https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/security/nss/lib/ckfw/pem/prsa.c_sec4

this shouldn't be different?

https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/security/nss/lib/ckfw/pem/prsa.c_sec5

    const NSSItem *classItem = pem_FetchAttribute(io, CKA_CLASS, &pError);
    const NSSItem *keyType = pem_FetchAttribute(io, CKA_KEY_TYPE, &pError);

The second call will overwrite pError returned from the first call.

https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/security/nss/lib/ckfw/pem/prsa.c_sec6

        return CKR_KEY_TYPE_INCONSISTENT; // return pError instead?

or how about

        return (pError ? pError : CKR_KEY_TYPE_INCONSISTENT);

https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/security/nss/lib/ckfw/pem/prsa.c_sec8

    const NSSItem *classItem = pem_FetchAttribute(iKey, CKA_CLASS, pError);
    const NSSItem *keyType = pem_FetchAttribute(iKey, CKA_KEY_TYPE, pError);

The second call will overwrite pError returned from the first call.

https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/security/nss/lib/ckfw/pem/prsa.c_sec9

        *pError = (*pError ? *pError : CKR_KEY_TYPE_INCONSISTENT);

https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/security/nss/lib/ckfw/pem/prsa.c_sec11

this shouldn't be different?

(In reply to Rich Megginson from comment #7)
> Comment on attachment 1117015 [details]
> Patch to catch failed ASN1 decoding of RSA keys
> 
> nack
> https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> security/nss/lib/ckfw/pem/pfind.c_sec1
>  
>     CK_RV * pError = CKR_OK;
> 
>     b = pem_FetchAttribute(o, a->type, pError);
> 
> should be
> 
>     CK_RV pError = CKR_OK;
> 
>     b = pem_FetchAttribute(o, a->type, &pError);
>     /* do something here with pError? */

Sure.

> 
> https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> security/nss/lib/ckfw/pem/pobject.c_sec6
> 
> Is it ok to overwrite pError if ((const NSSItem *) NULL != b) ?

Isn't that the idea? Are you suggested it check for *pError != CKR_OK at some point and return earlier?
 
> https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> security/nss/lib/ckfw/pem/prsa.c_sec1
> 
> this shouldn't be different?
> 
> https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> security/nss/lib/ckfw/pem/prsa.c_sec2
> 
> this shouldn't be different?
> 
> https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> security/nss/lib/ckfw/pem/prsa.c_sec3
> 
> second diff - this shouldn't be different?
> 
> https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> security/nss/lib/ckfw/pem/prsa.c_sec4
> 
> this shouldn't be different?

These are all trailing white-space fixes my vimrc picked up automatically.

> 
> https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> security/nss/lib/ckfw/pem/prsa.c_sec5
> 
>     const NSSItem *classItem = pem_FetchAttribute(io, CKA_CLASS, &pError);
>     const NSSItem *keyType = pem_FetchAttribute(io, CKA_KEY_TYPE, &pError);
> 
> The second call will overwrite pError returned from the first call.

I can add a check to see if it fails I suppose. I was trying to keep the patch small and change the least behavior.
> 
> https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> security/nss/lib/ckfw/pem/prsa.c_sec6
> 
>         return CKR_KEY_TYPE_INCONSISTENT; // return pError instead?
> 
> or how about
> 
>         return (pError ? pError : CKR_KEY_TYPE_INCONSISTENT);

Yes, I think you're right.
 
> https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> security/nss/lib/ckfw/pem/prsa.c_sec8
> 
>     const NSSItem *classItem = pem_FetchAttribute(iKey, CKA_CLASS, pError);
>     const NSSItem *keyType = pem_FetchAttribute(iKey, CKA_KEY_TYPE, pError);
> 
> The second call will overwrite pError returned from the first call.

Yeah, similar to other change earlier, trying to keep diffs small and least invasive as possible. I can add conditionals around these, it's the right thing to do.
 
> https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> security/nss/lib/ckfw/pem/prsa.c_sec9
> 
>         *pError = (*pError ? *pError : CKR_KEY_TYPE_INCONSISTENT);

Yup.
 
> https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> security/nss/lib/ckfw/pem/prsa.c_sec11
> 
> this shouldn't be different?

The final white-space change.

(In reply to Rob Crittenden from comment #8)
> (In reply to Rich Megginson from comment #7)
> > Comment on attachment 1117015 [details]
> > Patch to catch failed ASN1 decoding of RSA keys
> > 
> > nack
> > https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> > security/nss/lib/ckfw/pem/pfind.c_sec1
> >  
> >     CK_RV * pError = CKR_OK;
> > 
> >     b = pem_FetchAttribute(o, a->type, pError);
> > 
> > should be
> > 
> >     CK_RV pError = CKR_OK;
> > 
> >     b = pem_FetchAttribute(o, a->type, &pError);
> >     /* do something here with pError? */
> 
> Sure.
> 
> > 
> > https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> > security/nss/lib/ckfw/pem/pobject.c_sec6
> > 
> > Is it ok to overwrite pError if ((const NSSItem *) NULL != b) ?
> 
> Isn't that the idea? Are you suggested it check for *pError != CKR_OK at
> some point and return earlier?

No.  What I mean is this:

    mdItem.item = (NSSItem *) pem_FetchAttribute(io, attribute, pError);
If there is an error here, pError will have been set, then, if mdItem.item is NULL, pError will be overwritten and the original value will have been lost here:
    if ((NSSItem *) NULL == mdItem.item) {
        *pError = CKR_ATTRIBUTE_TYPE_INVALID;

maybe use *pError = (*pError ? *pError : CKR_ATTRIBUTE_TYPE_INVALID);


>  
> > https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> > security/nss/lib/ckfw/pem/prsa.c_sec1
> > 
> > this shouldn't be different?
> > 
> > https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> > security/nss/lib/ckfw/pem/prsa.c_sec2
> > 
> > this shouldn't be different?
> > 
> > https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> > security/nss/lib/ckfw/pem/prsa.c_sec3
> > 
> > second diff - this shouldn't be different?
> > 
> > https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> > security/nss/lib/ckfw/pem/prsa.c_sec4
> > 
> > this shouldn't be different?
> 
> These are all trailing white-space fixes my vimrc picked up automatically.
> 
> > 
> > https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> > security/nss/lib/ckfw/pem/prsa.c_sec5
> > 
> >     const NSSItem *classItem = pem_FetchAttribute(io, CKA_CLASS, &pError);
> >     const NSSItem *keyType = pem_FetchAttribute(io, CKA_KEY_TYPE, &pError);
> > 
> > The second call will overwrite pError returned from the first call.
> 
> I can add a check to see if it fails I suppose. I was trying to keep the
> patch small and change the least behavior.

Understood.  But the intent of the patch, in addition to fixing the crash, is to make sure the low level error is returned to the higher level.  If there is a problem with the classItem, the caller will have no way to know that it is the classItem that is the problem.

> > 
> > https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> > security/nss/lib/ckfw/pem/prsa.c_sec6
> > 
> >         return CKR_KEY_TYPE_INCONSISTENT; // return pError instead?
> > 
> > or how about
> > 
> >         return (pError ? pError : CKR_KEY_TYPE_INCONSISTENT);
> 
> Yes, I think you're right.
>  
> > https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> > security/nss/lib/ckfw/pem/prsa.c_sec8
> > 
> >     const NSSItem *classItem = pem_FetchAttribute(iKey, CKA_CLASS, pError);
> >     const NSSItem *keyType = pem_FetchAttribute(iKey, CKA_KEY_TYPE, pError);
> > 
> > The second call will overwrite pError returned from the first call.
> 
> Yeah, similar to other change earlier, trying to keep diffs small and least
> invasive as possible. I can add conditionals around these, it's the right
> thing to do.

See above.

>  
> > https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> > security/nss/lib/ckfw/pem/prsa.c_sec9
> > 
> >         *pError = (*pError ? *pError : CKR_KEY_TYPE_INCONSISTENT);
> 
> Yup.
>  
> > https://bugzilla.redhat.com/attachment.cgi?id=1117015&action=diff#a/mozilla/
> > security/nss/lib/ckfw/pem/prsa.c_sec11
> > 
> > this shouldn't be different?
> 
> The final white-space change.

Created attachment 1117288 [details]
Patch to catch failed ASN1 decoding of RSA keys

Hold off on this for a bit, I want to ensure that encrypted keys still work. I'm having trouble getting them to work and I'm not sure if it's me or the code.

Ok, the problem had to do with how I was loading the pem module. I added it manually using:

$ modutil -dbdir . -add pem -libfile /usr/lib64/libnsspem.so -string "`pwd`/server.pem;`pwd`//server.key `pwd`/ca.pem" 

This method doesn't support encrypted keys (I can provide a patch if interested). Once I fixed that it worked fine.

I would appreciate additional testing using other methods. I also tested curl briefly (so I was in fact using the pem module for both client and server).

Elio, are you ok with this patch?

I would prefer to commit white-space fixes and functional fixes separated from each other.  Also the "p" prefix in pError means "a pointer", doesn't it?  If you define a non-pointer variable, the "p" prefix is unnecessarily confusing.

I will give it a try and, if it works properly, I will fix the minor issues.

I'm okay withe the patch in general I don't think it will cause regressions elsewhere. I ado gree with Kamil's comments regarding the "p" prefix. 

I see lines like 
diff --git a/mozilla/security/nss/lib/ckfw/pem/ckpem.h b/mozilla/security/nss/lib/ckfw/pem/ckpem.h 
....
and that worries me because in the interim PEM upstream git repository I got rid of the "mozilla/security" part years ago to match the shallower tree we have in NSS upstream and when NSS upstream switched from cvs to mercurial. Please regenerate the patch using the temporary upstream instead.

It works fairly well.  I only fixed a memory leak on top of that.  All three patches are now available in a temporary upstream branch:

https://git.fedorahosted.org/cgit/nss-pem.git/commit/?h=kdudka&id=98490091
https://git.fedorahosted.org/cgit/nss-pem.git/commit/?h=kdudka&id=2bea33eb
https://git.fedorahosted.org/cgit/nss-pem.git/commit/?h=kdudka&id=dc6cfc85

Unless anybody objects, I would merge them into master...

The short circuit returns upon error in https://git.fedorahosted.org/cgit/nss-pem.git/commit/?h=kdudka&id=2bea33eb in nss/lib/ckfw/pem/pobject.c

Are you sure that they do not introduce any memory leaks?

I personally do not like the interface of pem_Fetch{,PrivKey}Attribute(), which can return non-null value with *pError != CKR_OK and vice versa.  It is neither intuitive, nor documented.  But no, I can see no obvious memory leak introduced by the patchset.  Could you please be more specific?

Operationally I don't see a leak when starting with a bad key in Apache and making a number of requests against the server but it certainly is not exercising all attribute requests.

(In reply to Kamil Dudka from comment #17)
> I personally do not like the interface of pem_Fetch{,PrivKey}Attribute(),
> which can return non-null value with *pError != CKR_OK and vice versa.  It
> is neither intuitive, nor documented.  But no, I can see no obvious memory
> leak introduced by the patchset.  Could you please be more specific?

In many cases adding short circuit returns will introduce memory leaks if memory is allocated before the return.  However, in this case, looking at the code where the short circuit returns were introduced, pem_FetchPrivKeyAttribute allocates nothing, and no other resources are referenced which need to be dereferenced, so in this case the short circuit returns are ok.

(In reply to Kamil Dudka from comment #15)
> It works fairly well.  I only fixed a memory leak on top of that.  All three
> patches are now available in a temporary upstream branch:
> 
> https://git.fedorahosted.org/cgit/nss-pem.git/commit/?h=kdudka&id=98490091
> https://git.fedorahosted.org/cgit/nss-pem.git/commit/?h=kdudka&id=2bea33eb
> https://git.fedorahosted.org/cgit/nss-pem.git/commit/?h=kdudka&id=dc6cfc85
> 
> Unless anybody objects, I would merge them into master...

I have merged them and pushed to master (while keeping the above hashes).

This bug appears to have been reported against 'rawhide' during the Fedora 24 development cycle.
Changing version to '24'.

More information and reason for this action is here:
https://fedoraproject.org/wiki/Fedora_Program_Management/HouseKeeping/Fedora24#Rawhide_Rebase

Elio, could you please include the up2date tarball of nss-pem in the next update of nss in Fedora?

Done.

nss-3.23.0-1.1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-71d6f90d9b

nss-3.23.0-1.2.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-71d6f90d9b

cool; nice work; removing myself from future emails on this issue.
cheers,
tehmasp

nss-3.23.0-1.2.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.