Nickname race in PK11_ImportCert (potential heap-use-after-free in nssUTF8_Duplicate)

RESOLVED FIXED in Firefox 14

Status

RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: inferno, Assigned: kaie)

Tracking

({csectype-uaf, sec-critical})

3.13.4
3.13.5
x86_64
Windows 7
csectype-uaf, sec-critical
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox12+, firefox13+ affected, firefox14+ fixed, firefox15+ fixed, firefox-esr1014+ fixed)

Details

(Whiteboard: [asan][sg:critical][qa?])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
Created attachment 615168 [details]
Crash testcase

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.83 Safari/535.11

Steps to reproduce:

1. Make sure that nss security lib is compiled with ASAN. This can be done in security/coreconf/Linux.mk, line OS_LIBS                 = $(OS_PTHREAD) -ldl -lc -faddress-sanitizer
2. Use this python script to reproduce easily (make sure to start with clean profile dirs to prevent caching of cert info). I have tested with Firefox ASANified 12 Beta. After running this script, you will see atleast one log file in /tmp of non-zero size. that one will have the asan stack.

import os
thread_num = 22
for i in range(0, thread_num):
  profile_dir = '/tmp/firefox_profile_%d' % i
  os.system('mkdir %s' % profile_dir)
  os.system('touch %s/pref.js' % profile_dir)
  cmd = "path-to-your-firefox/beta-src/objdir-ff-asan/dist/bin/firefox-bin -no-remote -profile '%s' test.html 2>&1 | tee /tmp/%d.log &" % (profile_dir, i)
  os.system(cmd)

=================================================================
==14919== ERROR: AddressSanitizer heap-use-after-free on address 0x7f4a7b995b90 at pc 0x42d1f0 bp 0x7f4a6d5190b0 sp 0x7f4a6d519098
READ of size 1 at 0x7f4a7b995b90 thread T15
    #0 0x42d1f0 in strlen ??:0
    #1 0x7f4ab199032a in PL_strlen firefox/beta-src/nsprpub/lib/libc/src/strlen.c:50
    #2 0x7f4ab02638f9 in nssUTF8_Duplicate firefox/beta-src/security/nss/lib/base/utf8.c:262
    #3 0x7f4ab0251566 in add_subject_entry firefox/beta-src/security/nss/lib/pki/tdcache.c:581
    #4 0x7f4ab02508fe in add_cert_to_cache firefox/beta-src/security/nss/lib/pki/tdcache.c:799
    #5 0x7f4ab0250714 in nssTrustDomain_AddCertsToCache firefox/beta-src/security/nss/lib/pki/tdcache.c:880
    #6 0x7f4ab01f5e4b in PK11_ImportCert firefox/beta-src/security/nss/lib/pk11wrap/pk11cert.c:1015
    #7 0x7f4aa29be655 in AuthCertificate firefox/beta-src/security/manager/ssl/src/SSLServerCertVerification.cpp:940
    #8 0x7f4aa29d19a5 in mozilla::psm::(anonymous namespace)::SSLServerCertVerificationJob::Run() firefox/beta-src/security/manager/ssl/src/SSLServerCertVerification.cpp:1038
    #9 0x7f4aa5577007 in nsThreadPool::Run() firefox/beta-src/xpcom/threads/nsThreadPool.cpp:217
    #10 0x7f4aa55782ec in non-virtual thunk to nsThreadPool::Run() crmftmpl.c:0
    #11 0x7f4aa5552879 in nsThread::ProcessNextEvent(bool, bool*) firefox/beta-src/xpcom/threads/nsThread.cpp:658
    #12 0x7f4aa5188590 in NS_ProcessNextEvent_P(nsIThread*, bool) firefox/beta-src/objdir-ff-asan-2/xpcom/build/nsThreadUtils.cpp:245
    #13 0x7f4aa5549683 in nsThread::ThreadFunc(void*) firefox/beta-src/xpcom/threads/nsThread.cpp:289
    #14 0x7f4ab219feb6 in _pt_root firefox/beta-src/nsprpub/pr/src/pthreads/ptthread.c:187
    #15 0x43219b in __asan::AsanThread::ThreadStart() ??:0
0x7f4a7b995b90 is located 16 bytes inside of 54-byte region [0x7f4a7b995b80,0x7f4a7b995bb6)
freed by thread T17 here:
    #0 0x42f082 in free ??:0
    #1 0x7f4ab2060423 in PR_Free firefox/beta-src/nsprpub/pr/src/malloc/prmem.c:491
    #2 0x7f4ab0261654 in nss_ZFreeIf firefox/beta-src/security/nss/lib/base/arena.c:976
    #3 0x7f4ab0253470 in nssPKIObject_AddInstance firefox/beta-src/security/nss/lib/pki/pkibase.c:225
    #4 0x7f4ab01f5e2d in PK11_ImportCert firefox/beta-src/security/nss/lib/pk11wrap/pk11cert.c:1014
    #5 0x7f4aa29be655 in AuthCertificate firefox/beta-src/security/manager/ssl/src/SSLServerCertVerification.cpp:940
    #6 0x7f4aa29d19a5 in mozilla::psm::(anonymous namespace)::SSLServerCertVerificationJob::Run() firefox/beta-src/security/manager/ssl/src/SSLServerCertVerification.cpp:1038
    #7 0x7f4aa5577007 in nsThreadPool::Run() firefox/beta-src/xpcom/threads/nsThreadPool.cpp:217
    #8 0x7f4aa55782ec in non-virtual thunk to nsThreadPool::Run() crmftmpl.c:0
    #9 0x7f4aa5552879 in nsThread::ProcessNextEvent(bool, bool*) firefox/beta-src/xpcom/threads/nsThread.cpp:658
    #10 0x7f4aa5188590 in NS_ProcessNextEvent_P(nsIThread*, bool) firefox/beta-src/objdir-ff-asan-2/xpcom/build/nsThreadUtils.cpp:245
    #11 0x7f4aa5549683 in nsThread::ThreadFunc(void*) firefox/beta-src/xpcom/threads/nsThread.cpp:289
    #12 0x7f4ab219feb6 in _pt_root firefox/beta-src/nsprpub/pr/src/pthreads/ptthread.c:187
    #13 0x43219b in __asan::AsanThread::ThreadStart() ??:0
previously allocated by thread T15 here:
    #0 0x42f1e6 in calloc ??:0
    #1 0x7f4ab205dfe1 in PR_Calloc firefox/beta-src/nsprpub/pr/src/malloc/prmem.c:475
    #2 0x7f4ab02612fa in nss_ZAlloc firefox/beta-src/security/nss/lib/base/arena.c:892
    #3 0x7f4ab025f6e2 in nssCKObject_GetAttributes firefox/beta-src/security/nss/lib/dev/ckhelper.c:142
    #4 0x7f4ab025d55d in nssCryptokiObject_Create firefox/beta-src/security/nss/lib/dev/devutil.c:65
    #5 0x7f4ab0259e1b in import_object firefox/beta-src/security/nss/lib/dev/devtoken.c:251
    #6 0x7f4ab02597db in nssToken_ImportCertificate firefox/beta-src/security/nss/lib/dev/devtoken.c:588
    #7 0x7f4ab01f5d3f in PK11_ImportCert firefox/beta-src/security/nss/lib/pk11wrap/pk11cert.c:980
    #8 0x7f4aa29be655 in AuthCertificate firefox/beta-src/security/manager/ssl/src/SSLServerCertVerification.cpp:940
    #9 0x7f4aa29d19a5 in mozilla::psm::(anonymous namespace)::SSLServerCertVerificationJob::Run() firefox/beta-src/security/manager/ssl/src/SSLServerCertVerification.cpp:1038
    #10 0x7f4aa5577007 in nsThreadPool::Run() firefox/beta-src/xpcom/threads/nsThreadPool.cpp:217
    #11 0x7f4aa55782ec in non-virtual thunk to nsThreadPool::Run() crmftmpl.c:0
    #12 0x7f4aa5552879 in nsThread::ProcessNextEvent(bool, bool*) firefox/beta-src/xpcom/threads/nsThread.cpp:658
    #13 0x7f4aa5188590 in NS_ProcessNextEvent_P(nsIThread*, bool) firefox/beta-src/objdir-ff-asan-2/xpcom/build/nsThreadUtils.cpp:245
    #14 0x7f4aa5549683 in nsThread::ThreadFunc(void*) firefox/beta-src/xpcom/threads/nsThread.cpp:289
    #15 0x7f4ab219feb6 in _pt_root firefox/beta-src/nsprpub/pr/src/pthreads/ptthread.c:187
    #16 0x43219b in __asan::AsanThread::ThreadStart() ??:0
Thread T15 created by T3 here:
    #0 0x42b4f3 in pthread_create ??:0
    #1 0x7f4ab2190516 in _PR_CreateThread firefox/beta-src/nsprpub/pr/src/pthreads/ptthread.c:424
    #2 0x7f4ab218ea9f in PR_CreateThread firefox/beta-src/nsprpub/pr/src/pthreads/ptthread.c:507
    #3 0x7f4aa554bdc8 in nsThread::Init() firefox/beta-src/xpcom/threads/nsThread.cpp:356
    #4 0x7f4aa5568fc6 in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) firefox/beta-src/xpcom/threads/nsThreadManager.cpp:248
    #5 0x7f4aa5574776 in nsThreadPool::PutEvent(nsIRunnable*) firefox/beta-src/xpcom/threads/nsThreadPool.cpp:114
    #6 0x7f4aa5578b82 in nsThreadPool::Dispatch(nsIRunnable*, unsigned int) firefox/beta-src/xpcom/threads/nsThreadPool.cpp:253
    #7 0x7f4aa29bc67b in mozilla::psm::(anonymous namespace)::SSLServerCertVerificationJob::Dispatch(void const*, nsNSSSocketInfo*, CERTCertificateStr*) firefox/beta-src/security/manager/ssl/src/SSLServerCertVerification.cpp:1000
    #8 0x7f4aa29bac0c in mozilla::psm::AuthCertificateHook(void*, PRFileDesc*, int, int) firefox/beta-src/security/manager/ssl/src/SSLServerCertVerification.cpp:1141
    #9 0x7f4aaff69648 in ssl3_HandleCertificate firefox/beta-src/security/nss/lib/ssl/ssl3con.c:8000
    #10 0x7f4aaff650b6 in ssl3_HandleHandshakeMessage firefox/beta-src/security/nss/lib/ssl/ssl3con.c:8734
    #11 0x7f4aaff633ca in ssl3_HandleHandshake firefox/beta-src/security/nss/lib/ssl/ssl3con.c:8858
    #12 0x7f4aaff61fd7 in ssl3_HandleRecord firefox/beta-src/security/nss/lib/ssl/ssl3con.c:9196
    #13 0x7f4aaff71275 in ssl3_GatherCompleteHandshake firefox/beta-src/security/nss/lib/ssl/ssl3gthr.c:240
    #14 0x7f4aaff72cbc in ssl_GatherRecord1stHandshake firefox/beta-src/security/nss/lib/ssl/sslcon.c:1253
    #15 0x7f4aaff82e42 in ssl_Do1stHandshake firefox/beta-src/security/nss/lib/ssl/sslsecur.c:153
    #16 0x7f4aaff85ee1 in ssl_SecureSend firefox/beta-src/security/nss/lib/ssl/sslsecur.c:1240
    #17 0x7f4aaff91a9f in ssl_Send firefox/beta-src/security/nss/lib/ssl/sslsock.c:1770
    #18 0x7f4aa2985124 in PSMSend firefox/beta-src/security/manager/ssl/src/nsNSSIOLayer.cpp:2257
    #19 0x7f4aa298721a in nsSSLIOLayerWrite firefox/beta-src/security/manager/ssl/src/nsNSSIOLayer.cpp:2275
    #20 0x7f4ab1fe4aef in PR_Write firefox/beta-src/nsprpub/pr/src/io/priometh.c:146
    #21 0x7f4a995291bc in nsSocketOutputStream::Write(char const*, unsigned int, unsigned int*) firefox/beta-src/netwerk/base/src/nsSocketTransport2.cpp:586
    #22 0x7f4a99aa4309 in nsHttpConnection::OnReadSegment(char const*, unsigned int, unsigned int*) firefox/beta-src/netwerk/protocol/http/nsHttpConnection.cpp:937
    #23 0x7f4a99b5a5a7 in nsHttpTransaction::ReadRequestSegment(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) firefox/beta-src/netwerk/protocol/http/nsHttpTransaction.cpp:470
    #24 0x7f4aa5456881 in nsStringInputStream::ReadSegments(unsigned int (*)(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) firefox/beta-src/xpcom/io/nsStringStream.cpp:261
    #25 0x7f4a99b5b564 in nsHttpTransaction::ReadSegments(nsAHttpSegmentReader*, unsigned int, unsigned int*) firefox/beta-src/netwerk/protocol/http/nsHttpTransaction.cpp:499
    #26 0x7f4a99aa5020 in nsHttpConnection::OnSocketWritable() firefox/beta-src/netwerk/protocol/http/nsHttpConnection.cpp:993
    #27 0x7f4a99aaa7cf in nsHttpConnection::OnOutputStreamReady(nsIAsyncOutputStream*) firefox/beta-src/netwerk/protocol/http/nsHttpConnection.cpp:1227
    #28 0x7f4a99aaac6f in non-virtual thunk to nsHttpConnection::OnOutputStreamReady(nsIAsyncOutputStream*) crmftmpl.c:0
    #29 0x7f4a99527453 in nsSocketOutputStream::OnSocketReady(unsigned int) firefox/beta-src/netwerk/base/src/nsSocketTransport2.cpp:524
    #30 0x7f4a99542a05 in nsSocketTransport::OnSocketReady(PRFileDesc*, short) firefox/beta-src/netwerk/base/src/nsSocketTransport2.cpp:1570
    #31 0x7f4a9956f18b in nsSocketTransportService::DoPollIteration(bool) firefox/beta-src/netwerk/base/src/nsSocketTransportService2.cpp:760
    #32 0x7f4a9956bf40 in nsSocketTransportService::Run() firefox/beta-src/netwerk/base/src/nsSocketTransportService2.cpp:645
    #33 0x7f4a995713ec in non-virtual thunk to nsSocketTransportService::Run() crmftmpl.c:0
    #34 0x7f4aa5552879 in nsThread::ProcessNextEvent(bool, bool*) firefox/beta-src/xpcom/threads/nsThread.cpp:658
    #35 0x7f4aa5188590 in NS_ProcessNextEvent_P(nsIThread*, bool) firefox/beta-src/objdir-ff-asan-2/xpcom/build/nsThreadUtils.cpp:245
    #36 0x7f4aa5549683 in nsThread::ThreadFunc(void*) firefox/beta-src/xpcom/threads/nsThread.cpp:289
    #37 0x7f4ab219feb6 in _pt_root firefox/beta-src/nsprpub/pr/src/pthreads/ptthread.c:187
    #38 0x43219b in __asan::AsanThread::ThreadStart() ??:0
Thread T17 created by T3 here:
    #0 0x42b4f3 in pthread_create ??:0
    #1 0x7f4ab2190516 in _PR_CreateThread firefox/beta-src/nsprpub/pr/src/pthreads/ptthread.c:424
    #2 0x7f4ab218ea9f in PR_CreateThread firefox/beta-src/nsprpub/pr/src/pthreads/ptthread.c:507
    #3 0x7f4aa554bdc8 in nsThread::Init() firefox/beta-src/xpcom/threads/nsThread.cpp:356
    #4 0x7f4aa5568fc6 in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) firefox/beta-src/xpcom/threads/nsThreadManager.cpp:248
    #5 0x7f4aa5574776 in nsThreadPool::PutEvent(nsIRunnable*) firefox/beta-src/xpcom/threads/nsThreadPool.cpp:114
    #6 0x7f4aa5578b82 in nsThreadPool::Dispatch(nsIRunnable*, unsigned int) firefox/beta-src/xpcom/threads/nsThreadPool.cpp:253
    #7 0x7f4aa29bc67b in mozilla::psm::(anonymous namespace)::SSLServerCertVerificationJob::Dispatch(void const*, nsNSSSocketInfo*, CERTCertificateStr*) firefox/beta-src/security/manager/ssl/src/SSLServerCertVerification.cpp:1000
    #8 0x7f4aa29bac0c in mozilla::psm::AuthCertificateHook(void*, PRFileDesc*, int, int) firefox/beta-src/security/manager/ssl/src/SSLServerCertVerification.cpp:1141
    #9 0x7f4aaff69648 in ssl3_HandleCertificate firefox/beta-src/security/nss/lib/ssl/ssl3con.c:8000
    #10 0x7f4aaff650b6 in ssl3_HandleHandshakeMessage firefox/beta-src/security/nss/lib/ssl/ssl3con.c:8734
    #11 0x7f4aaff633ca in ssl3_HandleHandshake firefox/beta-src/security/nss/lib/ssl/ssl3con.c:8858
    #12 0x7f4aaff61fd7 in ssl3_HandleRecord firefox/beta-src/security/nss/lib/ssl/ssl3con.c:9196
    #13 0x7f4aaff71275 in ssl3_GatherCompleteHandshake firefox/beta-src/security/nss/lib/ssl/ssl3gthr.c:240
    #14 0x7f4aaff72cbc in ssl_GatherRecord1stHandshake firefox/beta-src/security/nss/lib/ssl/sslcon.c:1253
    #15 0x7f4aaff82e42 in ssl_Do1stHandshake firefox/beta-src/security/nss/lib/ssl/sslsecur.c:153
    #16 0x7f4aaff85ee1 in ssl_SecureSend firefox/beta-src/security/nss/lib/ssl/sslsecur.c:1240
    #17 0x7f4aaff91a9f in ssl_Send firefox/beta-src/security/nss/lib/ssl/sslsock.c:1770
    #18 0x7f4aa2985124 in PSMSend firefox/beta-src/security/manager/ssl/src/nsNSSIOLayer.cpp:2257
    #19 0x7f4aa298721a in nsSSLIOLayerWrite firefox/beta-src/security/manager/ssl/src/nsNSSIOLayer.cpp:2275
    #20 0x7f4ab1fe4aef in PR_Write firefox/beta-src/nsprpub/pr/src/io/priometh.c:146
    #21 0x7f4a995291bc in nsSocketOutputStream::Write(char const*, unsigned int, unsigned int*) firefox/beta-src/netwerk/base/src/nsSocketTransport2.cpp:586
    #22 0x7f4a99aa4309 in nsHttpConnection::OnReadSegment(char const*, unsigned int, unsigned int*) firefox/beta-src/netwerk/protocol/http/nsHttpConnection.cpp:937
    #23 0x7f4a99b5a5a7 in nsHttpTransaction::ReadRequestSegment(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) firefox/beta-src/netwerk/protocol/http/nsHttpTransaction.cpp:470
    #24 0x7f4aa5456881 in nsStringInputStream::ReadSegments(unsigned int (*)(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) firefox/beta-src/xpcom/io/nsStringStream.cpp:261
    #25 0x7f4a99b5b564 in nsHttpTransaction::ReadSegments(nsAHttpSegmentReader*, unsigned int, unsigned int*) firefox/beta-src/netwerk/protocol/http/nsHttpTransaction.cpp:499
    #26 0x7f4a99aa5020 in nsHttpConnection::OnSocketWritable() firefox/beta-src/netwerk/protocol/http/nsHttpConnection.cpp:993
    #27 0x7f4a99aaa7cf in nsHttpConnection::OnOutputStreamReady(nsIAsyncOutputStream*) firefox/beta-src/netwerk/protocol/http/nsHttpConnection.cpp:1227
    #28 0x7f4a99aaac6f in non-virtual thunk to nsHttpConnection::OnOutputStreamReady(nsIAsyncOutputStream*) crmftmpl.c:0
    #29 0x7f4a99527453 in nsSocketOutputStream::OnSocketReady(unsigned int) firefox/beta-src/netwerk/base/src/nsSocketTransport2.cpp:524
    #30 0x7f4a99542a05 in nsSocketTransport::OnSocketReady(PRFileDesc*, short) firefox/beta-src/netwerk/base/src/nsSocketTransport2.cpp:1570
    #31 0x7f4a9956f18b in nsSocketTransportService::DoPollIteration(bool) firefox/beta-src/netwerk/base/src/nsSocketTransportService2.cpp:760
    #32 0x7f4a9956bf40 in nsSocketTransportService::Run() firefox/beta-src/netwerk/base/src/nsSocketTransportService2.cpp:645
    #33 0x7f4a995713ec in non-virtual thunk to nsSocketTransportService::Run() crmftmpl.c:0
    #34 0x7f4aa5552879 in nsThread::ProcessNextEvent(bool, bool*) firefox/beta-src/xpcom/threads/nsThread.cpp:658
    #35 0x7f4aa5188590 in NS_ProcessNextEvent_P(nsIThread*, bool) firefox/beta-src/objdir-ff-asan-2/xpcom/build/nsThreadUtils.cpp:245
    #36 0x7f4aa5549683 in nsThread::ThreadFunc(void*) firefox/beta-src/xpcom/threads/nsThread.cpp:289
    #37 0x7f4ab219feb6 in _pt_root firefox/beta-src/nsprpub/pr/src/pthreads/ptthread.c:187
    #38 0x43219b in __asan::AsanThread::ThreadStart() ??:0
==14919== ABORTING
Stats: 240M malloced (254M for red zones) by 550129 calls
Stats: 17M realloced by 27720 calls
Stats: 196M freed by 373556 calls
Stats: 61M really freed by 150761 calls
Stats: 440M (112703 full pages) mmaped in 110 calls
  mmaps   by size class: 8:311277; 9:65528; 10:24570; 11:28658; 12:5120; 13:5632; 14:1792; 15:512; 16:704; 17:224; 18:224; 19:16; 20:8;
  mallocs by size class: 8:398722; 9:72268; 10:29852; 11:34376; 12:5369; 13:5586; 14:2066; 15:635; 16:787; 17:226; 18:215; 19:18; 20:9;
  frees   by size class: 8:245398; 9:60590; 10:24604; 11:31354; 12:4229; 13:3960; 14:1812; 15:565; 16:658; 17:213; 18:153; 19:14; 20:6;
  rfrees  by size class: 8:109141; 9:22003; 10:7090; 11:10168; 12:662; 13:503; 14:542; 15:152; 16:378; 17:112; 18:6; 19:2; 20:2;
Stats: malloc large: 468 small slow: 3222
Shadow byte and word:
  0x1fe94f732b72: fd
  0x1fe94f732b70: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1fe94f732b50: fd fd fd fd fd fd fd fd
  0x1fe94f732b58: fd fd fd fd fd fd fd fd
  0x1fe94f732b60: fa fa fa fa fa fa fa fa
  0x1fe94f732b68: fa fa fa fa fa fa fa fa
=>0x1fe94f732b70: fd fd fd fd fd fd fd fd
  0x1fe94f732b78: fd fd fd fd fd fd fd fd
  0x1fe94f732b80: fa fa fa fa fa fa fa fa
  0x1fe94f732b88: fa fa fa fa fa fa fa fa
  0x1fe94f732b90: fd fd fd fd fd fd fd fd
Assignee: nobody → nobody
Component: Untriaged → Libraries
Product: Firefox → NSS
QA Contact: untriaged → libraries
Version: 12 Branch → unspecified
(Reporter)

Comment 1

7 years ago
Forgot to add the revision information. Retested and can still reproduce with Beta tip of tree - 0ba53003ae26.
Whiteboard: [asan]
(In reply to Abhishek Arya from comment #0)
> Steps to reproduce:
> 
> 1. Make sure that nss security lib is compiled with ASAN. This can be done
> in security/coreconf/Linux.mk, line OS_LIBS                 = $(OS_PTHREAD)
> -ldl -lc -faddress-sanitizer

Why are you making this change? NSS is compiled already with -faddress-sanitizer without any changes. Or did you backport something incomplete to beta (because asan isnt supported there yet without patching)?

Does this bug reproduce with aurora or tip?
(Reporter)

Comment 3

7 years ago
(In reply to Christian Holler (:decoder) from comment #2)
> (In reply to Abhishek Arya from comment #0)
> > Steps to reproduce:
> > 
> > 1. Make sure that nss security lib is compiled with ASAN. This can be done
> > in security/coreconf/Linux.mk, line OS_LIBS                 = $(OS_PTHREAD)
> > -ldl -lc -faddress-sanitizer
> 
> Why are you making this change? NSS is compiled already with
> -faddress-sanitizer without any changes. Or did you backport something
> incomplete to beta (because asan isnt supported there yet without patching)?

I didn't know that the patch wasn't required anymore.

> 
> Does this bug reproduce with aurora or tip?

No. It does not reproduce on aurora.
(In reply to Abhishek Arya from comment #3)

> I didn't know that the patch wasn't required anymore.

Where did you get that patch from? On beta you still need to apply the patch from bug 727445 because that landed later (it reached aurora now).
 
> No. It does not reproduce on aurora.

Maybe it has already been fixed. Someone familiar with the area of code might be able to tell.
(Reporter)

Comment 5

7 years ago
I was able to compile beta by adding -faddress-sanitizer in security/coreconf/Linux.mk and having wrapper around clang to remove -z,defs. Good to know that these hacks are not needed anymore.
(Assignee)

Comment 6

7 years ago
At the time of writing the above comments:

- Firefox beta == Firefox 12 -- uses NSS 3.13.3
  => has bug

- Firefox aurora == Firefox 13 -- uses NSS 3.13.4
  => no bug

The changelog for 3.13.4 can be found in the initial comment of bug 741135.

The code in security/nss/lib/base/utf8.c hasn't changed since 2005.
(Assignee)

Comment 7

7 years ago
This looks like a race between two threads:

==14919== ERROR: AddressSanitizer heap-use-after-free on address 0x7f4a7b995b90 at pc 0x42d1f0 bp 0x7f4a6d5190b0 sp 0x7f4a6d519098
READ of size 1 at 0x7f4a7b995b90 thread T15

and

0x7f4a7b995b90 is located 16 bytes inside of 54-byte region [0x7f4a7b995b80,0x7f4a7b995bb6)
freed by thread T17 here:
(Assignee)

Comment 8

7 years ago
T15 (use after free):

    #5 0x7f4ab0250714 in nssTrustDomain_AddCertsToCache firefox/beta-src/security/nss/lib/pki/tdcache.c:880
    #6 0x7f4ab01f5e4b in PK11_ImportCert firefox/beta-src/security/nss/lib/pk11wrap/pk11cert.c:1015


T17 (freed):

    #3 0x7f4ab0253470 in nssPKIObject_AddInstance firefox/beta-src/security/nss/lib/pki/pkibase.c:225
    #4 0x7f4ab01f5e2d in PK11_ImportCert firefox/beta-src/security/nss/lib/pk11wrap/pk11cert.c:1014


the affected lines are:

==============
PK11_ImportCert(PK11SlotInfo *slot, CERTCertificate *cert, 
		CK_OBJECT_HANDLE key, const char *nickname, 
                PRBool includeTrust) 
{
 ...
       /* add the new instance to the cert, force an update of the
        * CERTCertificate, and finish
        */
T17    nssPKIObject_AddInstance(&c->object, certobj);
T15    nssTrustDomain_AddCertsToCache(STAN_GetDefaultTrustDomain(), &c, 1);
       (void)STAN_ForceCERTCertificateUpdate(c);
       SECITEM_FreeItem(keyID,PR_TRUE);
       return SECSuccess;
==============


I don't know this code, but it seems like two threads are concurrently dealing with the same certificate object.

It seems an appropriate lock should be used, that ensures atomary execution of this sequence of calls.
(Assignee)

Comment 9

7 years ago
Function nssPKIObject_AddInstance uses:
  nssPKIObject_Lock(NSSCertificate.object);


nssTrustDomain_AddCertsToCache calls
  add_cert_to_cache(..., NSSCertificate);
which uses the cert's nickname,
which was obtained using nssCertificate_GetNickname(NSSCertificate).
which calls nssPKIObject_GetNicknameForToken.

nssPKIObject_GetNicknameForToken uses locking to obtain the pointer to the nickname,
but it returns the pointer to the internal string -- it doesn't return a copy.

In fact nssPKIObject_GetNicknameForToken contains the following comment:
  /* XXX should be copy? safe as long as caller has reference */

I don't know what reference this code refers to, a reference to the nssPKIObject?
I'm not sure if that's sufficient.
The wrapping NSSCertificate object probably has a reference to the nssPKIObject,
but still replaces the destination of the object in place.

I propose that we change nssPKIObject_GetNicknameForToken to always return
a copy of the string,
which would mean that we must change all callers to free the string after use.
(Assignee)

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 10

7 years ago
There is only one place that calls nssPKIObject_GetNicknameForToken,
which is nssCertificate_GetNickname.

We'd have to change each caller of nssCertificate_GetNickname
to free the result string.
(Assignee)

Updated

7 years ago
Assignee: nobody → kaie
(Assignee)

Comment 11

7 years ago
Created attachment 616587 [details] [diff] [review]
Patch v1


Would you like to test this potential fix?
(Assignee)

Updated

7 years ago
Summary: Heap-use-after-free in nssUTF8_Duplicate → Nickname race in PK11_ImportCert (potential heap-use-after-free in nssUTF8_Duplicate)
(Assignee)

Comment 12

7 years ago
I suspect this race has been present since the removal of the SSL thread (which was done for Firefox 11).
(Reporter)

Comment 13

7 years ago
I will test the fix tonight. In webkit contributors meeting.
(Assignee)

Comment 14

7 years ago
Comment on attachment 616587 [details] [diff] [review]
Patch v1

This patch crashes the test suite.

Probably too aggressive memory cleanup...
Attachment #616587 - Flags: review-
(Assignee)

Comment 15

7 years ago
Created attachment 616753 [details] [diff] [review]
Patch v2

This patch passes the test suite, no crashing.

Can you please test this patch in your environment?
Attachment #616587 - Attachment is obsolete: true

Comment 16

7 years ago
Created attachment 616769 [details] [diff] [review]
Replace the label only if it's different

This patch should be good enough for the way Mozilla calls
PK11_ImportCert from multiple worker threads.  But I agree
with Kai's approach as the proper solution.  This patch
may still be helpful independently.
(Reporter)

Comment 17

7 years ago
I tested with Kai's patch in c#15 on the beta branch and it looks to completely stop the crash. I tested with my script 7 times with clean profiles and 17 simultaneous firefox instances. Before patch, it used to reliably crash 2-3 instances in every script run.
(Reporter)

Comment 18

7 years ago
Wan-Teh, is chromium side implementation safe ? Do we need to uptake this or similar patch in our codebase ?

Comment 19

7 years ago
Abhishek: No, Chromium doesn't call PK11_ImportCert this way.
Comment on attachment 616753 [details] [diff] [review]
Patch v2

This is a good fix to have for Firefox 13 and later. I think Bob is the best reviewer for these patches. I have looked at them but as I don't understand the certificate database code very well, it is difficult for me to r+ them with much confidence.
Attachment #616753 - Flags: review?(rrelyea)
If we want these fixed for the upcoming Firefox releases then we'll have to go with Kai's NSS release plan with the addition of this patch.
status-firefox-esr10: --- → unaffected
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox12: --- → ?
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?
tracking-firefox15: --- → ?
(In reply to Brian Smith (:bsmith) from comment #21)
> If we want these fixed for the upcoming Firefox releases then we'll have to
> go with Kai's NSS release plan with the addition of this patch.

Would you mind providing more context here? It's not clear to me what the user impact is or what the security rating is (critical?).
(Assignee)

Updated

7 years ago
status-firefox-esr10: unaffected → ---
status-firefox11: affected → ---
status-firefox12: affected → ---
status-firefox13: affected → ---
status-firefox14: affected → ---
status-firefox15: affected → ---
tracking-firefox12: ? → ---
tracking-firefox13: ? → ---
tracking-firefox14: ? → ---
tracking-firefox15: ? → ---
Version: unspecified → 3.13.4
(Assignee)

Comment 24

7 years ago
Comment on attachment 616769 [details] [diff] [review]
Replace the label only if it's different

I think we should only consider to take this (Wan-Teh's) patch, 
if Abhishek Arya is able to test this patch and gives positive testing feedback.

However, I'm not sure we should spend any time on testing or reviewing this,
given that even Wan-Teh says that my patch seems like the better approach?
(Assignee)

Comment 25

7 years ago
(In reply to Alex Keybl [:akeybl] from comment #23)
> 
> Would you mind providing more context here? It's not clear to me what the
> user impact is or what the security rating is (critical?).

If I understand correctly,
any user using https (everybody) is at risk in running into this crash.
(Reporter)

Comment 26

7 years ago
I just tried Wan-Teh's patch. That fixes the crash too. I have tried 10 times running the script with 22 threads, never got the crash. Removing the patch causes the crash on atleast one thread in every script run.
(In reply to Kai Engert (:kaie) from comment #25)
> If I understand correctly,
> any user using https (everybody) is at risk in running into this crash.

What would the associated crash signature be? Unless this is a significant security concern, we'd need to see crash data before landing a patch on branches.
tracking-firefox12: --- → ?
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?
(Assignee)

Comment 28

7 years ago
> What would the associated crash signature be? Unless this is a significant
> security concern, we'd need to see crash data before landing a patch on
> branches.

My statement was too simplistic.

The risk is a "use of memory after it has been freed".

In the worst case, after freeing the memory, and before incorrectly writing to the freed memory, the memory area could have already been reassigned to some other variable - and this could lead to all sorts of data corruption and random behaviour.

I can't predict what a stack crash signature would look like. This bug might be a cause for that class of crash reports that appear to make no sense.
(In reply to Alex Keybl [:akeybl] from comment #27)

> What would the associated crash signature be? Unless this is a significant
> security concern, we'd need to see crash data before landing a patch on
> branches.

Per comment 28, this seems sg:critical and should be landed on branches for that reason.
Whiteboard: [asan] → [asan][sg:critical]
(Assignee)

Comment 30

7 years ago
It would be interesting to know if Firefox 10 is affected, too.

Updated

7 years ago
tracking-firefox12: ? → +
tracking-firefox13: ? → +
tracking-firefox14: ? → +
(Assignee)

Comment 31

7 years ago
Comment on attachment 616769 [details] [diff] [review]
Replace the label only if it's different

In today's NSS conference call Wan-Teh said we should mark his patch as obsolete and use the other patch.
Attachment #616769 - Attachment is obsolete: true
Attachment #616769 - Flags: review?(rrelyea)
(Assignee)

Comment 32

7 years ago
Comment on attachment 616753 [details] [diff] [review]
Patch v2

I'll wait for Bob's comments on this patch.

During the conference call there has been the concern that the memory allocation might happen very frequently. I don't know how frequently that happens. 

There was the idea to potentially introduce a new function "is there already a nickname", which would avoid the need of having to allocate and return the nickname string.

We decided to wait for Bob to make a decision, whether we need such a new function.
The relevant PSM code:

      if (node->cert->isperm) {
        // We don't need to remember certs already stored in perm db.
        continue;
      }
        
      ...

      char* nickname = nsNSSCertificate::defaultServerNickname(node->cert);
      if (nickname && *nickname) {
        PK11SlotInfo *slot = PK11_GetInternalKeySlot();
        if (slot) {
          PK11_ImportCert(slot, node->cert, CK_INVALID_HANDLE, 
                          nickname, false);
          PK11_FreeSlot(slot);
        }
      }
      PR_FREEIF(nickname);

So:

1. It does seem to check (using isperm) whether the cert is already in the database. This makes me think that the issue should be relatively rare.

2. defaultServerNickname checks for conflicting nicknames in the database, and tries to generate a unique nickname using a counter. However, this isn't an atomic operation, so it is possible/likely that two concurrent threads will pick the same nickname, though AFAICT, it is unlikely that they will even get to this point because of the check done in #1 above.
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox15: --- → +
(Assignee)

Updated

7 years ago
Blocks: 738458
Target Milestone: --- → 3.13.5

Comment 34

7 years ago
Comment on attachment 616753 [details] [diff] [review]
Patch v2

r-

The patch is mostly complete, but there are a few problems:



> /* Either there was no nickname yet, or we have a new nickname */
> stanNick = nssUTF8_Duplicate((NSSUTF8 *)nickname, c->object.arena);

This code is old code, but it allocates stanNick out of the c->object.arena (which is freed when c->object goes away). It's not clear why the existing code copied this value since nsstoken_ImportCertificate() doesn't adopt the value (it makes it's own copy). In any case we can just pass NULL in for the arena and get the proper semantics for the new code.

In add_cert_to_cache ()  (lib/pki/tdcache.c).

It looks like you are doing a bunch of work assuming that add_subject_entry and/or add_nickname_entry adopts the nickname. This is not the case. They both copy it to their own arena. You can dispense with the Boolean and just always free the nickname on exit.
Attachment #616753 - Flags: review?(rrelyea) → review-
(Assignee)

Comment 35

7 years ago
> > /* Either there was no nickname yet, or we have a new nickname */
> > stanNick = nssUTF8_Duplicate((NSSUTF8 *)nickname, c->object.arena);
> 
> This code is old code, but it allocates stanNick out of the c->object.arena
> (which is freed when c->object goes away). It's not clear why the existing
> code copied this value since nsstoken_ImportCertificate() doesn't adopt the
> value (it makes it's own copy). In any case we can just pass NULL in for the
> arena and get the proper semantics for the new code.


Ok, I will change
    stanNick = nssUTF8_Duplicate((NSSUTF8 *)nickname, c->object.arena);
to
    stanNick = nssUTF8_Duplicate((NSSUTF8 *)nickname, NULL);


> In add_cert_to_cache ()  (lib/pki/tdcache.c).
> 
> It looks like you are doing a bunch of work assuming that add_subject_entry
> and/or add_nickname_entry adopts the nickname. This is not the case. They
> both copy it to their own arena. You can dispense with the Boolean and just
> always free the nickname on exit.


That's what I did in my initial testing (prior to the attached patch), but it had crashed.

I tested again, with both changes included, and it still crashes the test suite:

merge.sh: #1346: Merging conflicting nicknames 1  - Core file is detected - FAILED
merge.sh: #1347: Merging conflicting nicknames 2  - Core file is detected - FAILED
merge.sh: #1348: Verify nicknames were deconflicted (Alice #4)  - FAILED
merge.sh: #1349: Verify nicknames were deconflicted (Alice #100)  - FAILED


Using add_cert_to_cache from patch v2: no crash

Comment 36

7 years ago
Ah... you probably need to free the nickname at the top of the loser: label.

The call

if (added>=2) {
    (void) remove_subject_entry(tc->cache,...., &certNickname, &arena)
}

Will trash your copy of the nickname, so your free will crash.

Also, you need to free the nickname in the success path as well.

bob
status-firefox-esr10: --- → ?
tracking-firefox-esr10: --- → ?
bsmith initially marked ESR as unaffected, and then kai seems to have cleared the flags - i'm assuming accidentally - so am resetting flag to unaffected for ESR, please comment/change if that is not the case here.
status-firefox-esr10: ? → unaffected
tracking-firefox-esr10: ? → ---
(Assignee)

Comment 38

6 years ago
Created attachment 624869 [details] [diff] [review]
Patch v4

sorry for the delay.

Bob, thanks for your comment 36, that trashing wasn't obvious to me.

This patch v4 passes all tests on trunk.
Attachment #616753 - Attachment is obsolete: true
Attachment #624869 - Flags: review?(rrelyea)

Comment 39

6 years ago
Comment on attachment 624869 [details] [diff] [review]
Patch v4

r+ rrelyea
Attachment #624869 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 40

6 years ago
Checkin comment:
================

Bug 745548, nssPKIObject_GetNicknameForToken should always return a copy, r=rrelyea


Checked in to trunk:
====================

Checking in certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.92; previous revision: 1.91
done
Checking in certhigh/certhigh.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certhigh.c,v  <--  certhigh.c
new revision: 1.47; previous revision: 1.46
done
Checking in pki/certificate.c;
/cvsroot/mozilla/security/nss/lib/pki/certificate.c,v  <--  certificate.c
new revision: 1.70; previous revision: 1.69
done
Checking in pki/pki.h;
/cvsroot/mozilla/security/nss/lib/pki/pki.h,v  <--  pki.h
new revision: 1.15; previous revision: 1.14
done
Checking in pki/pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.108; previous revision: 1.107
done
Checking in pki/pkibase.c;
/cvsroot/mozilla/security/nss/lib/pki/pkibase.c,v  <--  pkibase.c
new revision: 1.35; previous revision: 1.34
done
Checking in pki/pkistore.c;
/cvsroot/mozilla/security/nss/lib/pki/pkistore.c,v  <--  pkistore.c
new revision: 1.36; previous revision: 1.35
done
Checking in pki/tdcache.c;
/cvsroot/mozilla/security/nss/lib/pki/tdcache.c,v  <--  tdcache.c
new revision: 1.51; previous revision: 1.50
done


Checked in to NSS_3_13_4_BRANCH for 3.13.5:
===========================================

Checking in certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.90.2.1; previous revision: 1.90
done
Checking in certhigh/certhigh.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certhigh.c,v  <--  certhigh.c
new revision: 1.45.2.1; previous revision: 1.45
done
Checking in pki/certificate.c;
/cvsroot/mozilla/security/nss/lib/pki/certificate.c,v  <--  certificate.c
new revision: 1.68.2.1; previous revision: 1.68
done
Checking in pki/pki.h;
/cvsroot/mozilla/security/nss/lib/pki/pki.h,v  <--  pki.h
new revision: 1.13.196.1; previous revision: 1.13
done
Checking in pki/pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.106.2.1; previous revision: 1.106
done
Checking in pki/pkibase.c;
/cvsroot/mozilla/security/nss/lib/pki/pkibase.c,v  <--  pkibase.c
new revision: 1.33.6.1; previous revision: 1.33
done
Checking in pki/pkistore.c;
/cvsroot/mozilla/security/nss/lib/pki/pkistore.c,v  <--  pkistore.c
new revision: 1.34.2.1; previous revision: 1.34
done
Checking in pki/tdcache.c;
/cvsroot/mozilla/security/nss/lib/pki/tdcache.c,v  <--  tdcache.c
new revision: 1.49.6.1; previous revision: 1.49
done
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Not sure if this was fixed in Fx13 or 14 but either way it's fixed now.
status-firefox-esr10: unaffected → fixed
status-firefox14: affected → fixed
status-firefox15: affected → fixed
tracking-firefox-esr10: --- → 14+
Is this something we can put in-testsuite? or does it need manual verification?
Whiteboard: [asan][sg:critical] → [asan][sg:critical][qa?]
Group: core-security
Keywords: csec-uaf
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.