Open
Bug 423093
Opened 16 years ago
Updated 1 year ago
Decrypt With Merged SDR Key test failing
Categories
(NSS :: Libraries, defect, P5)
NSS
Libraries
Tracking
(Not tracked)
REOPENED
3.12.5
People
(Reporter: nelson, Assigned: rrelyea)
References
Details
Attachments
(1 file)
3.33 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
64 bit Standard Tinderbox tip run, Friday night, on SunOS/sparc system harpsichord, Started 21:16, finished 23:01, test failed. log excerpt: merge.sh: Decrypt - With Merged SDR Key sdrtest -d . -i /export/tinderbox/SunOS_5.10/mozilla/tests_results/security/harpsichord.2/tests.v1.9124 -t Test1 -f ../tests.pw.9124 merge.sh: #2236: Decrypt - Value 1 - FAILED Methinks that test output should be a little more verbose. An error message would be nice. I'm guessing this is a library bug, but it could be a test program bug.
Reporter | ||
Updated•16 years ago
|
OS: Windows XP → Solaris
Priority: -- → P1
Hardware: PC → Sun
Assignee | ||
Comment 1•16 years ago
|
||
When we have multiple abmiguous SDR keys, it's possible to accidently get a padding of 1 from and invalid encryption. This code checks to see if there is a better 'match' before returning decrypted result with padding of 1 as correct. bob
Reporter | ||
Comment 2•16 years ago
|
||
Comment on attachment 312133 [details] [diff] [review] Handle the padding of one case (checked in) asking self to review
Attachment #312133 -
Flags: review?(nelson)
Reporter | ||
Comment 3•16 years ago
|
||
Comment on attachment 312133 [details] [diff] [review] Handle the padding of one case (checked in) This patch assumes that an error probability of 1/256 is too high, but 1/65536 is low enough. One could imagine a situation in which one key decrypts a value to a one-byte pad that is correct, and another key decrypts to a two-byte pad that is actually incorrect, but less probably incorrect. With millions of users, I can see this turning into a multi-digit number of bug reports. I guess we'll have to hope that DB mergers are rare.
Attachment #312133 -
Flags: review?(nelson) → review+
Assignee | ||
Comment 4•16 years ago
|
||
This argues for the threshold of 2 instead of one (I had that in my original patch). That's probably a good idea. That would mean the incorrect encryption would only happen if the value had a size of 1 or 2 and we have a 'merged case' (the data was not encrypted with the key id of the requested key) and we hit the 1/256 or 1/65536 case. This will increase the number of correct encryption values that could fail (from 1 in 8 to 1 in 4), but decrease the number of correctly encoded keys that would fail. I think this is probably a good trade off. bob
Assignee | ||
Comment 5•16 years ago
|
||
Checking in pk11sdr.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11sdr.c,v <-- pk11sdr.c new revision: 1.20; previous revision: 1.19 done Checked in with the threshold set to 2. This means there is less chance that you will get a failure if the key is identified correctly, (1:4*256^3 versus 1:8*245^2), but a higher chance that a password that was encrypted with a merged key will no longer decrypt correctly (1:4*256 versus 1:8*256). Without the patch we have no chance of failure if the key is correctly identified, but a 1:256 chance a merged key will no longer decrypt, and a 1:256 chance our tests will fail. The former is by far the more common case than the latter. Our test cases would not hit either of them. bob
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 6•16 years ago
|
||
I don't understand the patch. (a) What confuses me most: The old code didn't have any calls to SECITEM_ZfreeItem, which you added. But you didn't mention any leaks in this bug. (b) I'd assume that "result" is the function's output parameter and sdrResult.data is simply the output some asn.1 decoding. But sdrResult.data is still a buffer that must get decrypted...? But the only places that populate "result" are the calls to pk11Decrypt. You never assign possibleResult to the out parameter result. (c) I'd assume it's "result" that you must save into possibleResult if you decide to try other keys, and I'd expect that you must free "result" if you decide to attempt further decrypt operationg, without saving "result" for later use. (d) I'd assume, if the temporary asn.1 decoded data in sdrResult.data must be destroyed, that you'd have a single destroy call somewhere in the cleanup section. But chances are high I've completely misunderstood this function, as I've never touched any code at this level.
Assignee | ||
Comment 7•16 years ago
|
||
Kai, So what is going on with this patch is follows: To help handle the case where we merge multiple databases into a single databases where the multiple databases each had separate SDR keys with the same ID. When we merge the databases, each key will get a unique id, but the application's SDR data will still have the old ID. The code handles these cases by seeing if the decryption 'worked' for the specified ID and if it didn't search for a result among the remaining keys. We detect whether the encryption 'worked' by seeing if we have a properly formatted result. Unfortunately there is an all to common case where the result we get back may be a false positive (padding cases of 1 (1:256) and 2 (1:65535)). The code prefers longer successful pads (3 and above yeilds false results in 1 in 64 million or better). In the new case we need to save the intermediate results and free them if they aren't what we want. In case of 'ties' (two weak results), we prefer the specified key ID over the other keys. So for a) the old code never maintained more than one result, once we found an acceptable result, we returned int b) c) and d) You are absolutely correct, this is the source of the bug.
Updated•16 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 8•16 years ago
|
||
Bob, You committed a fix for bug 427706. checked into 3.12.1 Checking in pk11sdr.c; new revision: 1.22; previous revision: 1.21 Does that checkin fix this bug? Can this be marked fixed?
Target Milestone: 3.12 → 3.12.1
Reporter | ||
Updated•16 years ago
|
Blocks: NSS312regressions
Assignee | ||
Comment 9•16 years ago
|
||
yes
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Comment 10•16 years ago
|
||
Reopening, seems that bug is still there: merge.sh: Decrypt - With Merged SDR Key sdrtest -d . -i /share/builds/mccrel3/security/securitytip/builds/20080622.1/biarritz_Solaris10_amd64/mozilla/tests_results/security/mandela.1/pkix/tests.v1.12404 -t Test1 -f ../tests.pw.12404 merge.sh: #3612: Decrypt - Value 1 - FAILED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 11•16 years ago
|
||
Retargetting to 3.12.2 since 3.12.1 is done.
Target Milestone: 3.12.1 → 3.12.2
Reporter | ||
Updated•16 years ago
|
Attachment #312133 -
Attachment description: Handle the padding of one case → Handle the padding of one case (checked in)
Reporter | ||
Updated•15 years ago
|
Target Milestone: 3.12.2 → 3.12.5
Comment 12•15 years ago
|
||
Occured also on Windows: sdr.sh: SDR Decrypt - Value 3 sdrtest -d . -i C:/mozilla-build/msys/export/tinderlight/data/goride_32_DBG/mozilla/tests_results/security/GORIDE.1/tests.v3.2400 -t "1234567" sdr.sh: #828: Decrypt - Value 3 - FAILED
OS: Solaris → All
Hardware: Sun → All
Comment 13•15 years ago
|
||
In Windows case old DB format was used and failure occurred directly in sdr.sh.
Is this still an issue of concern?
Assignee | ||
Comment 15•14 years ago
|
||
I don't think we are seeing test failures any more. There is still a chance that when merging 2 databases that we can't tell which key encrypted the SDR value. It's as good as we can get at the low level, since we don't know what the decrypted form is supposed to look like. In the case where there is ambiguity we always use the key the application told us to use (so in the end, the app has full control). There are things the application can do to make this better, including trying a different key if the decrypt succeeded, but the data was garbage. bob
Updated•2 years ago
|
Severity: normal → S3
Updated•1 year ago
|
Priority: P1 → P5
You need to log in
before you can comment on or make changes to this bug.
Description
•