536640 - valgrind warning in DecodeItem (about uninitialized local from nsslowkey_DecodePW)

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Can it be reproduced with 3.12.4 without the additional patches?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

thanks, David.

Comment 4

[Mats Palmgren (inactive)]

Attachment: wip

Fwiw, this fixed it for me.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Needs to also be initialized when we encode it, at line 1265.

Comment 6

[Wan-Teh Chang]

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

Comment 7

[Wan-Teh Chang]

Attachment: Fix DecodeItem to look at destItem->type only when necessary

Mats, could you test this patch under valgrind, without your "wip" patch?  Thanks.

Comment 8

[Wan-Teh Chang]

Attachment: Mats Palmgren's patch v2 (for SOFTOKEN_3_13_BRANCH only)

Bob, should param.iter.type be siBuffer or siUnsignedInteger?

Comment 9

[Wan-Teh Chang]

Attachment: Mats Palmgren's patch v2.1 (for SOFTOKEN_3_13_BRANCH only)

Added comments for the subtlety of siBuffer (as opposed to siUnsignedInteger).

Comment 10

[Robert Relyea]

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.

Comment 11

[Wan-Teh Chang]

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...)

Comment 12

[Wan-Teh Chang]

Attachment: Mats Palmgren's patch v3 (for SOFTOKEN_3_13_BRANCH only)

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?

Comment 13

[Robert Relyea]

>  (Perhaps hiding programming errors is a bad idea...)

yeah, that's my thinking. We would rather have valgrind catch these than not.

Comment 14

[Robert Relyea]

> 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

Comment 15

[Wan-Teh Chang]

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).

Comment 16

[Robert Relyea]

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 17

[Robert Relyea]

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.

Comment 18

[Robert Relyea]

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

Comment 19

[Wan-Teh Chang]

Attachment: Mats Palmgren's patch v4 (for SOFTOKEN_3_13_BRANCH only)(checked in)

Initialize param.iter.type to siBuffer, and remove the broken
comparison with def_iter.

Comment 20

[Robert Relyea]

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

Comment 21

[Wan-Teh Chang]

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

Comment 22

[Wan-Teh Chang]

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.

Comment 24

[Wan-Teh Chang]

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

Comment 25

[Julian Seward [:jseward]]

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?

Comment 26

[Wan-Teh Chang]

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.)