Bug 963150 (CVE-2014-1544)

heap-use-after-free in libnss3.so!CERT_DestroyCertificate

RESOLVED FIXED in Firefox 31

Status

defect
P1
critical
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: tsmith, Assigned: wtc)

Tracking

({csectype-uaf, sec-high})

Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox29 wontfix, firefox30+ wontfix, firefox31+ fixed, firefox32+ fixed, firefox33 fixed, firefox-esr24 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [qa-][adv-main31+][adv-esr24.7+][reporter-external])

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

5 years ago
Found by the BlackBerry Security Automated Analysis Team's fuzzing framework ALF.

At this time we are unable to create a test case that will reproduce the issue. However we do see it during our analysis a couple times a day so once a build with a fix is available we should be able to verify that this issue has been resolved. 

==5650==ERROR: AddressSanitizer: heap-use-after-free on address 0x61d0013a9f38 at pc 0x7f2091221f48 bp 0x7f207adcd0d0 sp 0x7f207adcd0c8
READ of size 8 at 0x61d0013a9f38 thread T3 (Socket Thread)
    #0 0x7f2091221f47 (libnss3.so!CERT_DestroyCertificate+0x57)
	Line 791 of "stanpcertdb.c"
    #1 0x7f208fb41b08 (libssl3.so!ssl3_DestroySSL3Info+0xd8)
	Line 9635 of "ssl3con.c"
    #2 0x7f208fb777ce (libssl3.so!ssl_DestroySocketContents+0x1e)
	Line 335 of "sslsock.c"
    #3 0x7f208fb77527 (libssl3.so!ssl_FreeSocket+0x187)
	Line 398 of "sslsock.c"
    #4 0x7f208fb5c129 (libssl3.so!ssl_DefClose+0xb9)
	Line 205 of "ssldef.c"
    #5 0x7f208a2baa20 (libxul.so!nsNSSSocketInfo::CloseSocketAndDestroy(nsNSSShutDownPreventionLock const&)+0xe0)
	Line 843 of "/builds/slave/m-in-l64-asan-0000000000000000/build/security/manager/ssl/src/nsNSSIOLayer.cpp"
    #6 0x7f208a2bc873 (libxul.so!nsSSLIOLayerClose(PRFileDesc*)+0x93)
	Line 821 of "/builds/slave/m-in-l64-asan-0000000000000000/build/security/manager/ssl/src/nsNSSIOLayer.cpp"
    #7 0x7f2084fa9f72 (libxul.so!nsSocketTransport::ReleaseFD_Locked(PRFileDesc*)+0x272)
	Line 1604 of "/builds/slave/m-in-l64-asan-0000000000000000/build/netwerk/base/src/nsSocketTransport2.cpp"
    #8 0x7f2084fb5d8e (libxul.so!nsSocketTransport::OnSocketDetached(PRFileDesc*)+0x3ce)
	Line 1867 of "/builds/slave/m-in-l64-asan-0000000000000000/build/netwerk/base/src/nsSocketTransport2.cpp"
    #9 0x7f2084fbbe0f (libxul.so!nsSocketTransportService::DetachSocket(nsSocketTransportService::SocketContext*, nsSocketTransportService::SocketContext*)+0x12f)
	Line 181 of "/builds/slave/m-in-l64-asan-0000000000000000/build/netwerk/base/src/nsSocketTransportService2.cpp"
    #10 0x7f2084fc0757 (libxul.so!nsSocketTransportService::DoPollIteration(bool)+0x9e7)
	Line 862 of "/builds/slave/m-in-l64-asan-0000000000000000/build/netwerk/base/src/nsSocketTransportService2.cpp"
    #11 0x7f2084fbf94f (libxul.so!nsSocketTransportService::Run()+0x31f)
	Line 684 of "/builds/slave/m-in-l64-asan-0000000000000000/build/netwerk/base/src/nsSocketTransportService2.cpp"
    #12 0x7f2084fc1139 (libxul.so!non-virtual thunk to nsSocketTransportService::Run()+0x9)
	Line 728 of "/builds/slave/m-in-l64-asan-0000000000000000/build/netwerk/base/src/nsSocketTransportService2.cpp"
    #13 0x7f2084ed9cf7 (libxul.so!nsThread::ProcessNextEvent(bool, bool*)+0xbd7)
	Line 637 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsThread.cpp"
    #14 0x7f2084db26d1 (libxul.so!NS_ProcessNextEvent(nsIThread*, bool)+0xb1)
	Line 263 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/glue/nsThreadUtils.cpp"
    #15 0x7f20856d484f (libxul.so!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)+0x1cf)
	Line 301 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/glue/MessagePump.cpp"
    #16 0x7f20856452f3 (libxul.so!MessageLoop::Run()+0x1c3)
	Line 226 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/chromium/src/base/message_loop.cc"
    #17 0x7f2084ed69a2 (libxul.so!nsThread::ThreadFunc(void*)+0x192)
	Line 258 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsThread.cpp"
    #18 0x7f2091dd7c49 (libnspr4.so!_pt_root+0x4e9)
	Line 205 of "/builds/slave/m-in-l64-asan-0000000000000000/build/nsprpub/pr/src/pthreads/ptthread.c"
    #19 0x44cf03 (firefox!__asan::AsanThread::ThreadStart(unsigned long)+0x83)
	Line 138 of "/builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_thread.cc"
    #20 0x7f20963c3e99 (libpthread.so.0!start_thread+0xd9)
	Line 308 of "pthread_create.c"
    #21 0x7f20954ce3fc (libc.so.6!clone+0x6c)
	Line 112 of "../sysdeps/unix/sysv/linux/x86_64/clone.S"
0x61d0013a9f38 is located 696 bytes inside of 2048-byte region [0x61d0013a9c80,0x61d0013aa480)
freed by thread T0 here:
    #0 0x446255 (firefox!free+0x55)
	Line 64 of "/builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc"
    #1 0x7f209195c7c3 (libplds4.so!PL_FinishArenaPool+0x43)
	Line 277 of "/builds/slave/m-in-l64-asan-0000000000000000/build/nsprpub/lib/ds/plarena.c"
    #2 0x7f20891da741 (libxul.so!PresShell::Paint(nsView*, nsRegion const&, unsigned int)+0xe11)
	Line 5847 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsPresShell.cpp"
    #3 0x7f208817a87c (libxul.so!nsViewManager::ProcessPendingUpdatesForView(nsView*, bool)+0x3fc)
	Line 420 of "/builds/slave/m-in-l64-asan-0000000000000000/build/view/src/nsViewManager.cpp"
    #4 0x7f20891fe1d0 (libxul.so!nsRefreshDriver::Tick(long, mozilla::TimeStamp)+0x2ba0)
	Line 1207 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsRefreshDriver.cpp"
    #5 0x7f2089202030 (libxul.so!mozilla::RefreshDriverTimer::Tick()+0x1f0)
	Line 168 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsRefreshDriver.cpp"
    #6 0x7f2084ee1d83 (libxul.so!nsTimerImpl::Fire()+0x6b3)
	Line 551 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsTimerImpl.cpp"
    #7 0x7f2084ee24e6 (libxul.so!nsTimerEvent::Run()+0x66)
	Line 635 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsTimerImpl.cpp"
    #8 0x7f2084db26d1 (libxul.so!NS_ProcessNextEvent(nsIThread*, bool)+0xb1)
	Line 263 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/glue/nsThreadUtils.cpp"
    #9 0x7f20856d3a60 (libxul.so!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)+0x2f0)
	Line 134 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/glue/MessagePump.cpp"
    #10 0x7f20856452f3 (libxul.so!MessageLoop::Run()+0x1c3)
	Line 226 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/chromium/src/base/message_loop.cc"
    #11 0x7f208780da5c (libxul.so!nsBaseAppShell::Run()+0x5c)
	Line 161 of "/builds/slave/m-in-l64-asan-0000000000000000/build/widget/xpwidgets/nsBaseAppShell.cpp"
    #12 0x7f208a316bda (libxul.so!XREMain::XRE_main(int, char**, nsXREAppData const*)+0x4fa)
	Line 4091 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #13 0x7f208a317b0b (libxul.so!XRE_main+0x3ab)
	Line 4331 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #14 0x459dcd (firefox!main+0x94d)
	Line 280 of "/builds/slave/m-in-l64-asan-0000000000000000/build/browser/app/nsBrowserApp.cpp"
    #15 0x7f20953fb76c (libc.so.6!__libc_start_main+0xec)
	Line 226 of "libc-start.c"
previously allocated by thread T3 (Socket Thread) here:
    #0 0x446395 (firefox!malloc+0x55)
	Line 74 of "/builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc"
    #1 0x7f209195bd8d (libplds4.so!PL_ArenaAllocate+0x1ed)
	Line 203 of "/builds/slave/m-in-l64-asan-0000000000000000/build/nsprpub/lib/ds/plarena.c"
Thread T3 (Socket Thread) created by T0 here:
    #0 0x437801 (firefox!pthread_create+0x71)
	Line 155 of "/builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc"
    #1 0x7f2091dd3be5 (libnspr4.so!_PR_CreateThread+0x4a5)
	Line 445 of "/builds/slave/m-in-l64-asan-0000000000000000/build/nsprpub/pr/src/pthreads/ptthread.c"
    #2 0x7f2091dd3737 (libnspr4.so!PR_CreateThread+0x17)
	Line 528 of "/builds/slave/m-in-l64-asan-0000000000000000/build/nsprpub/pr/src/pthreads/ptthread.c"
Shadow bytes around the buggy address:
  0x0c3a8026d390: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3a8026d3a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3a8026d3b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3a8026d3c0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3a8026d3d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c3a8026d3e0: fd fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd
  0x0c3a8026d3f0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3a8026d400: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3a8026d410: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3a8026d420: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c3a8026d430: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:     fa
  Heap right redzone:    fb
  Freed heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==5650==ABORTING
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → nobody
Component: Security → CA Certificates
Product: Core → NSS
Version: 29 Branch → 3.15.3

Updated

5 years ago
Component: CA Certificates → Libraries
Blocks: 965314
Almost definitely a problem with Necko or PSM. Thanks for reporting this.
Assignee: nobody → nobody
Component: Libraries → Security: PSM
Product: NSS → Core
Version: 3.15.3 → unspecified
(Reporter)

Comment 2

5 years ago
Posted file use-after-poison.txt (obsolete) —
Here is another log from ASAN. This one is a use after poison. Not sure how or why that happened.
Comment hidden (obsolete)
Wan-Teh, are you able to work on this issue?
Flags: needinfo?(wtc)
(Assignee)

Comment 5

5 years ago
Al:

My comment 3 was wrong. I confused PL_FinishArenaPool with PL_ArenaFinish.
It is perfectly fine to call PL_FinishArenaPool. Please disregard comment 3
in its entirety.

ssl3con.c:9635 is:

 9628 static void
 9629 ssl3_CleanupPeerCerts(sslSocket *ss)
 9630 {
 9631     PLArenaPool * arena = ss->ssl3.peerCertArena;
 9632     ssl3CertNode *certs = (ssl3CertNode *)ss->ssl3.peerCertChain;
 9633   
 9634     for (; certs; certs = certs->next) {
 9635         CERT_DestroyCertificate(certs->cert);
 9636     }
 9637     if (arena) PORT_FreeArena(arena, PR_FALSE);
 9638     ss->ssl3.peerCertArena = NULL;
 9639     ss->ssl3.peerCertChain = NULL;
 9640 }
 9641 

The call stack says it's the ssl3_DestroySSL3Info, so I guess compiler inlined
the ssl3_CleanupPeerCerts call. I inspected the relevant code in NSS but didn't
see anything wrong.
Flags: needinfo?(wtc)
(Reporter)

Comment 6

5 years ago
Posted file CERT_DestroyCertificate_bt_1.txt (obsolete) —
I wish I could get a working test case for this. I do have a few variations of the crash. Hopefully they help.
(Reporter)

Comment 7

5 years ago
Posted file CERT_DestroyCertificate_bt_2.txt (obsolete) —
(Reporter)

Comment 8

5 years ago
Posted file CERT_DestroyCertificate_bt_3.txt (obsolete) —
(Reporter)

Comment 9

5 years ago
Posted file CERT_DestroyCertificate_bt_4.txt (obsolete) —
(Assignee)

Comment 10

5 years ago
Tyson:

Thanks a lot for the four new crash reports. They show the problem is
independent of the caller of CERT_DestroyCertificate. I suspect some
code is calling CERT_DestroyCertificate when it should not.

Could you please set the environment variable NSS_DISABLE_ARENA_FREE_LIST
to 1, and run Firefox under AddressSanitizer again? Please build Firefox
with the in-tree NSS. If you configured Firefox with the --with-system-nss
configure option, please remove that configure option.

This will show us the real culprit who makes the incorrect call to
CERT_DestroyCertificate. Thanks.
(Reporter)

Comment 11

5 years ago
This one is a heap-buffer-overflow.
(Assignee)

Comment 12

5 years ago
Tyson: Thank you for the new heap-buffer-overflow report. I
assume you are only getting this error after setting the
environment variable NSS_DISABLE_ARENA_FREE_LIST to 1?

If so, we will need to fix this heap-buffer-overflow error
first before we can track down the original heap-use-after-free
error.
(Reporter)

Comment 13

5 years ago
The heap-buffer-overflow report was found before I set NSS_DISABLE_ARENA_FREE_LIST to 1. I have since done so and I will post a back trace as soon as I get one.

Should I log a separate bug for the heap-buffer-overflow?
(Assignee)

Comment 14

5 years ago
Yes, please log a separate bug for the heap-buffer-overflow.
The back trace is strange because it says NSS freed a huge
3026032-byte region. I inspected the relevant code yesterday
but didn't see anything wrong.

I ran the NSS test suite several times under AddressSanitizer
today but could not reproduce either the heap-use-after-free
or the heap-buffer-overflow. So I will wait for your new back
trace. Thanks.
(Reporter)

Comment 15

5 years ago
Logged the heap-buffer-overflow as https://bugzilla.mozilla.org/show_bug.cgi?id=976919
(Reporter)

Comment 16

5 years ago
Posted file stack_1.txt (obsolete) —
With NSS_DISABLE_ARENA_FREE_LIST=1 this seems to be what I get instead.
(Reporter)

Comment 17

5 years ago
Posted file stack_2.txt (obsolete) —
One more with NSS_DISABLE_ARENA_FREE_LIST=1
(Assignee)

Comment 18

5 years ago
Comment on attachment 8383121 [details]
stack_2.txt

Tyson:

Thanks a lot for the new stack traces. They are a different problem.
Were these crashes? They don't look like AddressSanitizer error reports.

>#0  0x00007ffff7bcc2cc in __libc_send (fd=<optimized out>, buf=<optimized out>, n=<optimized out>, flags=<optimized out>) at ../sysdeps/unix/sysv/linux/x86_64/send.c:33
>        resultvar = <optimized out>
>        oldtype = 0
>        result = <optimized out>
>#1  0x00007ffff46a1a8f in pt_Send (fd=<optimized out>, buf=0x62900050f200, amount=517, flags=0, timeout=4294967295) at /builds/slave/m-in-l64-asan-0000000000000000/build/nsprpub/pr/src/pthreads/ptio.c:1914
>        fNeedContinue = <error reading variable fNeedContinue (Cannot access memory at address 0x0)>
>        bytes = <error reading variable bytes (Cannot access memory at address 0xffffffffffffffff)>
>        syserrno = <optimized out>

nsprpub/pr/src/pthreads/ptio.c:1914 is a call to the send() system call:

1909 #if defined(SOLARIS)
1910     PR_ASSERT(0 == flags);
1911 retry:
1912     bytes = write(fd->secret->md.osfd, PT_SENDBUF_CAST buf, tmp_amount);
1913 #else
1914     bytes = send(fd->secret->md.osfd, PT_SENDBUF_CAST buf, amount, flags);
1915 #endif
(Reporter)

Comment 19

5 years ago
Wan-Teh:

Yeah you're right, gdb says: "Program received signal SIGPIPE, Broken pipe."

The reason I added this is because I believe that both of these issues are triggered by the timing of the server that is serving my test cases. This may be a long shot but is it possible that the SIGPIPE leads to the original issue?

I tried recreating the conditions that I believe are triggering the issue but I haven't had any luck, so I will continue to monitor our main test system for new logs.
(Assignee)

Comment 20

5 years ago
Tyson:

Receiving SIGPIPE is normal and is not an error. All networking apps need to
install a signal handler for SIGPIPE to ignore it.

When running Firefox in gdb, you need to tell gdb to ignore SIGPIPE. (I forgot
what the gdb command is.)
> When running Firefox in gdb, you need to tell gdb to ignore SIGPIPE. (I
> forgot what the gdb command is.)

Use "help handle"

handle SIGPIPE <action>

Recognized actions include "stop", "nostop", "print", "noprint",
"pass", "nopass", "ignore", or "noignore".
Is there anything actionable in this bug at this point?
Can we close this bug?
Flags: needinfo?(wtc)
(Reporter)

Comment 24

5 years ago
I am fine with closing it. I can reopen it if more information becomes available.
(Assignee)

Comment 25

5 years ago
OK, marked the bug WONTFIX.
Assignee: nobody → wtc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(wtc)
Resolution: --- → WONTFIX
INCOMPLETE is probably the better resolution here :)
Resolution: WONTFIX → INCOMPLETE
(Reporter)

Comment 27

5 years ago
Posted file stack_trace.txt
Hopefully this provides a few more clues.
Attachment #8370155 - Attachment is obsolete: true
Attachment #8379436 - Attachment is obsolete: true
Attachment #8379437 - Attachment is obsolete: true
Attachment #8379438 - Attachment is obsolete: true
Attachment #8379439 - Attachment is obsolete: true
(Reporter)

Comment 28

5 years ago
Wan-Teh:

What do you think? Feel free to close this issue again if this new stack trace does not provide the required missing pieces.
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Flags: needinfo?(wtc)

Comment 29

5 years ago
wtc's is away right now, I'd like him to review my analysis here when he gets back.

As a first cut at what is going on here, it looks like we have a race between 2 NSSCertificate structures being added to the trust domain. I think the normal case is the 2 certs point to identical cert pointers and the failing case is when they aren't identical. The free in the trust domain frees the NSS certificate out from under the CERTCertificate passed into PK11_ImportCertificates(). Since the caller of PK11_ImportCertificates doesn't expect his cert to go away, this will cause some problems. PK11_ImportCertificates will get the new cert (with a new reference) from the nssTrustDomain_AddCertsToCache(), while the old cert is freed out from underneath (also freeing the CERTCertificate associated with it).

If we add the following code to PK11_ImportCert(), we would preserve the old NSSCertificate and free our dangling reference to the new one. 

diff --git a/lib/pk11wrap/pk11cert.c b/lib/pk11wrap/pk11cert.c
--- a/lib/pk11wrap/pk11cert.c
+++ b/lib/pk11wrap/pk11cert.c
@@ -976,18 +976,20 @@ PK11_ImportCert(PK11SlotInfo *slot, CERT
 	cert->istemp = PR_FALSE;
 	cert->isperm = PR_TRUE;
     }
 
     /* add the new instance to the cert, force an update of the
      * CERTCertificate, and finish
      */
     nssPKIObject_AddInstance(&c->object, certobj);
+    nssCertificate_AddRef(c);
     nssTrustDomain_AddCertsToCache(STAN_GetDefaultTrustDomain(), &c, 1);
     (void)STAN_ForceCERTCertificateUpdate(c);
+    nssCertificate_Destroy(c);
     SECITEM_FreeItem(keyID,PR_TRUE);
     return SECSuccess;
 loser:
     CERT_MapStanError();
     SECITEM_FreeItem(keyID,PR_TRUE);
     if (PORT_GetError() != SEC_ERROR_TOKEN_NOT_LOGGED_IN) {
 	PORT_SetError(SEC_ERROR_ADDING_CERT);
     }


We still have the problem of how we wound up with two separate CERTCertificate/NSSCertificate structures pointing to the same certificate, but at least this would clear up the memory issue.
Group: dom-core-security
Hi Wan-Teh, are you back from your time off?
(Assignee)

Updated

5 years ago
Attachment #8383121 - Attachment is obsolete: true
Flags: needinfo?(wtc)
(Assignee)

Updated

5 years ago
Attachment #8383120 - Attachment is obsolete: true
(Assignee)

Comment 31

5 years ago
Bob: your analysis seems correct.

(In reply to Robert Relyea from comment #29)
>
> If we add the following code to PK11_ImportCert(), we would preserve the old
> NSSCertificate and free our dangling reference to the new one. 
> 
> diff --git a/lib/pk11wrap/pk11cert.c b/lib/pk11wrap/pk11cert.c
> --- a/lib/pk11wrap/pk11cert.c
> +++ b/lib/pk11wrap/pk11cert.c
> @@ -976,18 +976,20 @@ PK11_ImportCert(PK11SlotInfo *slot, CERT
>  	cert->istemp = PR_FALSE;
>  	cert->isperm = PR_TRUE;
>      }
>  
>      /* add the new instance to the cert, force an update of the
>       * CERTCertificate, and finish
>       */
>      nssPKIObject_AddInstance(&c->object, certobj);
> +    nssCertificate_AddRef(c);
>      nssTrustDomain_AddCertsToCache(STAN_GetDefaultTrustDomain(), &c, 1);
>      (void)STAN_ForceCERTCertificateUpdate(c);
> +    nssCertificate_Destroy(c);
>      SECITEM_FreeItem(keyID,PR_TRUE);
>      return SECSuccess;

If |c| is swapped by nssTrustDomain_AddCertsToCache, the
STAN_ForceCERTCertificateUpdate will be updating the new
value of |c|. Do you think it should update the original
value of |c| instead?
(Assignee)

Updated

5 years ago
Group: dom-core-security
Status: REOPENED → ASSIGNED
Component: Security: PSM → Libraries
Product: Core → NSS
Target Milestone: --- → 3.16.2
Version: unspecified → trunk
Wan-Teh, we are in the last couple of weeks of Beta for FF30 - is this something you are actively working on this week & next?
Flags: needinfo?(wtc)
We're too late for FF30, with the final beta being sent off to build and just the RC next Monday.  Will have to push this to target FF31.
(Assignee)

Comment 34

5 years ago
Bob, this is your patch in comment 29. Please review the comments
I added. One of the comments is a question for you.  Thanks.

I've checked it in: https://hg.mozilla.org/projects/nss/rev/204f22c527f8
Attachment #8434400 - Flags: review?(rrelyea)
Attachment #8434400 - Flags: checked-in+
Flags: needinfo?(wtc)
Bob, can you make some time to review this?
Flags: needinfo?(rrelyea)

Comment 36

5 years ago
Comment on attachment 8434400 [details] [diff] [review]
Bob Relyea's patch in comment 29

r+ rrelyea.... oops I forgot to r+ this after I reviewed it yesterday.

bob
Attachment #8434400 - Flags: review?(rrelyea) → review+
Flags: needinfo?(rrelyea)
(Assignee)

Comment 37

5 years ago
Comment on attachment 8434400 [details] [diff] [review]
Bob Relyea's patch in comment 29

Review of attachment 8434400 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/pk11wrap/pk11cert.c
@@ +991,1 @@
>      (void)STAN_ForceCERTCertificateUpdate(c);

Bob: this XXX comment is a question for you. Here we are passing the
updated value of 'c' to STAN_ForceCERTCertificateUpdate. Should we
pass the original value of 'c' instead?
(Assignee)

Updated

5 years ago
Flags: needinfo?(rrelyea)
How far back in NSS does this go?
Are we taking an NSS update soon to get this?

Comment 40

5 years ago
> Bob: this XXX comment is a question for you. Here we are passing the
> updated value of 'c' to STAN_ForceCERTCertificateUpdate. Should we
> pass the original value of 'c' instead?

No, I don't think so. The original 'c' is a doa pointer that will disappear once the caller frees it.

bob
Flags: needinfo?(rrelyea)
Wan-Teh, can this be marked fixed?
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #38)
> How far back in NSS does this go?

To answer my own question, 3.5.0 if I'm reading the blame correctly (bug 135429). Do we need to get this backported all the way to esr24?
If it is a sec-critical and affects ESR24, we'd need to backport it (or update NSS, in other words).
(Assignee)

Comment 44

5 years ago
(In reply to Robert Relyea from comment #40)
> > Bob: this XXX comment is a question for you. Here we are passing the
> > updated value of 'c' to STAN_ForceCERTCertificateUpdate. Should we
> > pass the original value of 'c' instead?
> 
> No, I don't think so. The original 'c' is a doa pointer that will disappear
> once the caller frees it.

Bob: I am confused. The original 'c' is still available in cert->nssCertificate.
Since we add a reference to the original 'c', cert->nssCertificate is unchanged
and still valid after the nssTrustDomain_AddCertsToCache call. So we could pass
cert->nssCertificate instead of 'c' to STAN_ForceCERTCertificateUpdate.
(Assignee)

Comment 45

5 years ago
Posted patch Fix commentsSplinter Review
Attachment #8446282 - Flags: review?(rrelyea)

Updated

5 years ago
Attachment #8446282 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 46

5 years ago
Comment on attachment 8446282 [details] [diff] [review]
Fix comments

Review of attachment 8446282 [details] [diff] [review]:
-----------------------------------------------------------------

Patch checked in: https://hg.mozilla.org/projects/nss/rev/872dd4d243ac
Attachment #8446282 - Flags: checked-in+
(Assignee)

Comment 47

5 years ago
Tyson: could you please verify if this bug is fixed? Thanks.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Flags: needinfo?(tyson.w.smith)
Priority: -- → P1
Resolution: --- → FIXED
(Reporter)

Comment 48

5 years ago
Since I no longer have access to the tools to verify this issue I'll assign it to Jesse instead.
Flags: needinfo?(jdschwa)
(Reporter)

Updated

5 years ago
Flags: needinfo?(tyson.w.smith)
(In reply to Al Billings [:abillings] from comment #43)
> If it is a sec-critical and affects ESR24, we'd need to backport it (or
> update NSS, in other words).

Al - This bug is several months old. Can you comment on the need to backport this to ESR, which will live for 3 more cycles (>16 more weeks).

I would also appreciate a short explanation of the risk associated with updating NSS on ESR and beta. Has NSS had any potentially breaking changes?
(In reply to Lawrence Mandel [:lmandel] from comment #49)
> (In reply to Al Billings [:abillings] from comment #43)
> > If it is a sec-critical and affects ESR24, we'd need to backport it (or
> > update NSS, in other words).
> 
> Al - This bug is several months old. Can you comment on the need to backport
> this to ESR, which will live for 3 more cycles (>16 more weeks).

It is a critical bug on a branch we're still supporting for three more cycles. That is the "need" in question. This is not a low rated issue.

The age of the bug is immaterial.
Alias: CVE-2014-1544
Based on comment 48, this is pending Jesse's verification.

I'm marking [qa-] on my end, due to lack of test or STR.
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][adv-main31+][adv-esr24.7+]
I'm lowering the security severity to sec-high since we were unable to find a testcase despite trying.
Keywords: sec-criticalsec-high
I haven't seen any more crashes with this signature.
Flags: needinfo?(jdschwa)
sec@ recieved a request from the reporter wishing to be considered for a bounty
Flags: sec-bounty?
Whiteboard: [qa-][adv-main31+][adv-esr24.7+] → [qa-][adv-main31+][adv-esr24.7+][reporter-external]
Flags: sec-bounty? → sec-bounty+

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.