Bug 623663 - curl returns CURLE_OUT_OF_MEMORY when client certificate is given
curl returns CURLE_OUT_OF_MEMORY when client certificate is given
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: curl (Show other bugs)
6.0
All Linux
high Severity high
: rc
: ---
Assigned To: Kamil Dudka
BaseOS QE Security Team
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-12 09:17 EDT by Karel Srot
Modified: 2013-04-17 06:04 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
libcurl HTTPS connections failed with a CURLE_OUT_OF_MEMORY error when given a certificate file name without a "/". This is now fixed to treat such a string as certificate nickname and if a file with the same name exists and libcurl runs in verbose mode, a warning is issued. The updated documentation now suggests to use the "./" prefix to load a file from the current directory.
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-05-19 09:12:25 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)
curl OUT OF MEMORY reproducer (1.43 KB, text/plain)
2010-08-12 09:17 EDT, Karel Srot
no flags Details
work in progress (5.06 KB, patch)
2010-08-13 04:39 EDT, Kamil Dudka
no flags Details | Diff
proposed fix for el6 (5.94 KB, patch)
2011-01-04 09:04 EST, Kamil Dudka
rcritten: review+
Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2011:0573 normal SHIPPED_LIVE curl bug fix update 2011-05-18 13:57:02 EDT

  None (edit)
Description Karel Srot 2010-08-12 09:17:24 EDT
Created attachment 438435 [details]
curl OUT OF MEMORY reproducer

Description of problem:
curl returns CURLE_OUT_OF_MEMORY when running https request and a client certificate is given.


Version-Release number of selected component (if applicable):
curl-7.19.7-16.el6.x86_64
openssl-1.0.0-4.el6.x86_64
nss-3.12.6-3.el6.x86_64

How reproducible:
always

Steps to Reproduce:
1. execute "bash curl_out_of_memory.sh" 
(in a temporary folder, script creates several files)
  
Actual results:
OUT_OF_MEMORY

Expected results:
content of index.html (probably)

Additional note:
I am not sure whether I use all commands properly ;-)
Comment 1 Kamil Dudka 2010-08-12 17:09:22 EDT
Thanks for filing the bug.  There is a problem in function fmt_nickname():

gdb -q --args curl -v --cert client.crt https://localhost:4433
(gdb) break fmt_nickname
(gdb) run
* About to connect() to localhost port 4433 (#0)
*   Trying ::1... Connection refused
*   Trying 127.0.0.1... connected
* Connected to localhost (127.0.0.1) port 4433 (#0)
* Initializing NSS with certpath: /etc/pki/nssdb
*   CAfile: /etc/pki/tls/certs/ca-bundle.crt
  CApath: none

Breakpoint 1, fmt_nickname (str=0x62a610 "client.crt", nickname_alloc=0x7fffffffc76f) at nss.c:282
282       char *nickname = NULL;

(gdb) list
277       return 0;
278     }
279
280     static char *fmt_nickname(char *str, bool *nickname_alloc)
281     {
282       char *nickname = NULL;
283       *nickname_alloc = FALSE;
284
285       if(is_file(str)) {
286         char *n = strrchr(str, '/');

(gdb) next
283       *nickname_alloc = FALSE;

(gdb)
285       if(is_file(str)) {

(gdb)
286         char *n = strrchr(str, '/');

(gdb)
287         if(n) {

(gdb)
292         return nickname;

(gdb) inf lo
n = 0x0
nickname = 0x0

(gdb) fin
0x00007ffff7bad2be in Curl_nss_connect (conn=0x62abb0, sockindex=0) at nss.c:1299
1299        char *nickname = fmt_nickname(data->set.str[STRING_CERT], &nickname_alloc);
Value returned is $1 = 0x0

(gdb) next
1300        if(!nickname)

(gdb)
1301          return CURLE_OUT_OF_MEMORY;


As a workaround I suggest to proceed with "./client.crt" instead of "client.crt" until the problem is fixed.
Comment 2 Rob Crittenden 2010-08-12 17:12:25 EDT
The reason for this is if you have set the NSS_DIR env variable you can use nicknames so any certificate name passed in that isn't a path is considered a nickname.
Comment 3 Kamil Dudka 2010-08-12 17:14:26 EDT
Rob, is the behavior intended?  Should it be fixed as a documentation bug?
Comment 4 Kamil Dudka 2010-08-12 17:17:21 EDT
Elio, do we have any regular expression that says what is a valid certificate nickname and what is not?
Comment 5 Kamil Dudka 2010-08-12 17:21:45 EDT
(In reply to comment #3)
> Rob, is the behavior intended?  Should it be fixed as a documentation bug?    

I realized this can't be really fixed as a documentation bug  -- CURLE_OUT_OF_MEMORY is inadequate response even in that case.  CURLE_SSL_CERTPROBLEM would make much more sense.
Comment 6 Rob Crittenden 2010-08-12 17:29:33 EDT
I absolutely agree that this is a terrible error message. I'm guessing its caused as a bad side-effect.

An optimization would be to try opening the nickname as a file if the nickname isn't found in the NSS db. Or do a stat() on it.
Comment 7 Kamil Dudka 2010-08-12 17:36:03 EDT
(In reply to comment #6)
> An optimization would be to try opening the nickname as a file if the nickname
> isn't found in the NSS db. Or do a stat() on it.

The stat() already happens:
285       if(is_file(str)) {

I am not sure if it is safe to call PK11_FindCertFromNickname() at that point, it might require some code reordering in order to get it working.

Finally it's about the precedence -- if we have both file and nickname that conform the given string, we should somehow say in advance which one is yields.

Rob, are you sure that the slash is not allowed in the cert nickname?
Comment 8 Rob Crittenden 2010-08-12 18:23:09 EDT
I'm not aware of a character restriction in nicknames and slash is definitely allowed. It was a convention I made.

I think it is safe to call PK11_FindCertFromNickname() here.

In terms of precedence, I'm torn. If you go through the trouble of setting SSL_DIR then you are probably more of a NSS power-user so understand nicknames, etc. So I think that if there are both, use the NSS nickname. But I seriously doubt that will ever come up.
Comment 9 Kamil Dudka 2010-08-12 18:37:28 EDT
(In reply to comment #8)
> In terms of precedence, I'm torn. If you go through the trouble of setting
> SSL_DIR then you are probably more of a NSS power-user so understand nicknames,
> etc. So I think that if there are both, use the NSS nickname. But I seriously
> doubt that will ever come up.    

As for the SSL_DIR setting, that's not anyhow required in order to use client certs from NSS db.  By default, nss is initialized with "sql:/etc/pki/nssdb", and depending on the content of /etc/pki/nssdb/pkcs11.txt it loads either system, or user, or even both NSS databases in th read-only mode.

The bug is open against Enterprise Linux, so I would slightly prefer to keep the accepted string format as is.  What about just replacing CURLE_OUT_OF_MEMORY by CURLE_SSL_CERTPROBLEM in case we have enough memory, and improving the documentation?
Comment 10 Kamil Dudka 2010-08-12 18:41:33 EDT
Of course, we can come up with some more challenging solution in Fedora/upstream and test how it feels...
Comment 11 Elio Maldonado Batiz 2010-08-12 20:52:04 EDT
(In reply to comment #4)
> Elio, do we have any regular expression that says what is a valid certificate
> nickname and what is not?    

I don't know whether there is a restriction on nicknames. None are
documented as far as I know and none appear upon reading the code.

Searching on nickname on the sources taught taught me these bits. 
When you import a certificate, certutil and pk12 and others all 
call PK11_ImportCert(... const char *nickname, ...) 
(in lib/pk11wrap/pk11cert.c)
Which calls nssCryptokiPrivateKey_SetCertificate
(keyobj, NULL, nickname, &c->id, &c->subject);
/* comment says in order to set matching attributes for the key */

and this one calls (softoken)
CKAPI(epv)->C_SetAttributeValue(session->handle,
      keyObject->handle, key_template, key_size);

where the template includes these attributes:
CKA_LABEL  <- nickname
CKA_ID     <- keyid 
CKA_SUBJECT <- subject (from the cert)
the keyid is a SHA1 of a public value {rsa.modulus | dsa.publicValue | ...}
It's up to the implementer to come up with a scheme for making ID's.
PKCS #11 doesn't impose restrictions on CKA_ID but it says:
"The CKA_ID attribute is intended as a means of distinguishing multiple public-
key/private-key pairs held by the same subject (whether stored in the same token or not). (Since the keys are distinguished by subject name as well as identifier, it is possible that keys for different subjects may have the same CKA_ID value without introducing any ambiguity.)"

Could there be 'nickname' choices that would cause C_SetAttributeValue
to fail? Would that be the only way for a nickname to be rejected?
This info probably doesn't help you much. Let's ask Bob.
Comment 12 Bob Relyea 2010-08-12 21:15:23 EDT
PKCS #11 takes UTF8 LABELS. There is not specific restriction, though if you pass something with embedded NULLS, then things are likely to get confused.

bob
Comment 13 Kamil Dudka 2010-08-13 04:39:49 EDT
Created attachment 438624 [details]
work in progress

Thank all of you for looking at the issue.

Rob, I tried to go your way and handle the string as nickname as long as there is no slash inside.  However the code does not seem to be ready for it.  The code of fmt_nickname() is duplicated in nss_load_cert() and it bails out there:

* About to connect() to localhost port 4433 (#0)
*   Trying ::1... Connection refused
*   Trying 127.0.0.1... connected
* Connected to localhost (127.0.0.1) port 4433 (#0)
* Initializing NSS with certpath: /etc/pki/nssdb
*   CAfile: /etc/pki/tls/certs/ca-bundle.crt
  CApath: none
* warning: certificate file name "client.crt" handled as nickname; please use "./" prefix to force file name
* Unable to load certificate client.crt
* Closing connection #0
curl: (58) Unable to load certificate client.crt


Attached is my work in progress.  Is there any valid reason for doing the check twice?
Comment 14 Rob Crittenden 2010-08-13 09:31:28 EDT
I'm not sure why the check is done twice, particularly the code that says the CA can't be referenced by nickname (though it could have to do with the CURL API. I had a hard time mapping some options to NSS).

What I think happened is that the code to use regular nicknames has been deprecate a bit. For example, if you pass in a nickname to nss_load_cert() and have the PEM module available it doesn't look like it will do the right thing. It looks more like if you have the PEM module it wants to use files, otherwise it falls back to nicknames, but it doesn't really support both.

From what I can tell the code at the beginning of nss_load_cert() can be replaced by a call to fmt_nickname().

And some other changes can happen too. If we centralize in one function the decision to use filenames vs NSS nicknames then we should be able to pass that around instead of values from data->set so it doesn't need to get calculated over and over.

Note that there is another rather horrible fixed setting here too. It assumes that cert/key is in slot 1. The PEM module itself puts all CA's into slot 0 so that is ok (the idea is 1 key per slot, many certs per slot).
Comment 15 Kamil Dudka 2010-08-17 09:29:00 EDT
If I understand it correctly, the separate key can be loaded only from file, because keys have no nickname.  Does it mean that libcurl option CURLOPT_SSLKEY takes only file names, and makes sense only if a file is given to CURLOPT_SSLCERT simultaneously?
Comment 16 Rob Crittenden 2010-08-17 09:37:00 EDT
Yes, that is correct. I suspect that a key only makes sense with a cert for all of the SSL providers.
Comment 17 Kamil Dudka 2010-08-17 09:52:21 EDT
(In reply to comment #16)
> I suspect that a key only makes sense with a cert for all of the SSL providers.

Sure, but NSS is the only provider that operates with nicknames.  The current code allows to pass a nickname to CURLOPT_SSLCERT, along with a key file to CURLOPT_SSLKEY.  Is that a valid combination?
Comment 18 Rob Crittenden 2010-08-17 09:59:43 EDT
No, the key argument would be ignored.
Comment 19 Kamil Dudka 2010-08-17 10:19:06 EDT
So here is some summary:

- CURLOPT_CAINFO takes only file names, as CA can't be specified by a nickname

- CURLOPT_CRLFILE takes only file names, as CRL can't be specified by a nickname

- CURLOPT_ISSUERCERT takes only nicknames (intentionally?)

- CURLOPT_SSLCERT takes either nickname, or a file name

- CURLOPT_SSLKEY should take only file name, and only if a file name was also given to CURLOPT_SSLCERT


Is the above correct?
Comment 25 Kamil Dudka 2011-01-04 09:04:49 EST
Created attachment 471666 [details]
proposed fix for el6

Rob, could you please review this one?  It should be safe enough change for el6.  For Fedora I'll come with something more ambitious perhaps.
Comment 26 Kamil Dudka 2011-01-04 11:25:20 EST
Thanks for review, pushed upstream:

https://github.com/bagder/curl/commit/d8f6d1c
Comment 29 Miroslav Vadkerti 2011-02-02 08:52:43 EST
The man documentation for --cacert is incorrect - contains this statement that is not true (option --cacert doesn't work with nickname):

--cacert <CA certificate>
=== snip ===
              If  curl  is built against the NSS SSL library then this option tells curl the nickname of the CA certificate to use within the NSS database defined by the environment vari-able SSL_DIR (or by default /etc/pki/nssdb).  If the NSS PEM PKCS#11 module (libnsspem.so) is available then PEM files may be loaded.
=== snip ===

I don't know if this is worth fixing.
Comment 31 Misha H. Ali 2011-04-19 19:57:46 EDT
    Technical note added. If any revisions are required, please edit the "Technical Notes" field
    accordingly. All revisions will be proofread by the Engineering Content Services team.
    
    New Contents:
libcurl HTTPS connections would fail with CURLE_OUT_OF_MEMORY errors when given a certificate file name that did not include a "/". This is now fixed so that libcurl HTTPS connections work as expected without a "/" in the certificate name.
Comment 32 Misha H. Ali 2011-04-19 21:22:40 EDT
    Technical note updated. If any revisions are required, please edit the "Technical Notes" field
    accordingly. All revisions will be proofread by the Engineering Content Services team.
    
    Diffed Contents:
@@ -1 +1 @@
-libcurl HTTPS connections would fail with CURLE_OUT_OF_MEMORY errors when given a certificate file name that did not include a "/". This is now fixed so that libcurl HTTPS connections work as expected without a "/" in the certificate name.+libcurl HTTPS connections failed with a CURLE_OUT_OF_MEMORY error when given a certificate file name without a "/". This is now fixed so that libcurl HTTPS connections work as expected without a "/" in the certificate name.
Comment 33 Misha H. Ali 2011-04-26 19:42:34 EDT
    Technical note updated. If any revisions are required, please edit the "Technical Notes" field
    accordingly. All revisions will be proofread by the Engineering Content Services team.
    
    Diffed Contents:
@@ -1 +1,2 @@
-libcurl HTTPS connections failed with a CURLE_OUT_OF_MEMORY error when given a certificate file name without a "/". This is now fixed so that libcurl HTTPS connections work as expected without a "/" in the certificate name.+libcurl HTTPS connections failed with a CURLE_OUT_OF_MEMORY error when given a certificate file name without a "/". This is now fixed to treat such a string as certificate nickname and if a file with the same name exists and libcurl runs in verbose mode, a warning is issued. The updated documentation now suggests to use the "./" prefix to load a file from the 
+current directory.
Comment 34 errata-xmlrpc 2011-05-19 09:12:25 EDT
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2011-0573.html

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