661061 - intel_aes_decrypt_cbc_256 doesn't work correctly when input and output buffers are the same

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Elio Maldonado]

Attachment: 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)

Comment 1

[Makoto Kato [:m_kato]]

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.

Comment 2

[Makoto Kato [:m_kato]]

Ahh, intel_aes_decrypt_cbc_192 and intel_aes_decrypt_cbc_128 had no problem.  This is intel_aes_decrypt_cbc_256 only.

Comment 3

[Robert Relyea]

Attachment: 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 4

[Wan-Teh Chang]

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).

Comment 5

[Robert Relyea]

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

Comment 6

[Robert Relyea]

branch:

Checking in intel-aes.s;
/cvsroot/mozilla/security/nss/lib/freebl/intel-aes.s,v  <--  intel-aes.s
new revision: 1.3.2.2; previous revision: 1.3.2.1
done