Bug 623663

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.

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.

Rob, is the behavior intended?  Should it be fixed as a documentation bug?

Elio, do we have any regular expression that says what is a valid certificate nickname and what is not?

(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.

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.

(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?

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.

(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?

Of course, we can come up with some more challenging solution in Fedora/upstream and test how it feels...

(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.

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

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?

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).

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?

Yes, that is correct. I suspect that a key only makes sense with a cert for all of the SSL providers.

(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?

No, the key argument would be ignored.

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?

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.

Thanks for review, pushed upstream:

https://github.com/bagder/curl/commit/d8f6d1c

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.

    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.

    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.

    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.

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