Skip to contentSkip to conversation

crypto: ansi_cprng: avoid incorrect extra call to _get_more_prng_bytes

PATCH In review Jarod Wilson 3 participants 4 messages Nov 12, 2008 – Nov 13, 2008 Crypto API

lore
Share & tools
lore

Jarod Wilson

author
#0 lore raw

PATCH 1/1 crypto: ansi_cprng: avoid incorrect extra call to _get_more_prng_bytes

While working with some FIPS RNGVS test vectors yesterday, I discovered a little bug in the way the ansi_cprng code works right now.

For example, the following test vector (complete with expected result) from http://csrc.nist.gov/groups/STM/cavp/documents/rng/RNGVS.pdf ...

Key = f3b1666d13607242ed061cabb8d46202
DT = e6b3be782a23fa62d71d4afbb0e922fc
V = f0000000000000000000000000000000
R = 88dda456302423e5f69da57e7b95c73a

...when run through ansi_cprng, yields an incorrect R value of e2afe0d794120103d6e86a2b503bdfaa.

If I load up ansi_cprng w/dbg=1 though, it was fairly obvious what was going wrong:

----8<----
getting 16 random bytes for context ffff810033fb2b10
Calling _get_more_prng_bytes for context ffff810033fb2b10 Input DT: 00000000: e6 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc Input I: 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Input V: 00000000: f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 tmp stage 0: 00000000: e6 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc tmp stage 1: 00000000: f4 8e cb 25 94 3e 8c 31 d6 14 cd 8a 23 f1 3f 84 tmp stage 2: 00000000: 8c 53 6f 73 a4 1a af d4 20 89 68 f4 58 64 f8 be Returning new block for context ffff810033fb2b10
Output DT: 00000000: e7 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc Output I: 00000000: 04 8e cb 25 94 3e 8c 31 d6 14 cd 8a 23 f1 3f 84 Output V: 00000000: 48 89 3b 71 bc e4 00 b6 5e 21 ba 37 8a 0a d5 70 New Random Data: 00000000: 88 dd a4 56 30 24 23 e5 f6 9d a5 7e 7b 95 c7 3a Calling _get_more_prng_bytes for context ffff810033fb2b10 Input DT: 00000000: e7 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc Input I: 00000000: 04 8e cb 25 94 3e 8c 31 d6 14 cd 8a 23 f1 3f 84 Input V: 00000000: 48 89 3b 71 bc e4 00 b6 5e 21 ba 37 8a 0a d5 70 tmp stage 0: 00000000: e7 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc tmp stage 1: 00000000: 80 6b 3a 8c 23 ae 8f 53 be 71 4c 16 fc 13 b2 ea tmp stage 2: 00000000: 2a 4d e1 2a 0b 58 8e e6 36 b8 9c 0a 26 22 b8 30 Returning new block for context ffff810033fb2b10
Output DT: 00000000: e8 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc Output I: 00000000: c8 e2 01 fd 9f 4a 8f e5 e0 50 f6 21 76 19 67 9a Output V: 00000000: ba 98 e3 75 c0 1b 81 8d 03 d6 f8 e2 0c c6 54 4b New Random Data: 00000000: e2 af e0 d7 94 12 01 03 d6 e8 6a 2b 50 3b df aa returning 16 from get_prng_bytes in context ffff810033fb2b10 ----8<----

The expected result is there, in the first "New Random Data", but we're incorrectly making a second call to _get_more_prng_bytes, due to some checks that are slightly off, which resulted in our original bytes never being returned anywhere.

One approach to fixing this would be to alter some byte_count checks in get_prng_bytes, but it would mean the last DEFAULT_BLK_SZ bytes would be copied a byte at a time, rather than in a single memcpy, so a slightly more involved, equally functional, and ultimately more efficient way of fixing this was suggested to me by Neil, which I'm submitting here. All of the RNGVS ANSI X9.31 AES128 VST test vectors I've passed through ansi_cprng are now returning the expected results with this change.

---

 crypto/ansi_cprng.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)
crypto/ansi_cprng.c +11 -6
index 486aa93..1b3b1da 100644
@@ -223,9 +223,10 @@ remainder:
 	}
 
 	/*
-	 * Copy up to the next whole block size
+	 * Copy any data less than an entire block
 	 */
 	if (byte_count < DEFAULT_BLK_SZ) {
+empty_rbuf:
 		for (; ctx->rand_data_valid < DEFAULT_BLK_SZ;
 			ctx->rand_data_valid++) {
 			*ptr = ctx->rand_data[ctx->rand_data_valid];

@@ -240,18 +241,22 @@ remainder:
 	 * Now copy whole blocks
 	 */
 	for (; byte_count >= DEFAULT_BLK_SZ; byte_count -= DEFAULT_BLK_SZ) {
-		if (_get_more_prng_bytes(ctx) < 0) {
-			memset(buf, 0, nbytes);
-			err = -EINVAL;
-			goto done;
+		if (ctx->rand_data_valid == DEFAULT_BLK_SZ) {
+			if (_get_more_prng_bytes(ctx) < 0) {
+				memset(buf, 0, nbytes);
+				err = -EINVAL;
+				goto done;
+			}
 		}
+		if (ctx->rand_data_valid > 0)
+			goto empty_rbuf;
 		memcpy(ptr, ctx->rand_data, DEFAULT_BLK_SZ);
 		ctx->rand_data_valid += DEFAULT_BLK_SZ;
 		ptr += DEFAULT_BLK_SZ;
 	}
 
 	/*
-	 * Now copy any extra partial data
+	 * Now go back and get any remaining partial block
 	 */
 	if (byte_count)
 		goto remainder;
Signature · 2 lines
Jarod Wilson
[email protected]
Acked-by on v1 1/1
#1 lore raw
Quoted 59 lines · Jarod Wilson

While working with some FIPS RNGVS test vectors yesterday, I discovered a little bug in the way the ansi_cprng code works right now.

For example, the following test vector (complete with expected result) from http://csrc.nist.gov/groups/STM/cavp/documents/rng/RNGVS.pdf ...

Key = f3b1666d13607242ed061cabb8d46202
DT = e6b3be782a23fa62d71d4afbb0e922fc
V = f0000000000000000000000000000000
R = 88dda456302423e5f69da57e7b95c73a

...when run through ansi_cprng, yields an incorrect R value of e2afe0d794120103d6e86a2b503bdfaa.

If I load up ansi_cprng w/dbg=1 though, it was fairly obvious what was going wrong:

----8<----
getting 16 random bytes for context ffff810033fb2b10
Calling _get_more_prng_bytes for context ffff810033fb2b10 Input DT: 00000000: e6 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc Input I: 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Input V: 00000000: f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 tmp stage 0: 00000000: e6 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc tmp stage 1: 00000000: f4 8e cb 25 94 3e 8c 31 d6 14 cd 8a 23 f1 3f 84 tmp stage 2: 00000000: 8c 53 6f 73 a4 1a af d4 20 89 68 f4 58 64 f8 be Returning new block for context ffff810033fb2b10
Output DT: 00000000: e7 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc Output I: 00000000: 04 8e cb 25 94 3e 8c 31 d6 14 cd 8a 23 f1 3f 84 Output V: 00000000: 48 89 3b 71 bc e4 00 b6 5e 21 ba 37 8a 0a d5 70 New Random Data: 00000000: 88 dd a4 56 30 24 23 e5 f6 9d a5 7e 7b 95 c7 3a Calling _get_more_prng_bytes for context ffff810033fb2b10 Input DT: 00000000: e7 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc Input I: 00000000: 04 8e cb 25 94 3e 8c 31 d6 14 cd 8a 23 f1 3f 84 Input V: 00000000: 48 89 3b 71 bc e4 00 b6 5e 21 ba 37 8a 0a d5 70 tmp stage 0: 00000000: e7 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc tmp stage 1: 00000000: 80 6b 3a 8c 23 ae 8f 53 be 71 4c 16 fc 13 b2 ea tmp stage 2: 00000000: 2a 4d e1 2a 0b 58 8e e6 36 b8 9c 0a 26 22 b8 30 Returning new block for context ffff810033fb2b10
Output DT: 00000000: e8 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc Output I: 00000000: c8 e2 01 fd 9f 4a 8f e5 e0 50 f6 21 76 19 67 9a Output V: 00000000: ba 98 e3 75 c0 1b 81 8d 03 d6 f8 e2 0c c6 54 4b New Random Data: 00000000: e2 af e0 d7 94 12 01 03 d6 e8 6a 2b 50 3b df aa returning 16 from get_prng_bytes in context ffff810033fb2b10 ----8<----

The expected result is there, in the first "New Random Data", but we're incorrectly making a second call to _get_more_prng_bytes, due to some checks that are slightly off, which resulted in our original bytes never being returned anywhere.

One approach to fixing this would be to alter some byte_count checks in get_prng_bytes, but it would mean the last DEFAULT_BLK_SZ bytes would be copied a byte at a time, rather than in a single memcpy, so a slightly more involved, equally functional, and ultimately more efficient way of fixing this was suggested to me by Neil, which I'm submitting here. All of the RNGVS ANSI X9.31 AES128 VST test vectors I've passed through ansi_cprng are now returning the expected results with this change.

Quoted 2 lines

---

I went over this with Jarod. Its a good fix.

Acked-by Neil Horman <[email protected]>

Neil

Signature · 4 lines
/****************************************************
 * Neil Horman <[email protected]>
 * Software Engineer, Red Hat
 ****************************************************/
author
#2 lore raw
In reply to Jarod Wilson
Quoted 51 lines · On Wednesday 12 November 2008 14:52:59 Jarod Wilson wrote:

While working with some FIPS RNGVS test vectors yesterday, I discovered a little bug in the way the ansi_cprng code works right now.

For example, the following test vector (complete with expected result) from http://csrc.nist.gov/groups/STM/cavp/documents/rng/RNGVS.pdf ...

Key = f3b1666d13607242ed061cabb8d46202
DT = e6b3be782a23fa62d71d4afbb0e922fc
V = f0000000000000000000000000000000
R = 88dda456302423e5f69da57e7b95c73a

...when run through ansi_cprng, yields an incorrect R value of e2afe0d794120103d6e86a2b503bdfaa.

If I load up ansi_cprng w/dbg=1 though, it was fairly obvious what was going wrong:

----8<----
getting 16 random bytes for context ffff810033fb2b10
Calling _get_more_prng_bytes for context ffff810033fb2b10 Input DT: 00000000: e6 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc Input I: 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Input V: 00000000: f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 tmp stage 0: 00000000: e6 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc tmp stage 1: 00000000: f4 8e cb 25 94 3e 8c 31 d6 14 cd 8a 23 f1 3f 84 tmp stage 2: 00000000: 8c 53 6f 73 a4 1a af d4 20 89 68 f4 58 64 f8 be Returning new block for context ffff810033fb2b10
Output DT: 00000000: e7 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc Output I: 00000000: 04 8e cb 25 94 3e 8c 31 d6 14 cd 8a 23 f1 3f 84 Output V: 00000000: 48 89 3b 71 bc e4 00 b6 5e 21 ba 37 8a 0a d5 70 New Random Data: 00000000: 88 dd a4 56 30 24 23 e5 f6 9d a5 7e 7b 95 c7 3a Calling _get_more_prng_bytes for context ffff810033fb2b10 Input DT: 00000000: e7 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc Input I: 00000000: 04 8e cb 25 94 3e 8c 31 d6 14 cd 8a 23 f1 3f 84 Input V: 00000000: 48 89 3b 71 bc e4 00 b6 5e 21 ba 37 8a 0a d5 70 tmp stage 0: 00000000: e7 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc tmp stage 1: 00000000: 80 6b 3a 8c 23 ae 8f 53 be 71 4c 16 fc 13 b2 ea tmp stage 2: 00000000: 2a 4d e1 2a 0b 58 8e e6 36 b8 9c 0a 26 22 b8 30 Returning new block for context ffff810033fb2b10
Output DT: 00000000: e8 b3 be 78 2a 23 fa 62 d7 1d 4a fb b0 e9 22 fc Output I: 00000000: c8 e2 01 fd 9f 4a 8f e5 e0 50 f6 21 76 19 67 9a Output V: 00000000: ba 98 e3 75 c0 1b 81 8d 03 d6 f8 e2 0c c6 54 4b New Random Data: 00000000: e2 af e0 d7 94 12 01 03 d6 e8 6a 2b 50 3b df aa returning 16 from get_prng_bytes in context ffff810033fb2b10 ----8<----

The expected result is there, in the first "New Random Data", but we're incorrectly making a second call to _get_more_prng_bytes, due to some checks that are slightly off, which resulted in our original bytes never being returned anywhere.

Quoted 7 lines

One approach to fixing this would be to alter some byte_count checks in get_prng_bytes, but it would mean the last DEFAULT_BLK_SZ bytes would be copied a byte at a time, rather than in a single memcpy, so a slightly more involved, equally functional, and ultimately more efficient way of fixing this was suggested to me by Neil, which I'm submitting here. All of the RNGVS ANSI X9.31 AES128 VST test vectors I've passed through ansi_cprng are now returning the expected results with this change.

D'oh. Forgot this:

Signed-off-by Jarod Wilson <[email protected]>
Signature · 2 lines
Jarod Wilson
[email protected]
maintainer
#3 lore raw
Quoted 2 lines · Jarod Wilson
Signed-off-by Jarod Wilson <[email protected]>

Patch applied. Thanks everyone!

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt