278381 - double frees found+fixed in pk11pars.h/pk11_mkCipherFlags()

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Tim Ruehsen]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.3; Linux) (KHTML, like Gecko)
Build Identifier: 

There are two (possible) double frees in pk11_mkCipherFlags() and one 
incomplete printf format specifier. 
 
The complete code looks like it needs to be rewritten, especially to be NSPR 
conforming (e.g. there are "%08x" PR_smprintf specifiers with 'long' arguments 
- looks like this might break where if have 64bit long integers) 
 
Anyway, this code is pretty old - is it still used anywhere? Is it worth 
working on? 
 

Reproducible: Always

Comment 1

[Tim Ruehsen]

Attachment: cvs diff for pk11pars.h

should fix the found issues

Comment 2

[Wan-Teh Chang]

Tim: good catch and your fix is correct.  How did you
discover this bug?

Bob: please evaluate the severity of these double frees.
Also, I don't understand the 0h0x%08x and 0l0x%08x format
strings.  What do the "0h" and "0l" mean?

Comment 3

[Wan-Teh Chang]

Attachment: patch for pk11pars.h, v1.1

This is Tim's patch plus my attempt to fix
the mismatch between the %08x format specifiers
and the unsigned long arguments.  I made them
%08lx and PRUint32.  (In NSPR, the "l" modifier
for "%x" means "32-bit", rather than "long".)  The
correctness of this fix requires that on platforms
where long is 64-bit, only the lower 32 bits of
slotID (unsigned long) are significant.

Comment 4

[Robert Relyea]

I'm surprised it hasn't caused a problem up to now. I would expect the softoken
to trip over this. (It should be patched as soon as possible).

I think the Oh and 0l stand for High and Low. The Database stores a 64 bit mask
as 2 32bit values.

Note: even if the long is 64 bits, the value of the long can only be up to 32 bits.

Good catch, BTW.

Comment 5

[Wan-Teh Chang]

I've checked in this patch on the NSS trunk (3.10).
Nelson, Julien, do you want this fix on the NSS 3.9
branch for the NSS 3.9.5 patch release?

Bob, the original code not only has double frees
but also fails to update 'cipher'.  Do you know
why things still work with the wrong value of
'cipher'?

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

when is this code used?   What commonly used NSS functions call it?

Comment 7

[Robert Relyea]

I believe it's used when reading configuration information out of the secmod.db.
I would expect it to trip on the softoken, and not on other PKCS #11 modules. I
may trip on other hardware accellerators.

Comment 8

[Tim Ruehsen]

Wan-Teh (#2): remember my request to use gcc's ability to check printf-like
formats? I built it into prprf.h for testing, and grepped all warnings coming
out. The one in pk11pars.h has been the worst. There are others line using "%d"
and having a 'long integer' as param. I make a list and file a bug next week.

There should be pendants within the javascript code to PR_s*printf(). I will
check the formats here as well. There are many other printf like functions, but
maybe not as intensivly used as PR_s*printf and JS_s*printf.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #5)
> Nelson, Julien, do you want this fix on the NSS 3.9 branch (...) ?

Yes, please.  Although there are no specific plans for another 3.9.x 
release at the moment, the liklihood of such another release in (say) 
the next 6 months seems high.  So, if you're willing, please do carry 
this fix back to the 3.9 branch.  Thanks!

Comment 10

[Wan-Teh Chang]

I carried this fix back to the 3.9 branch (NSS 3.9.6).

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Many thanks, Wan-Teh.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Setting priorities on unprioritized bugs resolved fixed for NSS 3.10.
Marking P1 since it fixes double free.

Comment 13

[Wan-Teh Chang]

Changed target milestone to 3.9.6 because we will make
a NSS 3.9.6 release before NSS 3.10.