Bug 1300652 - [PEM] insufficient input validity checking while loading a private key
[PEM] insufficient input validity checking while loading a private key
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: nss (Show other bugs)
24
All Linux
medium Severity high
: ---
: ---
Assigned To: Elio Maldonado Batiz
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-21 06:12 EST by Kamil Dudka
Modified: 2016-03-26 14:17 EDT (History)
6 users (show)

See Also:
Fixed In Version: nss-3.23.0-1.2.fc24
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: 512019
Environment:
Last Closed: 2016-03-26 14:17:46 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Patch to catch failed ASN1 decoding of RSA keys (13.17 KB, patch)
2016-01-21 17:32 EST, Rob Crittenden
no flags Details | Diff
Patch to catch failed ASN1 decoding of RSA keys (13.81 KB, patch)
2016-01-22 11:20 EST, Rob Crittenden
rmeggins: review+
Details | Diff

  None (edit)
Description Kamil Dudka 2016-01-21 06:12:19 EST
+++ This bug was initially created as a clone of Bug #512019 +++

--- Additional comment from  on 2016-01-19 22:36:15 CET ---

This is still an issue. Just had libnss3.so segfault on CentOS 7.1 system after slapd attempted to do a TLS cert exchange with a key file as follows:

  -----BEGIN PRIVATE KEY-----
  key
  -----END PRIVATE KEY-----


system/issue details:

CentOS Linux release 7.2.1511 (Core)

nss-3.19.1-19.el7_2.x86_64
openldap-2.4.40-8.el7.x86_64
openldap-clients-2.4.40-8.el7.x86_64
openldap-servers-2.4.40-8.el7.x86_64

kernel: slapd[2648]: segfault at 0 ip 00007f0af036c25c sp 00007f0ac0ea6610 error 4 in libnss3.so[7f0af0340000+11e000]

--- Additional comment from  on 2016-01-19 22:38:25 CET ---

CentOS 7.2 to be correct; typo on my first sentence.

Once I repaired the key file with a valid key - slapd TLS works like normal BTW. No more issues but definitely a reproducible segfault w/ details above.

Thanks.
Tehmasp Chaudhri
tehmasp@gmail.com

--- Additional comment from Kamil Dudka on 2016-01-20 13:41:44 CET ---

I am not able to repeat the crash.  Does curl crash if you pass such a key file to the --key option (together with --cert and an https:// URL)?

--- Additional comment from  on 2016-01-20 16:55:44 CET ---

This isn't reproducible via curl. I verified just now and curl does not crash. But specifically when slapd (openldap) is running on a server and using the bad key file then something in the code path causes a segfault in libnss3.so.

There's some edge case in libnss (in conjunction w/ how slapd w/ TLS) causes a segfault.

My setup is an openldap provider server (w/ bad slapd.key) and an openldap consumer server. When the consumer connects to the provider server which is using the key example above it crashes the openldap provider server.

Here are the journal provider slapd.service logs: 

Jan 19 21:01:20 gcestg-openldap-provider-use1c-01 systemd[1]: Started OpenLDAP Server Daemon.
Jan 19 21:01:27 gcestg-openldap-provider-use1c-01 slapd[867]: conn=1000 fd=28 ACCEPT from IP=10.240.0.19:43420 (IP=0.0.0.0:10389)
Jan 19 21:01:27 gcestg-openldap-provider-use1c-01 slapd[867]: conn=1000 op=0 EXT oid=1.3.6.1.4.1.1466.20037
Jan 19 21:01:27 gcestg-openldap-provider-use1c-01 slapd[867]: conn=1000 op=0 STARTTLS
Jan 19 21:01:27 gcestg-openldap-provider-use1c-01 slapd[867]: conn=1000 op=0 RESULT oid= err=0 text=
Jan 19 21:01:29 gcestg-openldap-provider-use1c-01 systemd[1]: slapd.service: main process exited, code=killed, status=11/SEGV
Jan 19 21:01:29 gcestg-openldap-provider-use1c-01 systemd[1]: Unit slapd.service entered failed state.
Jan 19 21:01:29 gcestg-openldap-provider-use1c-01 systemd[1]: slapd.service failed.


///

Thus, we've got 2 issues IMO (and this may not be the best place to capture all of this) but libnss3.so segfaults and an important piece of software in the RedHat ecosystem - ldap - segfaults badly when consumer servers connect to it. Would hate for someone else to hit up against this issue. Let me know how I can help more. If I somehow find the time I might be able to create a multi-VM vagrant env for repro purposes. Again, we're using the latest CentOS 7.2 w/ updates as of yesterday.

Cheers,
Tehmasp

--- Additional comment from Rob Crittenden on 2016-01-20 17:27:34 CET ---

So it fails the first time the key is used? It doesn't fail on server startup? (I'm not familiar with openssl cert/key handling).

A stacktrace would be incredibly helpful. I'm not sure how the value of "key" could get past the base64 decoding.

--- Additional comment from  on 2016-01-20 19:03:04 CET ---

Correct; it fails exactly after the ACCEPT from the consumer server. I tested via telnet connecting to the provider server on port 10389 (albeit w/o TLS) and that ACCEPT was all good. As soon as the consumer connects and after the ACCEPT line in the provider's journal slapd.service log the server crashes. Something w/ decoding the key like you say would be my guess as well.

--- Additional comment from Rob Crittenden on 2016-01-20 19:20:37 CET ---

Any chance on getting a stacktrace of the core?

--- Additional comment from  on 2016-01-20 19:28:32 CET ---

Let me work on this - I need to recreate the environment now as the previous servers are now in active use :)

--- Additional comment from  on 2016-01-20 22:04:45 CET ---

OK - had to learn how to enable coredumps w/ systemd :)

got a couple of coredumps with a bunch of gdb info - particularly:

(gdb) bt
(gdb) bt full
(gdb) info threads
(gdb) thread apply all bt
(gdb) thread apply all bt full


gdb trace w/o debuginfo installed: https://gist.github.com/tehmaspc/1b238c1766549fc5f72f

gdb trace w/ debuginfo installed: https://gist.github.com/tehmaspc/67e7c80efb63e1d6fada


Thanks,
Tehmasp Chaudhri

--- Additional comment from  on 2016-01-20 22:06:33 CET ---

BTW - this new recreated environment (provider/consumer server) was built w/ my same config management (Salt) tools; same everything to allay any concerns w/ these environments. Reproduced the segfault exactly the same way.

--- Additional comment from Rob Crittenden on 2016-01-20 22:47:41 CET ---

That seems to confirm my suspicions. The bad base64 data isn't being detected and when it is eventually pulled apart as an RSA key it blows up spectacularly.

--- Additional comment from  on 2016-01-21 03:56:40 CET ---

@Rob - cool; I'll leave it in your court for now. Let me know if I can help further!

Cheers,
Tehmasp Chaudhri
Comment 1 Kamil Dudka 2016-01-21 07:37:35 EST
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@fedoraproject.org,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@fedoraproject.org,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@redhat.com,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@fedoraproject.org,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│ }
Comment 2 Kamil Dudka 2016-01-21 07:42:32 EST
This bug seems to be introduced in commit 06c610cc:

https://git.fedorahosted.org/cgit/nss-pem.git/commit/?id=06c610cc
Comment 3 Rich Megginson 2016-01-21 09:30:16 EST
What should pem_getPrivateKey() do in this case at line 214?  Set pError to some code, free any allocated memory, and return NULL?
Comment 4 tehmasp 2016-01-21 11:29:06 EST
nice! glad there will be some fix; nice work!

cheers,
tehmasp
Comment 5 Rob Crittenden 2016-01-21 11:44:27 EST
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
Comment 6 Rob Crittenden 2016-01-21 17:32 EST
Created attachment 1117015 [details]
Patch to catch failed ASN1 decoding of RSA keys
Comment 7 Rich Megginson 2016-01-21 18:03:58 EST
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?
Comment 8 Rob Crittenden 2016-01-22 09:33:26 EST
(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.
Comment 9 Rich Megginson 2016-01-22 10:14:45 EST
(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.
Comment 10 Rob Crittenden 2016-01-22 11:20 EST
Created attachment 1117288 [details]
Patch to catch failed ASN1 decoding of RSA keys
Comment 11 Rob Crittenden 2016-01-22 13:16:15 EST
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.
Comment 12 Rob Crittenden 2016-01-22 17:41:38 EST
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?
Comment 13 Kamil Dudka 2016-01-24 11:40:03 EST
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.
Comment 14 Elio Maldonado Batiz 2016-01-24 12:12:54 EST
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.
Comment 15 Kamil Dudka 2016-01-24 17:09:14 EST
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...
Comment 16 Rich Megginson 2016-01-25 10:03:18 EST
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?
Comment 17 Kamil Dudka 2016-01-25 10:21:53 EST
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?
Comment 18 Rob Crittenden 2016-01-25 10:39:50 EST
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.
Comment 19 Rich Megginson 2016-01-25 10:41:13 EST
(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.
Comment 20 Kamil Dudka 2016-02-04 04:16:45 EST
(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).
Comment 21 Jan Kurik 2016-02-24 10:49:10 EST
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
Comment 22 Kamil Dudka 2016-03-08 04:27:13 EST
Elio, could you please include the up2date tarball of nss-pem in the next update of nss in Fedora?
Comment 23 Elio Maldonado Batiz 2016-03-08 17:49:21 EST
Done.
Comment 24 Fedora Update System 2016-03-08 17:55:52 EST
nss-3.23.0-1.1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-71d6f90d9b
Comment 25 Fedora Update System 2016-03-09 20:56:39 EST
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
Comment 26 tehmasp 2016-03-10 15:20:14 EST
cool; nice work; removing myself from future emails on this issue.
cheers,
tehmasp
Comment 27 Fedora Update System 2016-03-26 14:17:42 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.