238914 - M17beta topcrash in [@ secmod_DecodeData]

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[chris hofmann]

Not a lot to go on but this data has shown up in talback reports over the last
few days.

     Count   Offset    Real Signature
[ 27   secmod_DecodeData 2d23143d - secmod_DecodeData ]
 
     Crash date range: 23-MAR-04 to 27-MAR-04
     Min/Max Seconds since last crash: 1 - 29925
     Min/Max Runtime: 38 - 45590
 
     Count   Platform List 
     24   [Windows NT 5.1 build 2600]   
     3   [Windows NT 5.0 build 2195]   
 
     Count   Build Id List 
     27   2004031615
 
     No of Unique Users         4
 
 Stack trace(Frame) 

	 secmod_DecodeData
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/security/nss/lib/softoken/pk11db.c
 line 619] 
	 secmod_ReadPermDB
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/security/nss/lib/softoken/pk11db.c
 line 779] 
	 NSC_ModuleDBFunc
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/security/nss/lib/softoken/pkcs11.c
 line 2672] 
	 SECMOD_GetModuleSpecList
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/security/nss/lib/pk11wrap/pk11pars.c
 line 228] 
	 SECMOD_LoadModule
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/security/nss/lib/pk11wrap/pk11pars.c
 line 326] 
	 nss_Init
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/security/nss/lib/nss/nssinit.c
 line 457] 
	 NSS_InitReadWrite
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/security/nss/lib/nss/nssinit.c
 line 501] 
	 nsNSSComponent::InitializeNSS
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/security/manager/ssl/src/nsNSSComponent.cpp
 line 1118]  
 
     (5692)	URL: www.yahoo.com
     (5666)	URL: www.yahoo.com
     (5633)	URL: www.yahoo.com
     (5217)	URL: www.yahoo.com
     (5073)	URL: www.yahoo.com
     (4818)	URL: www.yahoo.com
     (4818)	Comments: just surfing. mozilla 1.6 and 1.7b really are crashing
very frequently  for me. www.msnbc.com crashes   www.mtv.com/mtvradio crashes. I
dont have the video card that everyone else is having a problem with. Dont know
whats up with this thing

Comment 1

[Wan-Teh Chang]

Bob, could you take a look at this?

Comment 2

[Robert Relyea]

Just looking at the stack traceback, it looks like a corrupted database of some
kind. I'll take a look at secmod_DecodeData and see if it needs to be more
defensive.

bob

Comment 3

[Robert Relyea]

It looks like the slotCount is thrashed in the secmod.db. The forthcoming patch
will be more agressive in protecting itself against invalid databases. Code
changes were made based on code inspection. Testing of the patch was limited to
running all.sh (doesn't seem to break anything new).

I suggest the patch get a couple of reviews, then some testing in the wild
before release into the wild.

The code in question is pretty stable, having been changed only once in the last
year.

Comment 4

[Robert Relyea]

Attachment: Be more paranoid about data we've fetched from the database.

Comment 5

[Robert Relyea]

Besides protecting against bad database data, the patch also fixes a memory
leak, and handles some out of memory cases it wasn't handling before.

bob

Comment 6

[chris hofmann]

Comment on attachment 145061 [details] [diff] [review]
Be more paranoid about data we've fetched from the database.

bob, do you think this one is ready to get checked in?	let me know.  it seems
like  a good one to get in for the next mozilla 1.7 release candidate build
that we want to do in a week or so.

Comment 7

[Robert Relyea]

Comment on attachment 145061 [details] [diff] [review]
Be more paranoid about data we've fetched from the database.

chris,

Let's get a couple of more eyeballs on it. I've added Nelson & wtc to the
review list.

Most of the changes were made based on my review, and though there are no
regressions in all.sh, it would be good to verify that there are no 'off by one
errors'.

Wan-Teh and Nelson, the changes should be easy to review, even if you are
unfamiliar with the database code. The change basically makes sure we are not
going to read beyond the end of datablock returned to us from the DB code. 


BTW this patch can be applied to NSS  3.8, 3.9 or the tip.

Comment 8

[Wan-Teh Chang]

Comment on attachment 145061 [details] [diff] [review]
Be more paranoid about data we've fetched from the database.

r=wtc.	I think this patch is correct.

Some comments.

>+    /* data needs to be at least as long as the old header format */
>+    if (data->size < (int)(((secmodData *)0)->trustOrder)) {

You may be able to use the offsetof macro here:
   offsetof(secmodData, trustOrder[0]).

I suggest that you use the (unsigned int) typecast on both
sides of the comparison to be consistent with how you handle
this compiler warning in the rest of this function.

>-    slotStrings = (char **)PORT_ArenaAlloc(arena, slotCount * sizeof(char *));
>+    slotStrings = (char **)PORT_ArenaZAlloc(arena, slotCount * sizeof(char *));

This change is not necessary.

Global comment: we should verify that there is no integer
overflow.  I am most worried about 'namesStart', which is
unsigned short and gets added with 'len' (also unsigned
short) several times.  Perhaps we should declare 'namesStart'
as unsigned long?  (Isn't that why 'slotCount' is declared
unsigned long even though it is stored as short in the db?)

Comment 9

[Wan-Teh Chang]

Comment on attachment 145061 [details] [diff] [review]
Be more paranoid about data we've fetched from the database.

Bob, another suggestion:

>     /*  slotCount; */
>-    slotStrings = (char **)PORT_ArenaAlloc(arena, slotCount * sizeof(char *));
>+    slotStrings = (char **)PORT_ArenaZAlloc(arena, slotCount * sizeof(char *));

We should handle the allocation failure here.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 145061 [details] [diff] [review]
Be more paranoid about data we've fetched from the database.

I will send review comments to Bob by private email.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: My alternative patch, unified diff

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Attachment: ny alternaive patch - context diff

This is the same patch as the previous attachment, but in context diff form.
Since this is practically a rewrite of the function, it's easier for a 
reviewer to review the correctness of the function using the second part
of this patch, than with a unified diff, IMO.  

I wish I could test this with a secmod.db file that is known to have one
or more of the problems that were the cause of this bug.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 145246 [details] [diff] [review]
ny alternaive patch - context diff

Please review this patch (or the equivalent patch in the previous attachment).  
Also, Please TEST this patch.  (I did, but my tree isn't completely clean of
other changes.)

Comment 14

[Robert Relyea]

Comment on attachment 145246 [details] [diff] [review]
ny alternaive patch - context diff

For the most part the patch is fine. There is only one stylistic thing I have a
problem with (doesn't warrant a r-), which is moving the intialization of the
internal paramters away from the point we initialize parmeters for other
tokens.

The part that does need to be fixed is the overlap check. For future
compatibility, the code should be agnostic about where the slot blocks and the
name blocks live in respect to each other).

I suggest either removing the check or reworking the check to handle the case
where the slot block comes before the name block.

I'll attach a patch which is a diff against nelson's patch (the majority of
which is fine) to accomplish this.

bob

Comment 15

[Robert Relyea]

Attachment: This is a context diff between my proposed code and nelson's patch

Comment 16

[Robert Relyea]

Attachment: Complete diff including nelson's patch & my changes to it

Comment 17

[Robert Relyea]

Comment on attachment 145279 [details] [diff] [review]
Complete diff including nelson's patch & my changes to it

This patch is basically nelson's patch 145246 with a delta. If you've already
reviewed nelson's patch, you can use the mozilla feature to display the
differences between 2 patches.

patch #145278 was an attemmpt to post just the diffs, but mozilla's diff
feature does not display it corrrectly.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

After I submitted the two patches above, I had these additional ideas on 
how to improve the checks:

1. Check that both the names offset and the slots offset do not overlap the
header part of the data buffer.
2. Check that both the names block and the slots blocks do not overlap,
regardless of order.  Since the length of the names block cannot be known 
until after that block is parsed, this check cannot be made until after the 
names block has been parsed, and the length of the slots block has been 
obtained (which is the same place where I placed the existing check).

IMO, the code should incorporate those improvements.
I will shortly review Bob's diffs.  

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 145279 [details] [diff] [review]
Complete diff including nelson's patch & my changes to it

Bob,  I agree with all of your latest patch except this part:

>+    /* 
>+     * Consistancy check: Part2. We now have the slot block length, we can 
>+     * check the case where the slotblock procedes the name block.
>+     */
>+    if (slotOffset < namesOffset) { /* slot block starts before name block */
>+	if (namesOffset > 
>+	       slotOffset + slotCount + 2 + slotCount*sizeof(secmodSlotData)) {
>+	    goto db_loser;
>+	}
>+    }

I think the sense of that inner comparison is backwards, and there seems to be 
an extra slotCount in the sum.	I think the proper test would be:

>+    if (slotOffset < namesOffset &&  /* slot block starts before name block */
>+	  slotOffset + 2 + slotCount*sizeof(secmodSlotData) > namesOffset) {
>+	    goto db_loser;
>+    }

Comment 20

[Robert Relyea]

Chris,

I've checked in the my final patch with nelson's changes to the NSS trunk. 
Where do you need the change to make the 1.7 release?

bob

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

I think this fix wants to go into the 3.9 branch, too, no?

Comment 22

[chris hofmann]

it is where ever we pick up NSS from... 
I think it is the 3.9 branch. 
wtc will know for sure

1.7 release has not branched yet it is still on the trunk.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Chris, This bug is not marked as blocking 1.7 final.  Does it?

The patches that fix this bug have been checked in on the NSS trunk, 
but not on any NSS branch.  For the actual checked-in diffs, see
http://bonsai.mozilla.org/cvsview2.cgi?command=DIFF&subdir=mozilla%2Fsecurity%2Fnss%2Flib%2Fsoftoken&file=pk11db.c&rev1=1.26&rev2=1.28&whitespace_mode=show&diff_mode=context

Unless the diffs are also applied to the NSS 3.9 branch, these changes 
will not go into NSS 3.9.1, and hence not into moz 1.7 final
(which intends to use NSS 3.9.1 RTM, as I understand it).  

Comment 24

[chris hofmann]

yes, we would very, very, much like to get this in 1.7...   can someone do the
dead...  marked as blocking and approved...

Comment 25

[chris hofmann]

Comment on attachment 145279 [details] [diff] [review]
Complete diff including nelson's patch & my changes to it

a=chofmann for 1.7

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for NSS 3.9 branch for moz 1.7

This patch is what Bob finally checked in on the NSS trunk to resolve this
bug for NSS 3.10.  But moz 1.7 will use NSS 3.9.1, which (IINM) is coming
from the NSS 3.9 branch.  

There is some question about whether this has had enough testing to go into 
moz 1.7 final.	It might be very helpful if some of the CC list readers of 
this bug would be willing to test moz 1.7 with a build of NSS shared libraries 

that include this fix.	Interested parties should email me directly.

Bob and Wan-Teh, are you ok with checking this into the NSS 3.9 branch for 
NSS 3.9.1 ?

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 145246 [details] [diff] [review]
ny alternaive patch - context diff

cancelling review request for obsolete patch.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 145279 [details] [diff] [review]
Complete diff including nelson's patch & my changes to it

cancelling review request for obsolete patch.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 145773 [details] [diff] [review]
patch for NSS 3.9 branch for moz 1.7

requesting second review for NSS 3.9.1 for moz 1.7 final.

Comment 30

[Wan-Teh Chang]

Comment on attachment 145773 [details] [diff] [review]
patch for NSS 3.9 branch for moz 1.7

I've checked in this patch on the NSS_3_9_BRANCH
(NSS 3.9.1) and into NSS_CLIENT_TAG (Mozilla 1.7
final).

Checking in pk11db.c;
/cvsroot/mozilla/security/nss/lib/softoken/pk11db.c,v  <--  pk11db.c
new revision: 1.26.16.1; previous revision: 1.26
done

Comment 31

[Asa Dotzler [:asa]]

Comment on attachment 145773 [details] [diff] [review]
patch for NSS 3.9 branch for moz 1.7

a=asa (on behalf of drivers) for checkin to 1.7

Comment 32

[Andreas Kunz]

Wan-Teh, Nelson:
I'm not familiar with the NSS branches, but this is now fixed on NSS_3_9_BRANCH,
NSS_CLIENT_TAG and trunk.
Are there any other relevant branches waiting or can this bug be resolved?

The fix seems also to be in what Mozilla 1.7 will use, so maybe the "fixed1.7"
keyword might be used to avoid drivers confusion with a open blocking1.7 bug.

Comment 33

[Wan-Teh Chang]

Marked the bug fixed.

This bug was fixed on the Mozilla trunk before the
Mozilla 1.7 branch was created, so the fix is also
in the Mozilla 1.7 branch.

Comment 34

[Julien Pierre]

Comment on attachment 145773 [details] [diff] [review]
patch for NSS 3.9 branch for moz 1.7

I don't claim to understand the whole of this patch. That said, I think the
following changes should be made :

1) in secmod_decodeData, check the value of the "data" argument for NULL first
thing after the variable declarations and return if NULL .

2) in the new code that reads
parameters = PORT_ArenaStrdup(arena, defParams);

Check for defParams to be non-NULL before .

3) In the new function secmod_FreeSlotStrings, check for slotStrings == NULL
upon entry, and add slotStrings[i] = NULL after PR_smprintf_free inside the
loop  to avoid possible double-frees

4) initialize all new and existig variables in their declarations. Even arena
which is initialized on the first line of code currenntly. Code can easily
change in the future without careful attention to the initialization value, and
bugs could be easily introduced if these variables remain uninitialized.

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

Attachment: The actual checkin

This patch shows what actually got checked in on the trunk, and the 3.9 branch.

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 145773 [details] [diff] [review]
patch for NSS 3.9 branch for moz 1.7

Sorry.	The last two attachments to this bug are identical.  I must be tired.
:(

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

I disagree with some of the suggestions in comment 34.  
But I think that the right place to discuss this (more null pointer checking)
is in bug 240546 so I will add comments there.

Comment 38

[Wan-Teh Chang]

Julien, thank you for the code review (comment 34).

None of the problems you pointed out are critical
or were introduced by Bob and Nelson's patch, so
we don't need to address them on the NSS 3.9 branch.

Your suggestions #1 and #2 and the first suggestion
in #3 are all about testing for NULL function arguments.
I can imagine people will disagree with some of these
suggestions.  So we should discuss them in bug 240546.

Your second suggestion in #3 and suggestion #4 are
good defensive programming practices and we can do
those on the trunk.

Comment 39

[Wan-Teh Chang]

Comment on attachment 145773 [details] [diff] [review]
patch for NSS 3.9 branch for moz 1.7

Bob, Nelson,

After 2 hours of staring at this patch, I finally
understood it.	I think this patch is correct.
The only change I suggest is to declare the
variable namesRunningOffset as 'unsigned long'
instead of 'unsigned short' so that it doesn't
overflow after we add len+2 to it (len is 'unsigned
short').

One thing I found suspicious is the 'hasRootTrust'
local variable, which is initialized to PR_FALSE
and never modified.  I am wondering why we don't
just use the constant PR_FALSE.  If Bob can look
into this, I'd appreciate it.

One comment about style: I see an inconsistency.
'slots' is a running pointer, where as 'names' is
a fixed pointer used in conjunction with the running
offset namesRunningOffset.  Just a nit.

Comment 40

[Robert Relyea]

Now that we've had 3 reveiwers and numerous authors, I've ran into a regression 
in this patch. Line 743 has:

if (nss == NULL) 
 	goto loser;

which I introduced in the original patch. Unfortunately nss=NULL is a valid 
value for tokens which don't have any NSS specific flags turned on (which 
usually applies to most *other* tokens besides the internal and rootcert tokens.

This line should change to:

   /* it's permissible (and normal) for nss to be NULL. it simply means
    * there are no NSS specific parameters in the database */

So much for agressive NULL checks. This change breaks smart cards.

Question: should we reopen this bug, or should we open a new one (since this is 
a regression, not an original problem...


bob


 

Comment 41

[chris hofmann]

either way... attach the new patch to this bug or open a new one, but lets get
it in soon.   Trying for another 1.7 Release candidate is next week.

Comment 42

[Wan-Teh Chang]

Attachment: Patch to fix regressions

This patch fixes the following bugs.

1. set slotStrings[i] to NULL after freeing it. (Julien's
   suggestion)
2. Fix spelling errors in comments.
3. Declare namesRunningOffset as unsigned long to
   avoid overflow.
4. Handle the allocation failure of slotStrings.
5. Remove the incorrect test of nss == NULL.  (Bob's fix)

Comment 43

[Wan-Teh Chang]

Comment on attachment 147359 [details] [diff] [review]
Patch to fix regressions

Requesting mozilla 1.7 approval.

Fixed two regressions. Added one defensive programming
change.  Fixed spelling errors in comments.

The risk of this patch is low.

Comment 44

[Wan-Teh Chang]

Comment on attachment 147359 [details] [diff] [review]
Patch to fix regressions

Patch checked into the NSS tip (NSS 3.10),
NSS_3_9_BRANCH (NSS 3.9.1) and NSS_CLIENT_TAG
(Mozilla 1.8 alpha).

Comment 45

[chris hofmann]

Comment on attachment 147359 [details] [diff] [review]
Patch to fix regressions

a=chofmann for 1.7

Comment 46

[Wan-Teh Chang]

Comment on attachment 147359 [details] [diff] [review]
Patch to fix regressions

Patch checked in on the MOZILLA_1_7_BRANCH
for Mozilla 1.7 final.