Created attachment 536483 [details] bug reproducer As reported in Fedora 15: Nalin Dahyabhai 2011-05-31 17:15:54 EDT Description of problem: It looks as though intel_aes_decrypt_cbc_256() doesn't work right when the input and output buffers are the same, though this appears to be something that PK11_CipherOp (which eventually calls this function, on processors that support it) is expected to be able to handle. Version-Release number of selected component (if applicable): nss-softokn-freebl-3.12.10-1.fc15.x86_64 How reproducible: Always, provided you're on a 64-bit processor with native AES instruction support (the "flags:" line in /proc/cpuinfo will include "aes"). I've tested this on an i5 processor and also had it checked on an i7. Steps to Reproduce: 1. Run reproducer with the "-i" flag, which will cause it to use the same buffer for input and output and default to a 256-bit key. Actual results: "data mismatch" Expected results: "Encrypted/recovered 32 bytes." Additional info: While NSS itself doesn't seem to trigger this behavior, the nascent support for using NSS as the backend for krb5's libk5crypto does. [reply] [-] Private Comment 1 Nalin Dahyabhai 2011-05-31 17:19:59 EDT Created attachment 502120 [details] test Compile: gcc -o aest aest.c `pkg-config --cflags --libs nss` Run: ./aest -i (uses in-place encryption, errors out) env NSS_DISABLE_HW_AES=1 ./aest -i (uses unoptimized implementation, no error) ./aest (doesn't do encryption in place, no error)
Root cause seems to be that next iv is overwritten by output. intel_aes_decrypt_cbc_192 and intel_aes_decrypt_cbc_128 are same problem.
Ahh, intel_aes_decrypt_cbc_192 and intel_aes_decrypt_cbc_128 had no problem. This is intel_aes_decrypt_cbc_256 only.
Created attachment 536777 [details] [diff] [review] Don't clobber the IV in the minor loop Short Description of patch: Read the next IV from the buffer before we overwrite it with the plain text. In the minor loop for the 256 bit aes operation. Long Description: In CBC mode the IV for the next buffer is simply the previous encrypted buffer. Each of the AES encrypt/decrypt operations (split up by key size) has 2 loops. The first loop works with blocks of 128 bytes or more. In that loop, we 1) read the iv. 2) modify the decrypted text then go to the next iv. 3) after the last block, we read the last IV. 4) we write all the decrypted texts out to the buffer. In the second loop, we only decrypt one block at a time. We 1) read the block to decrypt (which will be the next iv). 2) save the block in a register. 3) decrypt the block. 4) xor the block with the iv 5) store the block. 6) set the iv to our saved block in a register. For 256 decrypt, we have run out of registers in which to save the previous encrypted block, so we re-read the block out of the input buffer. The bug is we do this after we have written the decrypted block out the in output buffer. Simply swapping these 2 instructions solves the problem. There isn't a corresponding issue for the encrypt case because the IV in the encrypt case is the same as the result of the encryption, so there was no need to save the previous input buffer block. 128 and 192 were fine because the had saved the iv in a register. bob
Comment on attachment 536777 [details] [diff] [review] Don't clobber the IV in the minor loop r=wtc. Perhaps we should add cipher tests that encrypt and decrypt in place. I recently learned that our SSL library depends on this (bug 626925).
Attachment #536777 - Flags: review?(wtc) → review+
trunk: Checking in intel-aes.s; /cvsroot/mozilla/security/nss/lib/freebl/intel-aes.s,v <-- intel-aes.s new revision: 1.5; previous revision: 1.4 done
branch: Checking in intel-aes.s; /cvsroot/mozilla/security/nss/lib/freebl/intel-aes.s,v <-- intel-aes.s new revision: 184.108.40.206; previous revision: 220.127.116.11 done
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.