Open Bug 423093 Opened 16 years ago Updated 1 year ago

Decrypt With Merged SDR Key test failing

Categories

(NSS :: Libraries, defect, P5)

Tracking

(Not tracked)

REOPENED
3.12.5

People

(Reporter: nelson, Assigned: rrelyea)

References

Details

Attachments

(1 file)

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.
OS: Windows XP → Solaris
Priority: -- → P1
Hardware: PC → Sun
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
Comment on attachment 312133 [details] [diff] [review]
Handle the padding of one case (checked in)

asking self to review
Attachment #312133 - Flags: review?(nelson)
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+
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
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
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Blocks: 427706
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.
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.




Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
yes
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
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 → ---
Retargetting to 3.12.2 since 3.12.1 is done.
Target Milestone: 3.12.1 → 3.12.2
Attachment #312133 - Attachment description: Handle the padding of one case → Handle the padding of one case (checked in)
Target Milestone: 3.12.2 → 3.12.5
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
In Windows case old DB format was used and failure occurred directly in sdr.sh.
Is this still an issue of concern?
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
Severity: normal → S3
Priority: P1 → P5
You need to log in before you can comment on or make changes to this bug.