Bugzilla

Reporter

Comment 1

•

12 years ago

Forgot to add the revision information. Retested and can still reproduce with Beta tip of tree - 0ba53003ae26.

Christian Holler (:decoder)

Updated

•

12 years ago

Whiteboard: [asan]

Comment 2

•

12 years ago

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

Christian Holler (:decoder)

Reporter

Comment 3

•

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

Comment 4

•

12 years ago

(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

•

12 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

•

12 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

•

12 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

•

12 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

•

12 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

•

12 years ago

Status: UNCONFIRMED → NEW

Ever confirmed: true

Assignee

Comment 10

•

12 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

•

12 years ago

Assignee: nobody → kaie

Assignee

Comment 11

•

12 years ago

Attached patch Patch v1 (obsolete) — Details — Splinter Review

Would you like to test this potential fix?

Assignee

Updated

•

12 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

•

12 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

•

12 years ago

I will test the fix tonight. In webkit contributors meeting.

Assignee

Comment 14

•

12 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

•

12 years ago

Attached patch Patch v2 (obsolete) — Details — Splinter Review

This patch passes the test suite, no crashing.

Can you please test this patch in your environment?

Attachment #616587 - Attachment is obsolete: true

Wan-Teh Chang

Comment 16

•

12 years ago

Attached patch Replace the label only if it's different (obsolete) — Details — Splinter Review

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

•

12 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

•

12 years ago

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

Wan-Teh Chang

Comment 19

•

12 years ago

Abhishek: No, Chromium doesn't call PK11_ImportCert this way.

Comment 20

•

12 years ago

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)

Updated

•

12 years ago

Attachment #616769 - Flags: review?(rrelyea)

Comment 21

•

12 years ago

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: --- → ?

Alex Keybl [:akeybl]

Comment 23

•

12 years ago

(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

•

12 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

•

12 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

•

12 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

•

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

Alex Keybl [:akeybl]

Comment 27

•

12 years ago

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

Christian Holler (:decoder)

Assignee

Comment 28

•

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

Comment 29

•

12 years ago

(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

•

12 years ago

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

Alex Keybl [:akeybl]

Updated

•

12 years ago

tracking-firefox12: ? → +

tracking-firefox13: ? → +

tracking-firefox14: ? → +

Assignee

Comment 31

•

12 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

•

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

Comment 33

•

12 years ago

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.

Updated

•

12 years ago

status-firefox13: --- → affected

status-firefox14: --- → affected

status-firefox15: --- → affected

tracking-firefox15: --- → +

Assignee

Updated

•

12 years ago

Blocks: 738458

Target Milestone: --- → 3.13.5

Robert Relyea

Comment 34

•

12 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-

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

Assignee

Comment 35

•

12 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

Updated

•

12 years ago

Keywords: sec-critical

Robert Relyea

Comment 36

•

12 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

Lukas Blakk [:lsblakk] use ?needinfo

Updated

•

12 years ago

status-firefox-esr10: --- → ?

tracking-firefox-esr10: --- → ?

Comment 37

•

12 years ago

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

•

12 years ago

Attached patch Patch v4 — Details — Splinter Review

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)

Robert Relyea

Comment 39

•

12 years ago

Comment on attachment 624869 [details] [diff] [review]
Patch v4

r+ rrelyea

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

Assignee

Comment 40

•

12 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

•

12 years ago

Status: NEW → RESOLVED

Closed: 12 years ago

Resolution: --- → FIXED

Comment 41

•

12 years ago

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+

u279076

Comment 42

•

12 years ago

Is this something we can put in-testsuite? or does it need manual verification?

Whiteboard: [asan][sg:critical] → [asan][sg:critical][qa?]

Updated

•

12 years ago

Group: core-security