[2.6 patch] crypto/aes.c: array overrun

[2.6 patch] crypto/aes.c: array overrun

[Adrian Bunk]

The Coverity checker spotted the following in crypto/aes.c:

<--  snip  -->

...
struct aes_ctx {
        int key_length;
        u32 E[60];
        u32 D[60];
};

#define E_KEY ctx->E
...
#define loop8(i)                                    \
{   t = ror32(t,  8); ; t = ls_box(t) ^ rco_tab[i];  \
    t ^= E_KEY[8 * i];     E_KEY[8 * i + 8] = t;    \
    t ^= E_KEY[8 * i + 1]; E_KEY[8 * i + 9] = t;    \
    t ^= E_KEY[8 * i + 2]; E_KEY[8 * i + 10] = t;   \
    t ^= E_KEY[8 * i + 3]; E_KEY[8 * i + 11] = t;   \
    t  = E_KEY[8 * i + 4] ^ ls_box(t);    \
    E_KEY[8 * i + 12] = t;                \
    t ^= E_KEY[8 * i + 5]; E_KEY[8 * i + 13] = t;   \
    t ^= E_KEY[8 * i + 6]; E_KEY[8 * i + 14] = t;   \
    t ^= E_KEY[8 * i + 7]; E_KEY[8 * i + 15] = t;   \
}

static int
aes_set_key(void *ctx_arg, const u8 *in_key, unsigned int key_len, u32 *flags)
{
...
        case 32:
...
                for (i = 0; i < 7; ++i)
                        loop8 (i);
...

<--  snip  -->


The problem is:

  8 * 6 + 15 = 63  >  59


cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [2.6 patch] crypto/aes.c: array overrun

[Herbert Xu]

On Sat, Mar 11, 2006 at 02:03:39AM +0100, Adrian Bunk wrote:
>
> ...
> #define loop8(i)                                    \

...

>     t ^= E_KEY[8 * i + 7]; E_KEY[8 * i + 15] = t;   \
> }
> 
> static int
> aes_set_key(void *ctx_arg, const u8 *in_key, unsigned int key_len, u32 *flags)
> {
> ...
>         case 32:
> ...
>                 for (i = 0; i < 7; ++i)
>                         loop8 (i);

OK this is not pretty but it is actually correct.  Notice how we only
overstep the mark for E_KEY but never for D_KEY.  Since D_KEY is only
initialised after this, it is OK for us to trash the start of D_KEY.

It's just a trick that makes the code slightly nicer (and no I didn't
write this nor am I necessarily condoning it :)

Thanks for reporting this though.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [2.6 patch] crypto/aes.c: array overrun

[Pavel Machek]

On So 11-03-06 13:41:16, Herbert Xu wrote:
> On Sat, Mar 11, 2006 at 02:03:39AM +0100, Adrian Bunk wrote:
> >
> > ...
> > #define loop8(i)                                    \
> 
> ...
> 
> >     t ^= E_KEY[8 * i + 7]; E_KEY[8 * i + 15] = t;   \
> > }
> > 
> > static int
> > aes_set_key(void *ctx_arg, const u8 *in_key, unsigned int key_len, u32 *flags)
> > {
> > ...
> >         case 32:
> > ...
> >                 for (i = 0; i < 7; ++i)
> >                         loop8 (i);
> 
> OK this is not pretty but it is actually correct.  Notice how we only
                                  ~~~~~~~~~~~~~~~~~
> overstep the mark for E_KEY but never for D_KEY.  Since D_KEY is only
> initialised after this, it is OK for us to trash the start of D_KEY.
> 
> It's just a trick that makes the code slightly nicer (and no I didn't
> write this nor am I necessarily condoning it :)

Overstepping array is not correct C. Even if gcc lays it out in order
where array-to-be-thrashed is after it, so it works in practice, it is
not okay. [Some kind of security-hardened-gcc may stop this as buffer
overflow, for example]
								Pavel
-- 
161:    {

Re: [2.6 patch] crypto/aes.c: array overrun

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 495 bytes --]

On Sat, 11 Mar 2006 13:41:16 +1100, Herbert Xu said:

> OK this is not pretty but it is actually correct.  Notice how we only
> overstep the mark for E_KEY but never for D_KEY.  Since D_KEY is only
> initialised after this, it is OK for us to trash the start of D_KEY.

I think a big comment block describing this behavior is called for,
as it carries an implicit requirement that D_KEY and E_KEY remain
adjacent in memory.  Anybody allocating space between them is in for
a rude awakening....


[-- Attachment #2: Type: application/pgp-signature, Size: 228 bytes --]

Re: [2.6 patch] crypto/aes.c: array overrun

[David McCullough]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]


Jivin Valdis.Kletnieks@vt.edu lays it down ...
> On Sat, 11 Mar 2006 13:41:16 +1100, Herbert Xu said:
> 
> > OK this is not pretty but it is actually correct.  Notice how we only
> > overstep the mark for E_KEY but never for D_KEY.  Since D_KEY is only
> > initialised after this, it is OK for us to trash the start of D_KEY.
> 
> I think a big comment block describing this behavior is called for,
> as it carries an implicit requirement that D_KEY and E_KEY remain
> adjacent in memory.  Anybody allocating space between them is in for
> a rude awakening....

Sounds like a bug waiting to happen to me.
Why not do something like the attached patch.

Cheers,
Davidm

-- 
David McCullough, david_mccullough@au.securecomputing.com, Ph:+61 734352815
Secure Computing - SnapGear  http://www.uCdot.org http://www.cyberguard.com

[-- Attachment #2: aes.diff --]
[-- Type: text/plain, Size: 600 bytes --]

Index: linux-2.6.x/crypto/aes.c
===================================================================
RCS file: linux-2.6.x/crypto/aes.c,v
retrieving revision 1.1.1.6
diff -u -r1.1.1.6 aes.c
--- linux-2.6.x/crypto/aes.c	31 Aug 2005 00:33:03 -0000	1.1.1.6
+++ linux-2.6.x/crypto/aes.c	14 Mar 2006 22:53:06 -0000
@@ -78,12 +78,11 @@
 
 struct aes_ctx {
 	int key_length;
-	u32 E[60];
-	u32 D[60];
+	u32 _KEYS[120];
 };
 
-#define E_KEY ctx->E
-#define D_KEY ctx->D
+#define E_KEY (&ctx->_KEYS[0])
+#define D_KEY (&ctx->_KEYS[60])
 
 static u8 pow_tab[256] __initdata;
 static u8 log_tab[256] __initdata;

Re: [2.6 patch] crypto/aes.c: array overrun

[Herbert Xu]

On Wed, Mar 15, 2006 at 08:54:48AM +1000, David McCullough wrote:
>  
>  struct aes_ctx {
>  	int key_length;
> -	u32 E[60];
> -	u32 D[60];
> +	u32 _KEYS[120];
>  };

Looks good.  Thanks for this David.

Could you please change the name from _KEYS to buf and patch the x86-64
version as well?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [2.6 patch] crypto/aes.c: array overrun

[David McCullough]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]


Jivin Herbert Xu lays it down ...
> On Wed, Mar 15, 2006 at 08:54:48AM +1000, David McCullough wrote:
> >  
> >  struct aes_ctx {
> >  	int key_length;
> > -	u32 E[60];
> > -	u32 D[60];
> > +	u32 _KEYS[120];
> >  };
> 
> Looks good.  Thanks for this David.
> 
> Could you please change the name from _KEYS to buf and patch the x86-64
> version as well?

No problems, attached.

Cheers,
Davidm

-- 
David McCullough, david_mccullough@au.securecomputing.com, Ph:+61 734352815
Secure Computing - SnapGear  http://www.uCdot.org http://www.cyberguard.com

[-- Attachment #2: aes2.diff --]
[-- Type: text/plain, Size: 1235 bytes --]

Index: linux-2.6.x/crypto/aes.c
===================================================================
RCS file: linux-2.6.x/crypto/aes.c,v
retrieving revision 1.1.1.6
diff -u -r1.1.1.6 aes.c
--- linux-2.6.x/crypto/aes.c	31 Aug 2005 00:33:03 -0000	1.1.1.6
+++ linux-2.6.x/crypto/aes.c	15 Mar 2006 01:09:37 -0000
@@ -78,12 +78,11 @@
 
 struct aes_ctx {
 	int key_length;
-	u32 E[60];
-	u32 D[60];
+	u32 buf[120];
 };
 
-#define E_KEY ctx->E
-#define D_KEY ctx->D
+#define E_KEY (&ctx->buf[0])
+#define D_KEY (&ctx->buf[60])
 
 static u8 pow_tab[256] __initdata;
 static u8 log_tab[256] __initdata;
Index: linux-2.6.x/arch/x86_64/crypto/aes.c
===================================================================
RCS file: linux-2.6.x/arch/x86_64/crypto/aes.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 aes.c
--- linux-2.6.x/arch/x86_64/crypto/aes.c	31 Aug 2005 00:33:07 -0000	1.1.1.1
+++ linux-2.6.x/arch/x86_64/crypto/aes.c	15 Mar 2006 01:09:37 -0000
@@ -79,12 +79,11 @@
 struct aes_ctx
 {
 	u32 key_length;
-	u32 E[60];
-	u32 D[60];
+	u32 buf[120];
 };
 
-#define E_KEY ctx->E
-#define D_KEY ctx->D
+#define E_KEY (&ctx->buf[0])
+#define D_KEY (&ctx->buf[60])
 
 static u8 pow_tab[256] __initdata;
 static u8 log_tab[256] __initdata;

Re: [2.6 patch] crypto/aes.c: array overrun

[Herbert Xu]

On Wed, Mar 15, 2006 at 11:11:32AM +1000, David McCullough wrote:
> 
> No problems, attached.

Patch applied.  BTW, please attach a Signed-off-by line for your next
patch submission.  Thanks a lot.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt