Bug 642433 - libpnsspem aborts if cacert dir contains other directories
Summary: libpnsspem aborts if cacert dir contains other directories
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: nss
Version: 14
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Elio Maldonado Batiz
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 653619 (view as bug list)
Depends On:
Blocks: 643564
TreeView+ depends on / blocked
 
Reported: 2010-10-12 21:41 UTC by Simo Sorce
Modified: 2011-12-21 18:50 UTC (History)
9 users (show)

Fixed In Version: 3.12.9-1.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
: 643564 (view as bug list)
Environment:
Last Closed: 2011-12-21 18:50:38 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
FileToItem checks for and rejects directories (916 bytes, patch)
2010-12-01 23:27 UTC, Elio Maldonado Batiz
no flags Details | Diff
test application to reproduce the bug and verify fix (1.76 KB, text/plain)
2010-12-03 22:32 UTC, Elio Maldonado Batiz
no flags Details
Preliminary patch that gets rid of the crash (2.43 KB, patch)
2010-12-03 22:41 UTC, Elio Maldonado Batiz
no flags Details | Diff
Patch version 2 (1.47 KB, patch)
2010-12-07 22:54 UTC, Elio Maldonado Batiz
no flags Details | Diff
Patch V3 with changes suggested by Kamil and Rob (1.41 KB, patch)
2010-12-10 16:13 UTC, Elio Maldonado Batiz
no flags Details | Diff

Description Simo Sorce 2010-10-12 21:41:35 UTC
Description of problem:
try any openldap command and use the LDAPTLS_CACERTDIR to pass in an arbitrary directory containing other directories.

It will abort as soon as openldap passes down a directory as a file.

It should probably, at most, return an error, if not ignore the entry completely.

This is the backtrace:

(gdb) bt
#0  0x00132416 in __kernel_vsyscall ()
#1  0x00412501 in raise () from /lib/libc.so.6
#2  0x00413f6e in abort () from /lib/libc.so.6
#3  0x0044fa8d in __libc_message () from /lib/libc.so.6
#4  0x00457df4 in _int_free () from /lib/libc.so.6
#5  0x003bc118 in PR_Free (ptr=0x80ad3d0) at ../../../mozilla/nsprpub/pr/src/malloc/prmem.c:490
#6  0x0071ee92 in PORT_Free_Util (ptr=0x80ad3d0) at secport.c:151
#7  0x0071dfba in SECITEM_FreeItem_Util (zap=0xbfffe7c4, freeit=0) at secitem.c:264
#8  0x007062d0 in FileToItem (dst=0xbfffe7c4, src=0x80acd58) at util.c:129
#9  0x00706338 in ReadDERFromFile (derlist=0xbfffe868, filename=0x80ad1a0 "/etc/ipa/html", ascii=1, cipher=0xbfffe864, ivstring=0xbfffe860, certsonly=1)
    at util.c:155
#10 0x007044e9 in pem_CreateObject (fwInstance=0x808c7e0, fwSession=0x808eca0, mdToken=0x808c938, pTemplate=0xbfffee74, ulAttributeCount=4, pError=0xbfffed5c)
    at pobject.c:1118
#11 0x0070534f in pem_mdSession_CreateObject (mdSession=0x808ed10, fwSession=0x808eca0, mdToken=0x808c938, fwToken=0x808e0f0, mdInstance=0x731ee0, 
    fwInstance=0x808c7e0, arena=0x808e050, pTemplate=0xbfffee74, ulAttributeCount=4, pError=0xbfffed5c) at psession.c:157
#12 0x00709136 in nssCKFWSession_CreateObject (fwSession=0x808eca0, pTemplate=0xbfffee74, ulAttributeCount=4, pError=0xbfffed5c) at session.c:1353
#13 0x0070e769 in NSSCKFWC_CreateObject (fwInstance=0x808c7e0, hSession=1, pTemplate=0xbfffee74, ulCount=4, phObject=0xbfffee1c) at wrap.c:1994
#14 0x00700aed in pemC_CreateObject (hSession=1, pTemplate=0xbfffee74, ulCount=4, phObject=0xbfffee1c) at ../../../../../dist/public/nss/nssck.api:566
#15 0x0028f092 in PK11_CreateNewObject (slot=0x808dd48, session=1, theTemplate=0xbfffee74, count=4, token=0, objectID=0xbfffee1c) at pk11obj.c:412
#16 0x002905e1 in PK11_CreateGenericObject (slot=0x808dd48, pTemplate=0xbfffee74, count=4, token=0) at pk11obj.c:1347
#17 0x001691bc in tlsm_add_cert_from_file (ctx=0x8062178, filename=0x808bff8 "/etc/ipa/html", isca=1) at ../../../libraries/libldap/tls_m.c:1062
#18 0x0016ad03 in tlsm_init_ca_certs (arg=0x8062178) at ../../../libraries/libldap/tls_m.c:1188
#19 tlsm_deferred_init (arg=0x8062178) at ../../../libraries/libldap/tls_m.c:1331
#20 tlsm_deferred_ctx_init (arg=0x8062178) at ../../../libraries/libldap/tls_m.c:1684
#21 0x003c1909 in PR_CallOnceWithArg (once=0x806219c, func=0x16a360 <tlsm_deferred_ctx_init>, arg=0x8062178) at ../../../mozilla/nsprpub/pr/src/misc/prinit.c:836
#22 0x00168c4d in tlsm_session_new (ctx=0x8062178, is_server=0) at ../../../libraries/libldap/tls_m.c:1991
#23 0x00165a0c in alloc_handle (ctx_arg=<value optimized out>, is_server=<value optimized out>) at ../../../libraries/libldap/tls2.c:296
#24 0x00165b9c in ldap_int_tls_connect (ld=0x8058078, conn=<value optimized out>) at ../../../libraries/libldap/tls2.c:341
#25 0x00166727 in ldap_int_tls_start (ld=0x8058078, conn=0x8058228, srv=0x0) at ../../../libraries/libldap/tls2.c:833
#26 0x00166b3c in ldap_start_tls_s (ld=0x8058078, serverctrls=0x0, clientctrls=0x0) at ../../../libraries/libldap/tls2.c:939
#27 0x0804bc2b in tool_conn_setup (dont=0, private_setup=0) at ../../../clients/tools/common.c:1290
#28 0x0804aafc in main (argc=5, argv=0xbffff6f4) at ../../../clients/tools/ldappasswd.c:248

Comment 1 Rob Crittenden 2010-10-12 22:06:02 UTC
In FileToItem() we call PR_GetOpenFileInfo() so I suppose we can just make sure that the type is a PR_FILE_FILE otherwise return a failure.

Comment 2 Howard Chu 2010-11-15 20:29:11 UTC
Does PR_FILE_FILE preclude other types of nodes, e.g. fifos, named pipes? I know of an installation that exposes its cert/key thru a named fifo managed by another auth server...

Comment 3 Rich Megginson 2010-11-15 20:33:53 UTC
*** Bug 653619 has been marked as a duplicate of this bug. ***

Comment 4 Rich Megginson 2010-11-15 20:37:22 UTC
I have proposed a patch to openldap upstream to mitigate this problem: http://www.openldap.org/its/index.cgi/Contrib?id=6703

Comment 5 Jan Vcelak 2010-11-18 13:02:36 UTC
Please, try openldap-2.4.23-3.fc14 from updates-testing. Rich's patch is included.

Comment 6 Elio Maldonado Batiz 2010-11-26 19:56:03 UTC
(In reply to comment #2) PR_FILE_FILE does precude other types such as fifos, it's only regular file. The mapping is done by _MD_convert_stat_to_fileinfo at http://mxr.mozilla.org/nspr/source/nsprpub/pr/src/md/unix/unix.c#2565

Comment 7 Elio Maldonado Batiz 2010-11-29 19:55:38 UTC
(1) The original suggestion which would be patch like this
     prStatus = PR_GetOpenFileInfo(src, &info);
 
-    if (prStatus != PR_SUCCESS) {
+    if (prStatus != PR_SUCCESS || info.type == PR_FILE_FILE) {
 	return SECFailure;
     }
has the problem that it precludes FIFOS

(2) Another option is to just reject directories
     prStatus = PR_GetOpenFileInfo(src, &info);
 
-    if (prStatus != PR_SUCCESS) {
+    if (prStatus != PR_SUCCESS || info.type == PR_FILE_DIRECTORY) {
 	return SECFailure;
     }
It is a bit vague, we accepting PR_FILE_OTHER which together with PR_FILE_FILE gives FIFOS which we like, but we aren't checking for other undesirable files types. We would place the burden on client code to do proper checks and if they don't one wouldn't know until later when the error diagnostic may not be very informative.

(3) The only way for a fine-grained check would require bypassing NSPR in favor of direct file calls which is not acceptable for an a module we plan to make part of NSS proper.

Would option 2 be acceptable?

Comment 8 Rich Megginson 2010-11-29 20:11:35 UTC
What does openssl do?  Does openssl allow non-files like named pipes?  Since this feature is meant to be used to replace openssl with moznss, I would suggest at least using the current openssl behavior as a starting point.

Comment 9 Elio Maldonado Batiz 2010-12-01 20:01:06 UTC
(In reply to comment #8)
> What does openssl do?  Does openssl allow non-files like named pipes?  
openssl does not seem to support named pipes. This is judging from a simple test I performed where I created a fifo wrote to it and read with openssl cli tool.
on terminal one: mkfifo my_pipe; cat cert.pem > my_pipe
on terminal two: openssl x509 -in my_pipe -noout
Nothing was displayed.

Comment 10 Howard Chu 2010-12-01 20:12:59 UTC
Yo(In reply to comment #9)
> (In reply to comment #8)
> > What does openssl do?  Does openssl allow non-files like named pipes?  
> openssl does not seem to support named pipes. This is judging from a simple
> test I performed where I created a fifo wrote to it and read with openssl cli
> tool.
> on terminal one: mkfifo my_pipe; cat cert.pem > my_pipe
> on terminal two: openssl x509 -in my_pipe -noout
> Nothing was displayed.

Your test was invalid since fifos only retain their data while there is a descriptor open on them. This will work:

cd /tmp; mknod fifo p; cat >> fifo

In separate window:
cp cacert.pem /tmp/fifo

In separate window:
openssl x509 -in /tmp/fifo -text

Comment 11 Elio Maldonado Batiz 2010-12-01 23:27:22 UTC
Created attachment 464129 [details]
FileToItem checks for and rejects directories

Comment 12 Rich Megginson 2010-12-01 23:46:14 UTC
Comment on attachment 464129 [details]
FileToItem checks for and rejects directories

Ok - but if type == PR_FILE_OTHER, what happens?  Does pemnss crash still, as it does with PR_FILE_DIRECTORY?

Comment 13 Elio Maldonado Batiz 2010-12-02 01:02:06 UTC
Possibly bad things if the caller passes us one the "wrong" types. Lumped into PR_FILE_OTHER are fifo (okay), symbolic link, (maybe okay) but also socket, block device, and character device all three of which are likely to make pemss crash when it tries to read them. There is no way to check the real type with what the NSPR API provides.

Comment 14 Rich Megginson 2010-12-02 01:51:59 UTC
(In reply to comment #13)
> Possibly bad things if the caller passes us one the "wrong" types. Lumped into
> PR_FILE_OTHER are fifo (okay), symbolic link, (maybe okay) but also socket,
> block device, and character device all three of which are likely to make pemss
> crash when it tries to read them. There is no way to check the real type with
> what the NSPR API provides.

Which is fine, as long as it doesn't crash.

Comment 15 Elio Maldonado Batiz 2010-12-02 23:16:20 UTC
Assuming a socket or block/character device or symbolic links to bad files are passed in and assuming the read call returns. Mild case is that we read contents we can't parse later on in which case the PEM module will return an error. Worst case we read a large amount of data and the AllocItem for it fails. In either case the PEM module returns CKR_ARGUMENTS_BAD to NSS. This is early so the module won't load and NSS will map this error to SEC_ERROR_INVALID_ARGS and set the error to that. PK11_LoadUserModule returns NULL to the caller who should call PK11_GetModule() which will return SEC_ERROR_INVALID_ARGS. This should be a good hint that those arguments, the file paths, are the cause.

The documentation at https://developer.mozilla.org/en/PR_Read says "The thread invoking PR_Read blocks until it encounters an end-of-stream indication, some positive number of bytes (but no more than amount bytes) are read in, or an error occurs." Question, could we end up blocking in one of these other cases?

Comment 16 Rich Megginson 2010-12-02 23:20:58 UTC
(In reply to comment #15)
> Assuming a socket or block/character device or symbolic links to bad files are
> passed in and assuming the read call returns. Mild case is that we read
> contents we can't parse later on in which case the PEM module will return an
> error. Worst case we read a large amount of data and the AllocItem for it
> fails. In either case the PEM module returns CKR_ARGUMENTS_BAD to NSS. This is
> early so the module won't load and NSS will map this error to
> SEC_ERROR_INVALID_ARGS and set the error to that. PK11_LoadUserModule returns
> NULL to the caller who should call PK11_GetModule() which will return
> SEC_ERROR_INVALID_ARGS. This should be a good hint that those arguments, the
> file paths, are the cause.

In the original case, it was crashing somewhere deep in the nsspem or perhaps the nss code.

> The documentation at https://developer.mozilla.org/en/PR_Read says "The thread
> invoking PR_Read blocks until it encounters an end-of-stream indication, some
> positive number of bytes (but no more than amount bytes) are read in, or an
> error occurs." Question, could we end up blocking in one of these other cases?

Yes.  If some malicious user is using a pipe as the cert, and sets it up never to block the read, there is really not much you can do about it, short of setting up some sort of timeout.  I would assume openssl hangs in this same situation too, so I wouldn't worry about this case too much.

Comment 17 Bob Relyea 2010-12-02 23:50:28 UTC
So there was some reason that you were crashing when reading the directory (funkyness in info.size or the failure of PR_Read). Both of these could happen in other cases, so I think you need to find out why SECITEM_FreeItem_Util is crashing on your SECItem.

bob

Comment 18 Elio Maldonado Batiz 2010-12-03 22:31:11 UTC
(In reply to comment #17)
> in other cases, so I think you need to find out why SECITEM_FreeItem_Util is
> crashing on your SECItem.
> 
ReadDERFromFile	calls rv = FileToItem(&filedata, inFile); with an argument that's a pointer to an stack allocated item. When FileToItem fails to allocate memory for the data it read or the read iteslef fails it goes to the cleanup section.
  loser:
    SECITEM_FreeItem(dst, PR_FALSE);
    nss_ZFreeIf(dst);
    return SECFailure;
First it's freeing twice, second its freeing dst which was never allocated as I mentioned. It should have kept in a variable the result of AllocItem and free  that instead. As you suggested to me AllocItem is not needed. SECITEM_AllocItem and SECITEM_ZfreeItem do the job. I'll attach a test application and a preliminary patch next.

Comment 19 Elio Maldonado Batiz 2010-12-03 22:32:44 UTC
Created attachment 464668 [details]
test application to reproduce the bug and verify fix

Comment 20 Elio Maldonado Batiz 2010-12-03 22:41:25 UTC
Created attachment 464675 [details]
Preliminary patch that gets rid of the crash

This is not the final patch, it just shoes the new memory handling which fixes and that but the crash when the initilization argument is a directory instead of a regular file.

Comment 21 Elio Maldonado Batiz 2010-12-07 22:50:19 UTC
(In reply to comment #20)
> Created attachment 464675 [details]
> Preliminary patch that gets rid of the crash
Yes, but it caused a crash in curl. Getting rid of AllocItem, though desirable in the long term, takes a bit more work. A smaller and less ambitious patch is called for at the time.

Comment 22 Elio Maldonado Batiz 2010-12-07 22:54:20 UTC
Created attachment 467321 [details]
Patch version 2

This one works with the attached application's tests, with the named-pipe test, and with curl.

Comment 23 Kamil Dudka 2010-12-07 23:03:00 UTC
Comment on attachment 467321 [details]
Patch version 2

>@@ -107,16 +108,17 @@ static SECStatus FileToItem(SECItem * ds
>     PRFileInfo info;
>     PRInt32 numBytes;
>     PRStatus prStatus;
>+    SECItem *result = NULL;

Why did you introduce the variable 'result'?  You don't read it anywhere.

Comment 24 Elio Maldonado Batiz 2010-12-07 23:16:38 UTC
Oh, that's a left-over from previous attempt's where I used to have lines such
SECITEM_FreeItem(result, PR_FALSE); and alternatively nss_ZFreeIf(result->data);
That's when I wanted to get rid of the AllocItem function and caused crashes in curl. Yes, I should get rid of result.

Comment 25 Elio Maldonado Batiz 2010-12-07 23:24:35 UTC
By the way, when I was testing I had this line
21c21
< +    if (prStatus != PR_SUCCESS || info.type == PR_FILE_DIRECTORY) {
changed back to the original
> +    if (prStatus != PR_SUCCESS) {
so I could see the read a few lines down fail when the file is a directory. Otherwise, I couldn't test for crashes.

Comment 26 Kamil Dudka 2010-12-07 23:27:20 UTC
Comment on attachment 467321 [details]
Patch version 2

>@@ -47,6 +47,7 @@
> #include "prnetdb.h"
> #include "base.h"
> #include "base64.h"
>+#include "secitem.h"

The same question here probably.  How is the include going to prevent the crash?

Feel free to drop the other call of SECITEM_FreeItem() in util.c -- it's dead code anyway.

Comment 27 Rob Crittenden 2010-12-08 14:47:27 UTC
When the incoming data is binary we already check the return value of FileToItem() but don't expliclity check for SECSuccess. Probably should make that explicit as well.

Otherwise this looks ok if you address Kamil's issues.

Comment 28 Elio Maldonado Batiz 2010-12-10 16:13:44 UTC
Created attachment 468000 [details]
Patch V3 with changes suggested by Kamil and Rob


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