Closed Bug 536640 Opened 12 years ago Closed 11 years ago

valgrind warning in DecodeItem (about uninitialized local from nsslowkey_DecodePW)

Categories

(NSS :: Libraries, defect, P2)

3.12.4
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.8

People

(Reporter: dbaron, Assigned: wtc)

References

(Blocks 1 open bug)

Details

(Whiteboard: FIPS)

Attachments

(2 files, 4 obsolete files)

An invocation of certutil that's used in the automated testing harnesses that we use for testing Firefox causes a valgrind warning about use of uninitialized data.

Steps to reproduce (using the version of NSS currently on mozilla-central, which seems to be NSS 3.12.4 with some additional patches) are (in the dist/bin directory, which holds certutil and the NSS libraries):

rm -rf tmpdir
mkdir tmpdir
echo > tmpdir/.crtdbpw
LD_LIBRARY_PATH=. valgrind --tool=memcheck --leak-check=no --track-origins=yes \
  --num-callers=50 ./certutil -N -d ./tmpdir -f ./tmpdir/.crtdbpw


These steps yield the warning:

==17919== Conditional jump or move depends on uninitialised value(s)
==17919==    at 0x5644C5E: DecodeItem (quickder.c:827)
==17919==    by 0x56441C5: DecodeSequence (quickder.c:402)
==17919==    by 0x5644BA8: DecodeItem (quickder.c:809)
==17919==    by 0x5644DCC: SEC_QuickDERDecodeItem_Util (quickder.c:902)
==17919==    by 0x71D7F22: nsslowkey_DecodePW (keydb.c:1313)
==17919==    by 0x71D82D6: nsslowkey_PutPWCheckEntry (keydb.c:1440)
==17919==    by 0x71D97DD: lg_PutMetaData (keydb.c:2255)
==17919==    by 0x6A7B2DA: sftkdb_ChangePassword (sftkpwd.c:1264)
==17919==    by 0x6A56EE4: NSC_InitPIN (pkcs11.c:3254)
==17919==    by 0x52EA038: PK11_InitPin (pk11auth.c:449)
==17919==    by 0x41283A: SECU_ChangePW2 (secutil.c:442)
==17919==    by 0x40F6CF: certutil_main (certutil.c:2418)
==17919==    by 0x410ED2: main (certutil.c:2981)
==17919==  Uninitialised value was created by a stack allocation
==17919==    at 0x71D7E66: nsslowkey_DecodePW (keydb.c:1292)


The particular variable that's uninitialized is param.iter.type in the function nsslowkey_DecodePW.
Can it be reproduced with 3.12.4 without the additional patches?
They're bug 528277 (additional root certs) and bug 519550 (allow specifying an alternative name for the SQLite library), which both seem miles away from this code, so it doesn't seem worth the bother.

For the record, I also saw the warning when I accidentally forgot the LD_LIBRARY_PATH=. line and used the system NSS on Ubuntu 9.10.
thanks, David.
Assignee: nobody → rrelyea
Priority: -- → P2
Whiteboard: FIPS
Attached patch wip (obsolete) — Splinter Review
Fwiw, this fixed it for me.
Needs to also be initialized when we encode it, at line 1265.
Mats, thanks for the patch.

It's not clear whether the 'type' of param.iter should be initialized
to siBuffer or siUnsignedInteger.  The def_iter SECItem,
which is compared with param.iter, is initialized to siBuffer:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/legacydb/keydb.c&rev=1.11&mark=1249#1248

Since the expected iteration count (def_iter) is 1, it seems that
whether it's encoded as a signed or unsigned integer doesn't
matter because no leading zero is needed for 1 as unsigned
(which is why we don't get a valgrind warning when we encode it).
So I believe this valgrind warning can be safely ignored, and we
should fix this by initializing param.iter.type to siBuffer at those
two places.

We can also fix the QuickDER decoder to read param.iter.type
only when the input data has leading zeroes.  This fix will
allow us to not touch lib/softoken/legacydb/keydb.c for NSS
3.12.x.  We can add

  if (temp.len > 1 && temp.data[0] == 0)

around the highlighted code, and change the inner while loop
to a do-while loop:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/quickder.c&rev=1.23&mark=826-834#822
Mats, could you test this patch under valgrind, without your "wip" patch?  Thanks.
Attachment #425198 - Flags: review?(rrelyea)
Bob, should param.iter.type be siBuffer or siUnsignedInteger?
Attachment #425039 - Attachment is obsolete: true
Attachment #425199 - Flags: review?(rrelyea)
Added comments for the subtlety of siBuffer (as opposed to siUnsignedInteger).
Attachment #425199 - Attachment is obsolete: true
Attachment #425212 - Flags: review?(rrelyea)
Attachment #425199 - Flags: review?(rrelyea)
I think the iterator should be siUnsignedInteger. It's and integer, and it shouldn't be negative. Of course siBuffer is fine as long as we don't pass a value with the high bit on.

As far as the code refactoring, is there a reason to reorder the tests? The existing test seems easier to read to me. The reordered test just gets rid of valgrind warnings in some instances. I believe we decided the type field should always be initialized.

In short, I"m ok with Matt Palmgren's patch 2.1 with either siBuffer or siUnsignedInteger with a preference for the latter.
Bob, the main reason for the DecodeItem patch is to avoid changing
the softoken on the trunk, which is frozen for FIPS validation.
The other reason is that it makes this valgrind error less
likely for programming errors.  (Perhaps hiding programming
errors is a bad idea...)
When I implemented the siUnsignedInteger patch that Bob prefers, I found
that I didn't understand the code any more.  After studying DecodeItem
carefully, I convinced myself that the comparison with the default iteration
count is broken.  param.iter should have len=1 and data[0]=0x01 (a single
byte).  This patch fixes that.

The broken comparison only rejects an iteration count of 0x020101 (0x02
is the value of SEC_ASN1_INTEGER).  All other iteration counts are
accepted.

Bob, is the broken SECITEM_ItemsAreEqual(&param.iter, &def_iter) call a
problem?
Attachment #425212 - Attachment is obsolete: true
Attachment #425375 - Flags: review?(rrelyea)
Attachment #425212 - Flags: review?(rrelyea)
>  (Perhaps hiding programming errors is a bad idea...)

yeah, that's my thinking. We would rather have valgrind catch these than not.
> When I implemented the siUnsignedInteger patch that Bob prefers, I found
> that I didn't understand the code any more.  After studying DecodeItem
> carefully, I convinced myself that the comparison with the default iteration
> count is broken.  param.iter should have len=1 and data[0]=0x01 (a single
> byte).  This patch fixes that.

I'm a little afraid of this patch, even though it looks right. My fear is that somehow we've messed up the encoding of the iterator in that actual key database. If so, it's messed up in all versions of the old database. That means that our fix may not work with old databases.

If we convince ourselves that the fix is correct (and that the funky undecoded output was the result of the type being set to siBuffer), then I think this patch is preferable, otherwise I'm okay with going back to siBuffer.

bob
Bob: what I need from you first is to confirm that the
SECITEM_ItemsAreEqual(&param.iter, &def_iter) call is broken.

Once we agree that comparision is broken, then we can decide
what to do about it.  We can replace it with the correct
comparison (which my patch does), remove it (the comparison
returns success except in the unlikely event that param.iter
is 0x020101), or add a comment to document the comparison is
broken and we leave it there for fear of breaking old
databases.  We should not leave the broken comparison there
without a comment.  (I spent at least one hour studying the
relevant code because I was confused by it.)

I am fine with using siBuffer to reduce the risk, too, because
the uninitialized value of 'type' is more likely to be 0 (siBuffer)
than 10 (siUnsignedInteger).
Oh, I see the issue, the sense of the comparison was wrong. So it only failed if the number of iterations was set to 0x020101.

I don't think I would change this. It might be better just to loose the check, it's not that critical (obviously).

bob
Comment on attachment 425198 [details] [diff] [review]
Fix DecodeItem to look at destItem->type only when necessary

I'm going to r- this as per our discussion.

The patch does do what it's intended, but it looks like the intent is more harmful than the patch.
Attachment #425198 - Flags: review?(rrelyea) → review-
Comment on attachment 425375 [details] [diff] [review]
Mats Palmgren's patch v3 (for SOFTOKEN_3_13_BRANCH only)

r-  To be fair, wtc definately found some skanky code here. I would prefer to just remove the check altogether.

bob
Attachment #425375 - Flags: review?(rrelyea) → review-
Initialize param.iter.type to siBuffer, and remove the broken
comparison with def_iter.
Attachment #425375 - Attachment is obsolete: true
Comment on attachment 425587 [details] [diff] [review]
 Mats Palmgren's patch v4 (for SOFTOKEN_3_13_BRANCH only)(checked in)

r+ wtc already spent more time than it's worth, and this patch is acceptable (SOFTOKEN_3_13 branch).

bob
Attachment #425587 - Flags: review+
Comment on attachment 425587 [details] [diff] [review]
 Mats Palmgren's patch v4 (for SOFTOKEN_3_13_BRANCH only)(checked in)

I checked in the patch on the SOFTOKEN_3_13_BRANCH (NSS 3.13).

Checking in keydb.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/keydb.c,v  <--  keydb.c
new revision: 1.11.8.2; previous revision: 1.11.8.1
done
Attachment #425587 - Attachment description: Mats Palmgren's patch v4 (for SOFTOKEN_3_13_BRANCH only) → Mats Palmgren's patch v4 (for SOFTOKEN_3_13_BRANCH only)(checked in)
Since the MSB of 0x01 is 0, it doesn't need a leading 0 even if
we need to encode or decode it as unsigned integer, so the value
of the 'type' field doesn't matter, and this valgrind warning
can be safely suppressed.  It'll be fixed in NSS 3.13.
Assignee: rrelyea → wtc
Status: NEW → RESOLVED
Closed: 11 years ago
OS: Linux → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → 3.13
Blocks: FIPS2010
Duplicate of this bug: 584862
I checked in Mats Palmgren's patch v4 (attachment 425587 [details] [diff] [review])
on the NSS_3_12_BRANCH (NSS 3.12.8).

Checking in keydb.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/keydb.c,v  <--  keydb.c
new revision: 1.11.22.1; previous revision: 1.11
done
Target Milestone: 3.13 → 3.12.8
I'm still seeing these complaints in mozilla-central of 23 Aug

(TEST_PATH=toolkit/components/passwordmgr/test/test_master_password.html)

and indeed it does not look like the fix made it to m-c yet.  When will
that happen?
Julian: Definitely before Firefox 4 final ships.  Probably
within two or three weeks.  (The fix has been checked in
on the NSS_3_12_BRANCH, so it'll make it to mozilla-central
in the next NSS update for mozilla-central.)
You need to log in before you can comment on or make changes to this bug.