diff mbox

crypto/testmgr: don't copy from source IV too much

Message ID 1441279921-26838-1-git-send-email-aryabinin@odin.com (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show

Commit Message

Andrey Ryabinin Sept. 3, 2015, 11:32 a.m. UTC
While the destination buffer 'iv' is MAX_IVLEN size,
the source 'template[i].iv' could be smaller. Thus
copying it via memcpy() leads to invalid memory access.
Use strlcpy() instead.

Signed-off-by: Andrey Ryabinin <aryabinin@odin.com>
---
 crypto/testmgr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Herbert Xu Sept. 3, 2015, 1:20 p.m. UTC | #1
On Thu, Sep 03, 2015 at 02:32:00PM +0300, Andrey Ryabinin wrote:
> While the destination buffer 'iv' is MAX_IVLEN size,
> the source 'template[i].iv' could be smaller. Thus
> copying it via memcpy() leads to invalid memory access.
> Use strlcpy() instead.
> 
> Signed-off-by: Andrey Ryabinin <aryabinin@odin.com>

Thanks for the patch.  Unfortunately it's broken because the IV
is not a string and can contain NULs.  So either fix it by using
the real ivsize, or change template[i].iv to a char array.

Cheers,
Andrey Ryabinin Sept. 4, 2015, 4:42 p.m. UTC | #2
On 09/03/2015 04:20 PM, Herbert Xu wrote:
> On Thu, Sep 03, 2015 at 02:32:00PM +0300, Andrey Ryabinin wrote:
>> While the destination buffer 'iv' is MAX_IVLEN size,
>> the source 'template[i].iv' could be smaller. Thus
>> copying it via memcpy() leads to invalid memory access.
>> Use strlcpy() instead.
>>
>> Signed-off-by: Andrey Ryabinin <aryabinin@odin.com>
> 
> Thanks for the patch.  Unfortunately it's broken because the IV
> is not a string and can contain NULs.  So either fix it by using
> the real ivsize,

So I've tried to use crypto_ablkcipher_ivsize(tfm) for that.

But noticed that some algs don't set ivsize (which makes it zero).
E.g. "ecb-cast6-avx" doesn't set it, but test vectors (cast6_enc_tv_template[], cast6_dec_tv_template[])
have .iv of 16bytes.

So I'm not sure what part is wrong here.
Is it wrong to use crypto_ablkcipher_ivsize(tfm) to get ivsize here?
Is it bug in 'ecb-cast6-avx'?
Or maybe something else?


> or change template[i].iv to a char array.
> 
> Cheers,
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu Sept. 5, 2015, 2:04 a.m. UTC | #3
On Fri, Sep 04, 2015 at 07:42:26PM +0300, Andrey Ryabinin wrote:
> 
> But noticed that some algs don't set ivsize (which makes it zero).
> E.g. "ecb-cast6-avx" doesn't set it, but test vectors (cast6_enc_tv_template[], cast6_dec_tv_template[])
> have .iv of 16bytes.

ECB should always have an IV size of zero so this is correct.
If the test vectors contain an IV it'll simply be ignored.

Cheers,
diff mbox

Patch

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index d0a42bd..d85221c 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -974,7 +974,7 @@  static int __test_skcipher(struct crypto_ablkcipher *tfm, int enc,
 			continue;
 
 		if (template[i].iv)
-			memcpy(iv, template[i].iv, MAX_IVLEN);
+			strlcpy(iv, template[i].iv, MAX_IVLEN);
 		else
 			memset(iv, 0, MAX_IVLEN);
 
@@ -1049,7 +1049,7 @@  static int __test_skcipher(struct crypto_ablkcipher *tfm, int enc,
 			continue;
 
 		if (template[i].iv)
-			memcpy(iv, template[i].iv, MAX_IVLEN);
+			strlcpy(iv, template[i].iv, MAX_IVLEN);
 		else
 			memset(iv, 0, MAX_IVLEN);