Closed Bug 536640 Opened 12 years ago Closed 11 years ago
valgrind warning in Decode
Item (about uninitialized local from nsslowkey _Decode PW)
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.
Assignee: nobody → rrelyea
Priority: -- → P2
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) 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?
Added comments for the subtlety of siBuffer (as opposed to siUnsignedInteger).
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=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(¶m.iter, &def_iter) call a problem?
> (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=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(¶m.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: 18.104.22.168; previous revision: 22.214.171.124 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
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: 126.96.36.199; 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.