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
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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: {
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....
Jivin [email protected] 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, [email protected], Ph:+61 734352815
Secure Computing - SnapGear http://www.uCdot.org http://www.cyberguard.com
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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, [email protected], Ph:+61 734352815
Secure Computing - SnapGear http://www.uCdot.org http://www.cyberguard.com
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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt