Bug 474394

Created attachment 325561 [details]
Patch that got cbc(des3_ede) working

Huh? Weak keys are totally unacceptable to 3DES because they reduce it to DES.  I'd like to see where FIPS requires this.  Thanks.

Having reread your description, I think your patch does make sense.  If the user wants insecurity, we should always give it to them.

Regarding the ecb(des3_ede) problem, what exactly does it produce for you? Do you have some code that I can compile and run? Thanks!

Turns out I was chasing two different problems at the same time. One was simply my test infra not expanding the fips cavs single-key tdes inputs into three keys. The other is the weak key flag. I've got both cbc and ecb mode working reliably now.

However, I think the flag check in des_setkey and the one I added in my patch in comment #1 to des3_ede_setkey are actually inverted. That flag should only be set if we want to permit weak keys, and the check returns true if the flag is set, which I believe is the opposite of what is intended.


Test code:

http://git.engineering.redhat.com/?p=users/jwilson/fips-cavs-kernel-test.git;a=summary

I'm building this on RHEL5 w/the following caveats:
-the kernel module also needs rng.h, which isn't yet being installed as part of kernel-devel (Neil is working on a fix for this)
-a few kernel patches atop the 5.3 kernel are required, all of which have been posted to rhkl
-the userspace requires a newer libnl than we ship in RHEL5 (1.1 plus a 2.6.18 compatibility patch).

Haven't actually tested w/a current upstream kernel yet, on my TODO list once everything is ironed out on the RHEL5 side... The dsa signature verification bits definitely have to be disabled for upstream kernels, since the RHEL5 module signing patches aren't upstream.

Once everything's built, load the crypto-tester-kmod.ko, then:

crypto-tester -m cipher -c "cbc(des3_ede)" -e -k 8001010101010101 -i 0000000000000000 0000000000000000

The help text should be reasonably accurate. I can provide RHEL5 libnl and kernel srpms w/most of the necessary bits included, if so desired.

Created attachment 325733 [details]
Updated des weak key patch

So yeah, I'm pretty sure the flag check is inverted now, unless I'm misinterpreting what the flag actually means. I've added a 'reject_weak_keys' flag to my test kmod, and built a new kernel with this patch.

# insmod crypto-tester-kmod.ko
crypto_tester: kernel module loaded
 * Weak key checks disabled

[module loaded, sets the weak key flag for all blkcipher ops]

# crypto-tester -m cipher -c "ecb(des)" -e -k 8001010101010101 0000000000000000
95a8d72813daa94d

[good, got expected result for des here]

# crypto-tester -m cipher -c "ecb(des3_ede)" -e -k 800101010101010180010101010101018001010101010101 0000000000000000
95a8d72813daa94d

[good, tdes w/the same key 3x returns the same result as des]

# rmmod crypto-tester-kmod.ko ; insmod crypto-tester-kmod.ko reject_weak_keys=1

[module reloaded w/weak key flag not set]

[root@xaphod kmod]# crypto-tester -m cipher -c "ecb(des3_ede)" -e -k 800101010101010180010101010101018001010101010101 0000000000000000
des3_ede_setkey: weak key rejected
8ca64de9c1b123a7

[the incorrect result, w/reason in dmesg]

Submitting two patches upstream:
-one to fix the inverted weak key flag check in des_setkey()
-one to add a weak key flag check to des3_ede_setkey()

Created attachment 325761 [details]
v3 of weak keys patch

Just noticed that testmgr's test 5 for des-generic now reports a setkey failure. Updated patch sent upstream, rhel5 version attached here.

The existing WEAK flag convention might seem unusual, but it is self-consistent.  All implementations and users follow this rule.  So you can't just change it now.

Please submit your original patch instead.  Thanks!

Created attachment 325900 [details]
Latest rhel5 version of weak keys patch submitted upstream

Okay, submitted an updated version of the original patch, based on Herbert's comments upstream, which also changes the RES flag to something more appropriate (which matches des_setkey()).

Finally managed to get the patch right, has been committed upstream.

http://git.kernel.org/?p=linux/kernel/git/herbert/cryptodev-2.6.git;a=commit;h=3f46fa71cd6afb238e8fbe39cf23faacd2ddd1e3

This request was evaluated by Red Hat Product Management for inclusion in a Red
Hat Enterprise Linux maintenance release.  Product Management has requested
further review of this request by Red Hat Engineering, for potential
inclusion in a Red Hat Enterprise Linux Update release for currently deployed
products.  This request is not yet committed for inclusion in an Update
release.

Updating PM score.

in kernel-2.6.18-132.el5
You can download this test kernel from http://people.redhat.com/dzickus/el5

Please do NOT transition this bugzilla state to VERIFIED until our QE team
has sent specific instructions indicating when to do so.  However feel free
to provide a comment indicating that this fix has been verified.

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHSA-2009-1243.html