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
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.
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...
*** Bug 653619 has been marked as a duplicate of this bug. ***
I have proposed a patch to openldap upstream to mitigate this problem: http://www.openldap.org/its/index.cgi/Contrib?id=6703
Please, try openldap-2.4.23-3.fc14 from updates-testing. Rich's patch is included.
(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
(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?
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.
(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.
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
Created attachment 464129 [details] FileToItem checks for and rejects directories
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?
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.
(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.
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?
(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.
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
(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.
Created attachment 464668 [details] test application to reproduce the bug and verify fix
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.
(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.
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 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.
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.
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 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.
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.
Created attachment 468000 [details] Patch V3 with changes suggested by Kamil and Rob