298906 - crash when accepting new certificate permanently on taschenonkel.de

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

(Forwarded from http://bugzilla.gnome.org/show_bug.cgi?id=309123)

Steps to reproduce:
0) Load https://taschenorakel.de/
1) In the cert dialogue, choose "Accept this certificate permanantly" and click OK
2) Crash!

Firefox debug build from cvs 2005-06-26 (but also reproducible with ff 1.0.x).

#0  0xb4d03c33 in PORT_ArenaAlloc (arena=0x0, size=855) at secport.c:230
#1  0xb4ca7fc8 in nsslowcert_KeyFromIssuerAndSN (arena=0x0, issuer=0x89caa2c,
sn=0x89caa44, key=0x89caa6c) at lowcert.c:377
#2  0xb4ca8165 in nsslowcert_DecodeDERCertificate (derSignedCert=0xb4a44090,
nickname=0x8b28188 "www.fuxbaukind.de") at lowcert.c:435
#3  0xb4cb8266 in sftk_handleCertObject (session=0x8bb6d08, object=0x89e3b98) at
pkcs11.c:678
#4  0xb4cbaa8b in sftk_handleObject (object=0x89e3b98, session=0x8bb6d08) at
pkcs11.c:1795
#5  0xb4cbe448 in NSC_CreateObject (hSession=16777218, pTemplate=0xb4a441d0,
ulCount=10, phObject=0xb4a44164) at pkcs11.c:3691
#6  0xb4da0ad2 in import_object (tok=0x8843980, sessionOpt=0x0,
objectTemplate=0xb4a441d0, otsize=10) at devtoken.c:333
#7  0xb4da169f in nssToken_ImportCertificate (tok=0x8843980, sessionOpt=0x0,
certType=NSSCertificateType_PKIX, id=0x8a88794, nickname=0x8a1e8a0
"www.fuxbaukind.de", encoding=0x8a8879c, issuer=0x8a887a4, 
    subject=0x8a887ac, serial=0x8a887b4, email=0x8b91dd8
"[e-mail]", asTokenObject=1) at devtoken.c:653
#8  0xb4d7fe19 in __CERT_AddTempCertToPerm (cert=0x89a4608, nickname=0x88e5e40
"www.fuxbaukind.de", trust=0xb4a44300) at stanpcertdb.c:180
#9  0xb4c05152 in addCertToDB (peerCert=0x89a4608, addType=2) at
/opt/source/firefox-trunk/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp:1218
#10 0xb4c05656 in nsContinueDespiteCertError (infoObject=0x89f0640,
sslSocket=0x89b1c60, error=-8172, nssCert=0x8aeefd0) at
/opt/source/firefox-trunk/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp:1351
#11 0xb4c06686 in nsNSSBadCertHandler (arg=0x89f0640, sslSocket=0x89b1c60) at
/opt/source/firefox-trunk/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp:2197
#12 0xb4c79638 in ssl3_HandleCertificate (ss=0x8af6e90, b=0x8bd65b7 "",
length=0) at ssl3con.c:7515
#13 0xb4c7ab98 in ssl3_HandleHandshakeMessage (ss=0x8af6e90, b=0x8bd59dc "",
length=3035) at ssl3con.c:8147
#14 0xb4c7afaa in ssl3_HandleHandshake (ss=0x8af6e90, origBuf=0x8af7034) at
ssl3con.c:8264
#15 0xb4c7b893 in ssl3_HandleRecord (ss=0x8af6e90, cText=0xb4a44660,
databuf=0x8af7034) at ssl3con.c:8532
#16 0xb4c7c9bb in ssl3_GatherCompleteHandshake (ss=0x8af6e90, flags=0) at
ssl3gthr.c:206
#17 0xb4c7f345 in ssl_GatherRecord1stHandshake (ss=0x8af6e90) at sslcon.c:1260
#18 0xb4c86b4c in ssl_Do1stHandshake (ss=0x8af6e90) at sslsecur.c:149
#19 0xb4c8867a in ssl_SecureSend (ss=0x8af6e90, 
    buf=0x8b93920 "GET / HTTP/1.1\r\nHost: taschenorakel.de\r\nUser-Agent:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050626
Firefox/1.0+\r\nAccept:
text/xml,application/xml,application/xhtml+xml,text/html;q=0."..., len=395,
flags=0) at sslsecur.c:1038
#20 0xb4c887b0 in ssl_SecureWrite (ss=0x8af6e90, 
    buf=0x8b93920 "GET / HTTP/1.1\r\nHost: taschenorakel.de\r\nUser-Agent:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050626
Firefox/1.0+\r\nAccept:
text/xml,application/xml,application/xhtml+xml,text/html;q=0."..., len=395) at
sslsecur.c:1072
#21 0xb4c8e092 in ssl_Write (fd=0x89b1c60, buf=0x8b93920, len=395) at sslsock.c:1310
#22 0xb4c04ed9 in nsSSLIOLayerWrite (fd=0x8327fb0, buf=0x8b93920, amount=395) at
/opt/source/firefox-trunk/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp:1146
#23 0xb7d61918 in PR_Write (fd=0x8327fb0, buf=0x8b93920, amount=395) at
/opt/source/firefox-trunk/mozilla/nsprpub/pr/src/io/priometh.c:146
#24 0xb7030a0c in nsSocketOutputStream::Write (this=0x8b3debc, 
    buf=0x8b93920 "GET / HTTP/1.1\r\nHost: taschenorakel.de\r\nUser-Agent:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050626
Firefox/1.0+\r\nAccept:
text/xml,application/xml,application/xhtml+xml,text/html;q=0."..., count=395,
countWritten=0xb4a4497c) at
/opt/source/firefox-trunk/mozilla/netwerk/base/src/nsSocketTransport2.cpp:549
#25 0xb70adfe4 in nsHttpConnection::OnReadSegment (this=0x8b3dcc8, 
    buf=0x8b93920 "GET / HTTP/1.1\r\nHost: taschenorakel.de\r\nUser-Agent:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050626
Firefox/1.0+\r\nAccept:
text/xml,application/xml,application/xhtml+xml,text/html;q=0."..., count=395,
countRead=0xb4a4497c) at
/opt/source/firefox-trunk/mozilla/netwerk/protocol/http/src/nsHttpConnection.cpp:524
#26 0xb70bb808 in nsHttpTransaction::ReadRequestSegment (stream=0x8b93c28,
closure=0x8b93828, 
    buf=0x8b93920 "GET / HTTP/1.1\r\nHost: taschenorakel.de\r\nUser-Agent:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050626
Firefox/1.0+\r\nAccept:
text/xml,application/xml,application/xhtml+xml,text/html;q=0."..., offset=0,
count=395, countRead=0xb4a4497c) at
/opt/source/firefox-trunk/mozilla/netwerk/protocol/http/src/nsHttpTransaction.cpp:354
#27 0xb7e736d2 in nsStringInputStream::ReadSegments (this=0x8b93c28,
writer=0xb70bb7d2 <nsHttpTransaction::ReadRequestSegment(nsIInputStream*, void*,
char const*, unsigned int, unsigned int, unsigned int*)>, 
    closure=0x8b93828, aCount=395, result=0xb4a4497c) at
/opt/source/firefox-trunk/mozilla/xpcom/io/nsStringStream.cpp:239
#28 0xb70bb95b in nsHttpTransaction::ReadSegments (this=0x8b93828,
reader=0x8b3dcc8, count=4096, countRead=0xb4a4497c) at
/opt/source/firefox-trunk/mozilla/netwerk/protocol/http/src/nsHttpTransaction.cpp:379
#29 0xb70ae123 in nsHttpConnection::OnSocketWritable (this=0x8b3dcc8) at
/opt/source/firefox-trunk/mozilla/netwerk/protocol/http/src/nsHttpConnection.cpp:559
#30 0xb70aecd9 in nsHttpConnection::OnOutputStreamReady (this=0x8b3dcc8,
out=0x8b3debc) at
/opt/source/firefox-trunk/mozilla/netwerk/protocol/http/src/nsHttpConnection.cpp:770
#31 0xb703074a in nsSocketOutputStream::OnSocketReady (this=0x8b3debc,
condition=0) at
/opt/source/firefox-trunk/mozilla/netwerk/base/src/nsSocketTransport2.cpp:488
#32 0xb703384f in nsSocketTransport::OnSocketReady (this=0x8b3ddb8,
fd=0x8327fb0, outFlags=2) at
/opt/source/firefox-trunk/mozilla/netwerk/base/src/nsSocketTransport2.cpp:1457
#33 0xb7038259 in nsSocketTransportService::Run (this=0x82538b8) at
/opt/source/firefox-trunk/mozilla/netwerk/base/src/nsSocketTransportService2.cpp:582
#34 0xb7e994ab in nsThread::Main (arg=0x8253fb8) at
/opt/source/firefox-trunk/mozilla/xpcom/threads/nsThread.cpp:118
#35 0xb7d853f0 in _pt_root (arg=0x8254038) at
/opt/source/firefox-trunk/mozilla/nsprpub/pr/src/pthreads/ptthread.c:220
#36 0xb7d34ae0 in start_thread () from /lib/tls/i686/cmov/libpthread.so.0
#37 0xb7695c9a in clone () from /lib/tls/i686/cmov/libc.so.6

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

*** Bug 298891 has been marked as a duplicate of this bug. ***

Comment 2

[timeless]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/secport.c&rev=1.18&mark=220,226,228-230#215
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/lowcert.c&rev=1.18&mark=373,375,377#366

that else branch in nsslowcert_KeyFromIssuerAndSN seems very broken

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch (for NSS_3_10_BRANCH)

Christian, could you test this patch?

timeless, the logic in the else branch does seem broken,
but it isn't.  When NSS calls this function with a NULL
"arena", the "key" SECItem is initialized to point to a
static buffer.	That's why the else branch has a test to
find out whether the static buffer is large enough.

The only caller of this function with a NULL "arena" is
http://lxr.mozilla.org/security/source/security/nss/lib/softoken/lowcert.c#432.

Comment 4

[Wan-Teh Chang]

Question for patch reviewers: another issue is whether the
size of the static buffer (the certKeySpace field in the
NSSLOWCERTCertificateStr structure) should be increased.
It is 512 bytes now.

Mattias, in the debugger, could you set a breakpoint in
nsslowcert_KeyFromIssuerAndSN and print sn->len and issuer->len?

Comment 5

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #3)
> Created an attachment (id=187407) [edit]
> Proposed patch
> 
> Christian, could you test this patch?

This patch fixes the crash for me, thanks!

Comment 6

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #4)
> Question for patch reviewers: another issue is whether the
> size of the static buffer (the certKeySpace field in the
> NSSLOWCERTCertificateStr structure) should be increased.
> It is 512 bytes now.
> 
> Mattias, in the debugger, could you set a breakpoint in
> nsslowcert_KeyFromIssuerAndSN and print sn->len and issuer->len?

Breakpoint 4, nsslowcert_KeyFromIssuerAndSN (arena=0x8b8fd30, issuer=0xb6cadf9c,
sn=0xb6cadf90, key=0xb6cadff0) at lowcert.c:370
370         unsigned int len = sn->len + issuer->len;
Current language:  auto; currently c
(gdb) p sn->len
$5 = 9
(gdb) p issuer->len
$6 = 846

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 187407 [details] [diff] [review]
Proposed patch (for NSS_3_10_BRANCH)

The patch does fix the immediate cause of the crash, so I suppose 
the patch deserves r+,	but the function being patched deserves r-.
I'd MUCH prefer to see nsslowcert_KeyFromIssuerAndSN be rewritten 
to have consistent behavior whether arena is NULL or not.  

When I first read nsslowcert_KeyFromIssuerAndSN, my first thought
was "This code is duplicating SECITEM_AllocItem, and should just
call that function rather than attempting to duplicate that code,
and getting it wrong".	Then I noticed that when arena is NULL,
it behaves more like SECITEM_ReallocItem (should).  Then I groaned.  

Imagine writing a block comment describing this function, in less
than (say) 5 sentences.  Such a comment would be a big if-then-else
statement, describing two algorithms, depending on arena being NULL
or not.

IMO, this function should have one defined algorithm that it uses
whether using an arena or not.	It should either unconditionally
allocate in both paths, or conditionally allocate in both paths.
The arena argument should affect only the source of the allocated 
memory, and not change the overall description of the function's 
behavior, IMO.	 Please fix it.

Comment 8

[Mathias Hasselmann]

Woah, I am impressed how quickly you guys picked up this issue.
Thumbs up!

Comment 9

[Wan-Teh Chang]

Comment on attachment 187407 [details] [diff] [review]
Proposed patch (for NSS_3_10_BRANCH)

Nelson, would you accept this patch for NSS 3.10.1?
We can target the code cleanup you suggested at 3.11.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 9,  we must change the behavior of this function
because the present/historic behavior is just wrong.  So, in some sense,
we must create a new behavioral definition.  I oppose creating TWO new 
behavioral definititions, one for the 3.10 branch and one for the trunk.
Doing so would create a maintenance burden going forward, with subsequent 
patches that affect callerf of this function potentially having two 
different effects, one on the 3.10 branch and another on the trunk.  
It is doubtful that patches would be tested separately on both branch 
and trunk if they appear to apply cleanly to both,

Since this is a crash fix, I would NOT object to cleaning up this function
on BOTH the 3.10 branch and the trunk.

Comment 11

[Robert Relyea]

Sigh, Looks like it was something I did a while back.

The function is supposed to handle allocation from the arena, static, or the
heap, depending on the parameters (several functions in softoken have this).
Usually the functions take a pointer for an arena, or a pointer to space to
allocate.

Anyway the 'allocate space if the static buffer isn't big enough' logic needs to
go somewhere. The lowcert structure doesn't have an arena (an attempt to keep
the memory allocations down in softoken). I'll attach a patch that includes a
utility function that handles the allocation.

Comment 12

[Robert Relyea]

Attachment: New patch which uses 'functions' to better document what is going on.

This patch compiles, but is untested.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 187738 [details] [diff] [review]
New patch which uses 'functions' to better document what is going on.

My objection to this function is that the allocation is unconditional when an 
arena is present, and conditional when an arena is NOT present.  The code 
should be conditional, or unconditional in both cases.	

>+    /*
>+     * if the passed in buffer is not big enough, or doesn't exist,
>+     * allocate a bigger one out of the arena.
>+     */

That comment correctly describes the desired behavior, IMO, (at least for
the case where an arena is present), but is NOT what the patch actually 
implements.  Instead, the patched code unconditionally allocates space from 
the arena, whenever there is an area.  

>     if (arena) {
> 	key->data = (unsigned char*)PORT_ArenaAlloc(arena, len);
>     } else {
>-	if (len > key->len) {
>-	    key->data = (unsigned char*)PORT_ArenaAlloc(arena, len);
>-	}
>+	key->data = pkcs11_allocStaticData(len, space, spaceLen);
>     }

This patch basically takes the length test and the subsequent conditional 
allocation and moves them to a separate function, but preserves the 
fact that the conditional test is applied ONLY when no arena is present.

Here are a couple of alternative implementations for the above code snippet.  
Please pick one, or write one that is equivalent to one of these.

a) conditional in both cases:

      if (!key->data || len > key->len) {
	  key->data = (unsigned char*)( arena 
		       ? PORT_ArenaAlloc(arena, len);
		       : PORT_Alloc(len) );
      }

b) unconditional in both cases:

      key->data = (unsigned char*)( arena 
		       ? PORT_ArenaAlloc(arena, len);
		       : PORT_Alloc(len) );

Comment 14

[Robert Relyea]

The comment is wrong, When I attempted to use the semantic of that comment, I
quickly discovered it was inconsistant with the rest of the usage with softoken.

This function can allocate data in exactly 2 ways:
1) with the arena.
2) static+heap.

There are already helper functions in softoken to handle 1 and 2. using both
arena and static+heap is not allowed. 

If you would like I can add an assert to make that clearer.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 14,
> using both arena and static+heap is not allowed. 
I suspect you meant to write
> using both static+arena and static+heap is not allowed. 
Yes?

Assuming so, then I think what is needed is two functions
that replace the existing nsslowcert_KeyFromIssuerAndSN,
which are: 
nsslowcert_ArenaKeyFromIssuerAndSN
nsslowcert_StaticKeyFromIssuerAndSN

The first would take an arena pointer, the second would not.
To avoid code duplication, both of those new functions could call 
a third function, declared static, perhaps named _KeyFromIssuerAndSN 
which does the code common to the two cases.  Each function would
be preceeded by a block comment, accurately describing its exact 
behavior with respect to allocation.  In all cases, the comments 
must accurately describe the intended code behavior.  

Comment 16

[Robert Relyea]

Yes.

2 functions are possible. You may notice this is a static function and is called
in exactly 2 places in the code. It's rather common in NSS to have 2 allocations
schemes for essentially the same function triggered off it's parameters.

Anyway in that case it seems easier just to have the existing function take the
preallocated space always (assume it's always correct) and have the caller
allocate the space. 

I don't see where either is any clearer than the existing code.

bob

Comment 17

[Robert Relyea]

Attachment: Patch unrelated to bug 298906

I still prefer a single function, but I don't have time to argue religion and I
need to reclaim my tree.

Comment 18

[Robert Relyea]

Attachment: (duplicate of previous patch)

I still prefer a single function, but I don't have time to argue religion and I
need to reclaim my tree.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 190904 [details] [diff] [review]
(duplicate of previous patch)

This patch is an exact duplicate of the previous one, including the description
and the review requests, so I am obsoleting this one and removing the review
requests from it.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 190903 [details] [diff] [review]
Patch unrelated to bug 298906

Bob, this patch modifies files in ckfw. These changes appear unrelated to this
bug report.  This bug is about code in softoken.

Comment 21

[Robert Relyea]

Attachment: Try this one ...

Thanks nelson... trying to do to much at once (at least I didn't attach a
mozilla patch;).

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 190982 [details] [diff] [review]
Try this one ...

Looks good to me.  Thanks Bob.

Comment 23

[Wan-Teh Chang]

Comment on attachment 190982 [details] [diff] [review]
Try this one ...

r=wtc.

One minor problem: in nsslowcert_KeyFromIssuerAndSN,
>+    if (!arena) {
>+	goto loser;
>+    }

We may want to set the error code to SEC_ERROR_INVALID_ARGS
or SEC_ERROR_LIBRARY_FAILURE before "goto loser".

One nit: in the interest of minimizing diffs (because decision makers
often assess the risk of a patch by counting lines), you might want
to change the local variable "data" back to "copy" in
pkcs11_allocStaticData.

Comment 24

[Robert Relyea]

Checking in lowcert.c;
/cvsroot/mozilla/security/nss/lib/softoken/lowcert.c,v  <--  lowcert.c
new revision: 1.19; previous revision: 1.18
done
Checking in pcert.h;
/cvsroot/mozilla/security/nss/lib/softoken/pcert.h,v  <--  pcert.h
new revision: 1.11; previous revision: 1.10
done
Checking in pcertdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/pcertdb.c,v  <--  pcertdb.c
new revision: 1.49.8.2; previous revision: 1.49.8.1
done

Comment 25

[Wan-Teh Chang]

*** Bug 320454 has been marked as a duplicate of this bug. ***

Comment 26

[Wan-Teh Chang]

The fix for this bug is not yet in any Firefox build.

Our best plan is to get the "Prosed patch (for NSS_3_10_BRANCH)"
(attachment 187407 [details] [diff] [review]) into Firefox 1.5.0.1 (via NSS 3.10.3).  Is
there a strong interest?  It is near impossible to get a
non-security fix into a Firefox patch release.

Comment 27

[Michael Hovis]

Attachment: Handles case where NULL is passed as arena, increases certdata struct size

Just throwing this in here - there is a fundamental problem with PORT_ArenaAlloc in that it never checks for a NULL arena.  It's not safe to assume that all future calls into this function will never pass NULL...

If limiting the allocations is necessary, then just increase the size of the certKeySpace in the NSSLOWCERTCertificateStr struct.  Granted, this pushes the problem out until the next larger certificate comes along, but it does work. Not sure if I should submit this for review - but just a thought...

Comment 28

[timeless]

that isn't the way this function works. the api requires a non null arena. and nspr/nss and friends are reasonable in expecting valid input to certain kinds of functions.

Comment 29

[Wan-Teh Chang]

Comment on attachment 206088 [details] [diff] [review]
Handles case where NULL is passed as arena, increases certdata struct size

Michael, thanks for submitting a good bug report
and suggesting this fix.  We already have fixes
for this bug.  The only question is whether there
is a strong interest in going through the approval
process to get a fix into Firefox 1.5.0.1.  As I
mentioned yesterday, it is very hard to get a
non-security bug fix into a Firefox patch release.