Closed
Bug 298906
Opened 19 years ago
Closed 19 years ago
crash when accepting new certificate permanently on taschenonkel.de
Categories
(NSS :: Libraries, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11
People
(Reporter: chpe, Assigned: wtc)
References
()
Details
Attachments
(2 files, 4 obsolete files)
648 bytes,
patch
|
nelson
:
superreview-
|
Details | Diff | Splinter Review |
3.69 KB,
patch
|
wtc
:
review+
nelson
:
superreview+
|
Details | Diff | Splinter Review |
(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
Reporter | ||
Comment 1•19 years ago
|
||
*** Bug 298891 has been marked as a duplicate of this bug. ***
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
Assignee | ||
Comment 3•19 years ago
|
||
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•19 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?
Reporter | ||
Comment 5•19 years ago
|
||
(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!
Reporter | ||
Comment 6•19 years ago
|
||
(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•19 years ago
|
||
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•19 years ago
|
||
Woah, I am impressed how quickly you guys picked up this issue. Thumbs up!
Assignee | ||
Comment 9•19 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)
Comment 10•19 years ago
|
||
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•19 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•19 years ago
|
||
This patch compiles, but is untested.
Comment 13•19 years ago
|
||
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•19 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.
Comment 15•19 years ago
|
||
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•19 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•19 years ago
|
Attachment #187407 -
Flags: review?(rrelyea)
Comment 17•19 years ago
|
||
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•19 years ago
|
||
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 19•19 years ago
|
||
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 20•19 years ago
|
||
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•19 years ago
|
||
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 22•19 years ago
|
||
Comment on attachment 190982 [details] [diff] [review] Try this one ... Looks good to me. Thanks Bob.
Attachment #190982 -
Flags: superreview?(nelson) → superreview+
Assignee | ||
Updated•19 years ago
|
Attachment #187738 -
Attachment is obsolete: true
Assignee | ||
Comment 23•19 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•19 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
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.11
Assignee | ||
Comment 25•19 years ago
|
||
*** Bug 320454 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 26•19 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•19 years ago
|
||
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•19 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•19 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.
Description
•