Bug 733657 - nss: select client certificates by DER
Summary: nss: select client certificates by DER
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: curl
Version: rawhide
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: ---
Assignee: Kamil Dudka
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 733685 734760 754771 768669
Blocks: 733749 733752 746629
TreeView+ depends on / blocked
 
Reported: 2011-08-26 11:47 UTC by Kamil Dudka
Modified: 2012-10-08 13:13 UTC (History)
6 users (show)

Fixed In Version: curl-7.22.0-2.fc17
Doc Type: Bug Fix
Doc Text:
Clone Of:
: 733685 733749 733752 746629 (view as bug list)
Environment:
Last Closed: 2012-10-08 13:13:01 UTC


Attachments (Terms of Use)
first draft of the patch (15.35 KB, patch)
2011-08-26 14:01 UTC, Kamil Dudka
no flags Details | Diff
[PATCH 1/4] nss: select client certificates by DER (4.26 KB, patch)
2011-08-31 11:32 UTC, Kamil Dudka
emaldona: review+
Details | Diff
[PATCH 2/4] nss: refactor fmt_nickname() -> dup_nickname() (3.41 KB, patch)
2011-08-31 11:33 UTC, Kamil Dudka
emaldona: review+
Details | Diff
[PATCH 3/3] nss: big cleanup in nss_load_cert() and cert_stuff() (8.92 KB, patch)
2011-08-31 11:33 UTC, Kamil Dudka
no flags Details | Diff
[PATCH 3/4] nss: big cleanup in nss_load_cert() and cert_stuff() (8.91 KB, patch)
2011-09-06 16:28 UTC, Kamil Dudka
emaldona: review+
Details | Diff
[PATCH 4/4] nss: avoid a SIGSEGV with immature version of NSS (1.51 KB, patch)
2011-09-06 16:29 UTC, Kamil Dudka
emaldona: review+
Details | Diff
two connections in a row using a client certificate (467 bytes, text/plain)
2012-09-08 10:45 UTC, Kamil Dudka
no flags Details

Description Kamil Dudka 2011-08-26 11:47:12 UTC
Description of problem:
File base names are not globally unique.  Their collisions cause us problems.


Version-Release number of selected component (if applicable):
curl-7.21.7-4.fc17


Steps to Reproduce:
1. load two client certificates from file with the same file base names


Actual results:
nickname collision


Additional info:
The same problem occurs when the contents of a file is changed meanwhile.

Comment 1 Kamil Dudka 2011-08-26 14:01:36 UTC
Created attachment 520096 [details]
first draft of the patch

Comment 2 Kamil Dudka 2011-08-31 11:30:37 UTC
Comment on attachment 520096 [details]
first draft of the patch

I did only some minor changes in order to return more precise error codes and wrote a workaround for bug #733657 -- Elio, could you please review these patches?

Comment 3 Kamil Dudka 2011-08-31 11:32:11 UTC
Created attachment 520798 [details]
[PATCH 1/4] nss: select client certificates by DER

... instead of nicknames, which are not unique.

Comment 4 Kamil Dudka 2011-08-31 11:33:03 UTC
Created attachment 520799 [details]
[PATCH 2/4] nss: refactor fmt_nickname() -> dup_nickname()

Do not use artificial nicknames for certificates from files.

Comment 5 Kamil Dudka 2011-08-31 11:33:32 UTC
Created attachment 520800 [details]
[PATCH 3/3] nss: big cleanup in nss_load_cert() and cert_stuff()

Comment 6 Kamil Dudka 2011-08-31 11:35:42 UTC
(In reply to comment #2)
> I did only some minor changes in order to return more precise error codes and
> wrote a workaround for bug #733657

I meant bug #734760 of course :-)

Comment 7 Kamil Dudka 2011-09-06 16:28:20 UTC
Created attachment 521707 [details]
[PATCH 3/4] nss: big cleanup in nss_load_cert() and cert_stuff()

Updated version of the patch, just two cosmetic changes.

Comment 8 Kamil Dudka 2011-09-06 16:29:46 UTC
Created attachment 521708 [details]
[PATCH 4/4] nss: avoid a SIGSEGV with immature version of NSS

workaround for bug #733685 comment #1

Comment 9 Elio Maldonado Batiz 2011-09-12 17:59:10 UTC
Comment on attachment 521708 [details]
[PATCH 4/4] nss: avoid a SIGSEGV with immature version of NSS

ok until 733685 gets fixed.

Comment 10 Kamil Dudka 2011-09-13 13:49:36 UTC
Thanks for review Elio.  We are now one step forward to get those patches to upstream and Fedora.  The patch 2/4 did not get review+.  Does it mean there is still something to be improved?

Comment 11 Elio Maldonado Batiz 2011-09-13 14:10:35 UTC
(In reply to comment #10) I just forgot click on +, done now.

Comment 12 Kamil Dudka 2011-09-13 14:49:31 UTC
Great!  I will send the patches upstream as soon as curl-7.22.0 is out.  They have a feature freeze right now...

Comment 13 Kamil Dudka 2011-09-14 10:56:52 UTC
patches sent upstream:

http://thread.gmane.org/gmane.comp.web.curl.library/32984

Comment 14 Kamil Dudka 2011-09-19 12:42:47 UTC
fixed in curl-7.22.0-2.fc17

Comment 15 Kamil Dudka 2011-10-17 10:29:00 UTC
pushed upstream:

https://github.com/bagder/curl/compare/d47d95ac3b...491c5a497c

Comment 16 Thomas Vander Stichele 2012-09-08 09:10:16 UTC
This patch breaks curl for me.  I use a client certificate and private key.  

Before this patch, I was able to make multiple requests using pycurl (so, reusing the curl library, instead of using the curl command line tool which can only do single shot requests).

Due to this patch, the second request made (even if it is exactly the same as the first one) fails.


$ python credex.py
* About to connect() to api.test.credex.net.local port 9001 (#0)
*   Trying 127.0.0.1... * connected
* Initializing NSS with certpath: sql:/etc/pki/nssdb
*   CAfile: /home/thomas/credex/git/dev/partner/credexapp/credexapp/ca_bundle.pem
  CApath: none
* NSS: client certificate from file
* 	subject: C=US,ST=New York,L=New Jersey,E=client_app@api.test.credex.net.local,CN=TestClientAppLocal,OU=CredEx,O=http://www.credex.net.local/featcredex,O="Emerging Payment Technologies, Local"
* 	start date: Jan 13 16:58:47 2012 GMT
* 	expire date: Jan 11 16:58:47 2020 GMT
* 	common name: TestClientAppLocal
* 	issuer: C=US,ST=New York,L=New Jersey,E=featcredex@credex.net.local,CN=FeatCredEx Development Gateway CA,OU=CredEx,O=http://www.credex.net.local/featcredex,O="Emerging Payment Technologies, Local"
* SSL connection using TLS_RSA_WITH_AES_256_CBC_SHA
* Server certificate:
* 	subject: C=US,ST=New York,L=New Jersey,E=client_app@api.credex.net.local,CN=api.test.credex.net.local,OU=CredEx,O=http://www.credex.net.local/featcredex,O="Emerging Payment Technologies, Local"
* 	start date: Jan 13 16:58:46 2012 GMT
* 	expire date: Jan 11 16:58:46 2020 GMT
* 	common name: api.test.credex.net.local
* 	issuer: C=US,ST=New York,L=New Jersey,E=featcredex@credex.net.local,CN=FeatCredEx Development Gateway CA,OU=CredEx,O=http://www.credex.net.local/featcredex,O="Emerging Payment Technologies, Local"
> POST /api/1.0/eva/reports HTTP/1.1
User-Agent: PycURL/7.21.3
Host: api.test.credex.net.local:9001
Accept: application/json
Content-Type:application/json
Content-Length: 146

* upload completely sent off: 146out of 146 bytes
< HTTP/1.1 200 OK
< Transfer-Encoding: chunked
< Date: Sat, 08 Sep 2012 09:03:59 GMT
< Connection: close
< Cache-Control: no-cache
< Content-Type: application/json
< Server: TwistedWeb/11.1.0
< 
* Closing connection #0
* About to connect() to api.test.credex.net.local port 9001 (#0)
*   Trying 127.0.0.1... * connected
*   CAfile: /home/thomas/credex/git/dev/partner/credexapp/credexapp/ca_bundle.pem
  CApath: none
* NSS error -5938
* Closing connection #0
* SSL connect error

I narrowed it down to this patch by rebuilding curl from the fedora pkg repository.

commit 95558f1 (which adds this patch) is the first one where it breaks - 7.22.0-2

commit eaba13 still works - 7.22.0-1

This is pretty critical for anyone using curl as a library with client certificates, and I can't see any other workaround right now except for downgrading curl.

Comment 17 Thomas Vander Stichele 2012-09-08 09:52:22 UTC
I ran into this problem on F17, F16 had a version of curl that still worked.

The current package for F17 (7.24.0-5) gives an explanation for the NSS error:


* About to connect() to api.test.credex.net.local port 9001 (#0)
*   Trying 127.0.0.1...
* connected
* Connected to api.test.credex.net.local (127.0.0.1) port 9001 (#0)
*   CAfile: /home/thomas/credex/git/dev/partner/credexapp/credexapp/ca_bundle.pem
  CApath: none
* NSS error -5938 (PR_END_OF_FILE_ERROR)
* Encountered end of file
* Closing connection #0

From looking at the patch and comparing the output between successful and failed attempts, I'd say the changes in

static SECStatus SelectClientCert(void *arg, PRFileDesc *s
ock,
                                   struct CERTCertificateStr **pRetCert,
                                   struct SECKEYPrivateKeyStr **pRetKey)


have affected handling of client certificates.

Comment 18 Kamil Dudka 2012-09-08 10:45:49 UTC
Created attachment 610977 [details]
two connections in a row using a client certificate

Thanks for feedback!  Could you please attach a pycurl-based minimal example that repeats the problem for you?  You can use the attached one, which works for me, as a template...

Comment 19 Kamil Dudka 2012-09-17 11:21:26 UTC
Thomas, could you please provide additional info on how to trigger the bug?

I'd be happy to write a fix for this, but I need to know what the problem is...

Comment 20 Kamil Dudka 2012-10-08 13:13:01 UTC
The issue described in comment #0 is believed to be fixed.  Please open a separate bug for the problem you are facing with some steps to reproduce.  Thanks in advance!


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