crash when accepting new certificate permanently on taschenonkel.de

RESOLVED FIXED in 3.11

Status

--
critical
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: chpe, Assigned: wtc)

Tracking

unspecified
3.11
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(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
"webmaster@taschenorakel.de", 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
*** Bug 298891 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 3

14 years ago
Created attachment 187407 [details] [diff] [review]
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.
Attachment #187407 - Flags: superreview?(nelson)
Attachment #187407 - Flags: review?(rrelyea)
(Assignee)

Comment 4

14 years ago
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?
(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!
(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 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.
Attachment #187407 - Flags: superreview?(nelson) → superreview-

Comment 8

14 years ago
Woah, I am impressed how quickly you guys picked up this issue.
Thumbs up!
(Assignee)

Comment 9

14 years ago
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.
Attachment #187407 - Attachment description: Proposed patch → Proposed patch (for NSS_3_10_BRANCH)
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

14 years ago
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

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

This patch compiles, but is untested.
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) );
Attachment #187738 - Flags: review-

Comment 14

14 years ago
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.
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

14 years ago
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

Updated

14 years ago
Attachment #187407 - Flags: review?(rrelyea)

Comment 17

13 years ago
Created attachment 190903 [details] [diff] [review]
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.
Attachment #190903 - Flags: superreview?(nelson)
Attachment #190903 - Flags: review?(wtchang)

Comment 18

13 years ago
Created attachment 190904 [details] [diff] [review]
(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.
Attachment #190904 - Flags: superreview?(nelson)
Attachment #190904 - Flags: review?(wtchang)
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.
Attachment #190904 - Attachment description: I prefer the previous patch but I need to clean out my tree. → (duplicate of previous patch)
Attachment #190904 - Attachment is obsolete: true
Attachment #190904 - Flags: superreview?(nelson)
Attachment #190904 - Flags: review?(wtchang)
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.
Attachment #190903 - Attachment description: I prefer the previous patch but I need to clean out my tree. → Patch unrelated to bug 298906
Attachment #190903 - Flags: superreview?(nelson)
Attachment #190903 - Flags: superreview-
Attachment #190903 - Flags: review?(wtchang)

Comment 21

13 years ago
Created attachment 190982 [details] [diff] [review]
Try this one ...

Thanks nelson... trying to do to much at once (at least I didn't attach a
mozilla patch;).
Attachment #190903 - Attachment is obsolete: true
Attachment #190982 - Flags: superreview?(nelson)
Attachment #190982 - Flags: review?(wtchang)
Comment on attachment 190982 [details] [diff] [review]
Try this one ...

Looks good to me.  Thanks Bob.
Attachment #190982 - Flags: superreview?(nelson) → superreview+
(Assignee)

Updated

13 years ago
Attachment #187738 - Attachment is obsolete: true
(Assignee)

Comment 23

13 years ago
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.
Attachment #190982 - Flags: review?(wtchang) → review+

Comment 24

13 years ago
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
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.11
(Assignee)

Comment 25

13 years ago
*** Bug 320454 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 26

13 years ago
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

13 years ago
Created attachment 206088 [details] [diff] [review]
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...
Attachment #206088 - Flags: review?

Comment 28

13 years ago
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.
(Assignee)

Comment 29

13 years ago
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.
Attachment #206088 - Attachment is obsolete: true
Attachment #206088 - Flags: review? → review-
You need to log in before you can comment on or make changes to this bug.