963150 - (CVE-2014-1544) heap-use-after-free in libnss3.so!CERT_DestroyCertificate

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Tyson Smith [:tsmith]]

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

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Almost definitely a problem with Necko or PSM. Thanks for reporting this.

Comment 2

[Tyson Smith [:tsmith]]

Attachment: use-after-poison.txt

Here is another log from ASAN. This one is a use after poison. Not sure how or why that happened.

Comment 3

[Wan-Teh Chang]

PL_FinishArenaPool should not be called when any thread that uses NSS
is still running. This is the cause of the heap-use-after-free and
use-after-poison.

The easiest fix would be to not call PL_FinishArenaPool, just like
we do not call PR_Cleanup.

Comment 4

[Al Billings [:abillings - ex-MoCo]]

Wan-Teh, are you able to work on this issue?

Comment 5

[Wan-Teh Chang]

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.

Comment 6

[Tyson Smith [:tsmith]]

Attachment: CERT_DestroyCertificate_bt_1.txt

I wish I could get a working test case for this. I do have a few variations of the crash. Hopefully they help.

Comment 7

[Tyson Smith [:tsmith]]

Attachment: CERT_DestroyCertificate_bt_2.txt

Comment 8

[Tyson Smith [:tsmith]]

Attachment: CERT_DestroyCertificate_bt_3.txt

Comment 9

[Tyson Smith [:tsmith]]

Attachment: CERT_DestroyCertificate_bt_4.txt

Comment 10

[Wan-Teh Chang]

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.

Comment 11

[Tyson Smith [:tsmith]]

Attachment: CERT_DestroyCertificate-heap-buffer-overflow.txt

This one is a heap-buffer-overflow.

Comment 12

[Wan-Teh Chang]

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.

Comment 13

[Tyson Smith [:tsmith]]

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?

Comment 14

[Wan-Teh Chang]

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.

Comment 15

[Tyson Smith [:tsmith]]

Logged the heap-buffer-overflow as https://bugzilla.mozilla.org/show_bug.cgi?id=976919

Comment 16

[Tyson Smith [:tsmith]]

Attachment: stack_1.txt

With NSS_DISABLE_ARENA_FREE_LIST=1 this seems to be what I get instead.

Comment 17

[Tyson Smith [:tsmith]]

Attachment: stack_2.txt

One more with NSS_DISABLE_ARENA_FREE_LIST=1

Comment 18

[Wan-Teh Chang]

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

Comment 19

[Tyson Smith [:tsmith]]

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.

Comment 20

[Wan-Teh Chang]

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

Comment 21

[Kai Engert [:KaiE:]]

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

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Is there anything actionable in this bug at this point?

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Can we close this bug?

Comment 24

[Tyson Smith [:tsmith]]

I am fine with closing it. I can reopen it if more information becomes available.

Comment 25

[Wan-Teh Chang]

OK, marked the bug WONTFIX.

Comment 26

[Christian Holler (:decoder)]

INCOMPLETE is probably the better resolution here :)

Comment 27

[Tyson Smith [:tsmith]]

Attachment: stack_trace.txt

Hopefully this provides a few more clues.

Comment 28

[Tyson Smith [:tsmith]]

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.

Comment 29

[Robert Relyea]

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.

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

Hi Wan-Teh, are you back from your time off?

Comment 31

[Wan-Teh Chang]

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?

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

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?

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 34

[Wan-Teh Chang]

Attachment: Bob Relyea's patch in comment 29

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

Comment 35

[Al Billings [:abillings - ex-MoCo]]

Bob, can you make some time to review this?

Comment 36

[Robert Relyea]

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

Comment 37

[Wan-Teh Chang]

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?

Comment 38

[Ryan VanderMeulen [:RyanVM]]

How far back in NSS does this go?

Comment 39

[Al Billings [:abillings - ex-MoCo]]

Are we taking an NSS update soon to get this?

Comment 40

[Robert Relyea]

> 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

Comment 41

[Kai Engert [:KaiE:]]

Wan-Teh, can this be marked fixed?

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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

Comment 43

[Al Billings [:abillings - ex-MoCo]]

If it is a sec-critical and affects ESR24, we'd need to backport it (or update NSS, in other words).

Comment 44

[Wan-Teh Chang]

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

Comment 45

[Wan-Teh Chang]

Attachment: Fix comments

Comment 46

[Wan-Teh Chang]

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

Comment 47

[Wan-Teh Chang]

Tyson: could you please verify if this bug is fixed? Thanks.

Comment 48

[Tyson Smith [:tsmith]]

Since I no longer have access to the tools to verify this issue I'll assign it to Jesse instead.

Comment 49

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 50

[Al Billings [:abillings - ex-MoCo]]

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

Comment 51

[Ryan VanderMeulen [:RyanVM]]

NSS was updated to version 3.16.2 across all supported branches in bug 1020695.

Comment 52

[Matt Wobensmith [:mwobensmith][:matt:]]

Based on comment 48, this is pending Jesse's verification.

I'm marking [qa-] on my end, due to lack of test or STR.

Comment 53

[Daniel Veditz [:dveditz]]

I'm lowering the security severity to sec-high since we were unable to find a testcase despite trying.

Comment 54

[Jesse Schwartzentruber (:truber)]

I haven't seen any more crashes with this signature.

Comment 55

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

sec@ recieved a request from the reporter wishing to be considered for a bounty