[PATCH] Fix ESP SA loading (by default)

[PATCH] Fix ESP SA loading (by default)

[Alexey Dobriyan]

digest_null algorithm is now mandatory for ESP.

Steps to reproduce:

	kernel with CONFIG_CRYPTO_NULL=n

	#!/usr/sbin/setkey -f
	flush;
	spdflush;
	add 192.168.0.1 192.168.0.42 esp 15701 -E 3des-cbc "123456789012123456789012";

This will successfully create ESP SA.

Now, apply commit 38320c70d282be1997a5204c7c7fe14c3aa6bfaa aka
"[IPSEC]: Use crypto_aead and authenc in ESP" and ESP SAs won't be created.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 Ewwww, such a cool bug turned out to be configuration issue!

 And I was thinking why on earth why Debian 2.6.26 based kernel is OK,
 but 2.6.25-rc1 (!) fails. Ditto for minimalistic config for testing with KVM.
 Not mentioning Debian's gcc creating references to __ucmdhowitiscalled up
 and including to 2.6.18 and screwing bisection hard way.

 Now that I passed first IPsec tutorial, allow me to start netns XFRM work :^)

 net/ipv4/Kconfig |    1 +
 net/ipv6/Kconfig |    1 +
 2 files changed, 2 insertions(+)

--- a/net/ipv4/Kconfig
+++ b/net/ipv4/Kconfig
@@ -346,6 +346,7 @@ config INET_ESP
 	select CRYPTO_AUTHENC
 	select CRYPTO_HMAC
 	select CRYPTO_MD5
+	select CRYPTO_NULL
 	select CRYPTO_CBC
 	select CRYPTO_SHA1
 	select CRYPTO_DES
--- a/net/ipv6/Kconfig
+++ b/net/ipv6/Kconfig
@@ -86,6 +86,7 @@ config INET6_ESP
 	select CRYPTO_AUTHENC
 	select CRYPTO_HMAC
 	select CRYPTO_MD5
+	select CRYPTO_NULL
 	select CRYPTO_CBC
 	select CRYPTO_SHA1
 	select CRYPTO_DES

Re: [PATCH] Fix ESP SA loading (by default)

[Herbert Xu]

On Sat, Nov 01, 2008 at 07:37:37AM +0300, Alexey Dobriyan wrote:
> digest_null algorithm is now mandatory for ESP.
> 
> Steps to reproduce:
> 
> 	kernel with CONFIG_CRYPTO_NULL=n
> 
> 	#!/usr/sbin/setkey -f
> 	flush;
> 	spdflush;
> 	add 192.168.0.1 192.168.0.42 esp 15701 -E 3des-cbc "123456789012123456789012";

You do realise that this is totally insecure against man-in-the-
middle attacks, right?

We don't make ESP depend on every single algorithm out there.
It is the user's responsibility to turn on esoteric choices.
IMHO the use of encryption without authentication is definitely
non-standard.

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: [PATCH] Fix ESP SA loading (by default)

[Alexey Dobriyan]

On Sat, Nov 01, 2008 at 01:04:58PM +0800, Herbert Xu wrote:
> On Sat, Nov 01, 2008 at 07:37:37AM +0300, Alexey Dobriyan wrote:
> > digest_null algorithm is now mandatory for ESP.
> > 
> > Steps to reproduce:
> > 
> > 	kernel with CONFIG_CRYPTO_NULL=n
> > 
> > 	#!/usr/sbin/setkey -f
> > 	flush;
> > 	spdflush;
> > 	add 192.168.0.1 192.168.0.42 esp 15701 -E 3des-cbc "123456789012123456789012";
> 
> You do realise that this is totally insecure against man-in-the-
> middle attacks, right?

It's a cut down example. This won't work either:

#!/usr/sbin/setkey -f
flush;
spdflush;

add 192.168.0.1 192.168.0.42 ah 15700 -A hmac-md5 "1234567890123456";
add 192.168.0.1 192.168.0.42 esp 15701 -E 3des-cbc "123456789012123456789012";

add 192.168.0.42 192.168.0.1 ah 24500 -A hmac-md5 "1234567890123456";
add 192.168.0.42 192.168.0.1 esp 24501 -E 3des-cbc "123456789012123456789012";

spdadd 192.168.0.42 192.168.0.1 any -P out ipsec
	esp/transport//require
	ah/transport//require;

spdadd 192.168.0.1 192.168.0.42 any -P in ipsec
	esp/transport//require
	ah/transport//require;

Re: [PATCH] Fix ESP SA loading (by default)

[Herbert Xu]

On Sat, Nov 01, 2008 at 02:38:35PM +0300, Alexey Dobriyan wrote:
>
> It's a cut down example. This won't work either:

OK, in that case I suggest we make AH depend on the null algorithms
instead.  This is because very few people use AH these days and only
they need this.

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: [PATCH] Fix ESP SA loading (by default)

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 1 Nov 2008 19:50:43 +0800

> On Sat, Nov 01, 2008 at 02:38:35PM +0300, Alexey Dobriyan wrote:
> >
> > It's a cut down example. This won't work either:
> 
> OK, in that case I suggest we make AH depend on the null algorithms
> instead.  This is because very few people use AH these days and only
> they need this.

One could even argue that we should blow away all of those select
statements.

Re: [PATCH] Fix ESP SA loading (by default)

[Alexey Dobriyan]

On Sat, Nov 01, 2008 at 09:33:15PM -0700, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Sat, 1 Nov 2008 19:50:43 +0800
> 
> > On Sat, Nov 01, 2008 at 02:38:35PM +0300, Alexey Dobriyan wrote:
> > >
> > > It's a cut down example. This won't work either:
> > 
> > OK, in that case I suggest we make AH depend on the null algorithms
> > instead.  This is because very few people use AH these days and only
> > they need this.
> 
> One could even argue that we should blow away all of those select
> statements.

Keep in mind that the only error message is "line N: returned (null)"
from setkey(8) or something like that and no SA created.

It took me full printk session to realize what's going on.

Re: [PATCH] Fix ESP SA loading (by default)

[Herbert Xu]

On Mon, Nov 03, 2008 at 03:16:43AM +0300, Alexey Dobriyan wrote:
>
> Keep in mind that the only error message is "line N: returned (null)"
> from setkey(8) or something like that and no SA created.
> 
> It took me full printk session to realize what's going on.

As our error passing really sucks, I'm happy to accept a patch
to crypto_alloc_tfm which prints out a message if it fails.

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: [PATCH] Fix ESP SA loading (by default)

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 3 Nov 2008 09:04:31 +0800

> On Mon, Nov 03, 2008 at 03:16:43AM +0300, Alexey Dobriyan wrote:
> >
> > Keep in mind that the only error message is "line N: returned (null)"
> > from setkey(8) or something like that and no SA created.
> > 
> > It took me full printk session to realize what's going on.
> 
> As our error passing really sucks, I'm happy to accept a patch
> to crypto_alloc_tfm which prints out a message if it fails.

As we've discussed several times it's not "passing" errors
that sucks, it's the fact that we use the same traditional
UNIX error codes for a thousand different errors. :-)

I really think we should explore the idea where the current
process can get tagged with a string when an error is going
to be returned.  Something like:

	const char *error_desc;

in the task_struct.

So when you return an error, you also can mark the task with
some descriptive text that describes what is wrong.

A task is guarenteed that when an error returns from a system
call and the very next system call they make is "sys_get_error"
or whatever we'll call it, they will the correct value of
current->error_desc

This way you don't just get "-EINVAL" returned from a
complicated IPSEC configuration operation request.

Re: [PATCH] Fix ESP SA loading (by default)

[Herbert Xu]

On Wed, Nov 05, 2008 at 01:31:48AM -0800, David Miller wrote:
>
> As we've discussed several times it's not "passing" errors
> that sucks, it's the fact that we use the same traditional
> UNIX error codes for a thousand different errors. :-)

Right.

> I really think we should explore the idea where the current
> process can get tagged with a string when an error is going
> to be returned.  Something like:
> 
> 	const char *error_desc;
> 
> in the task_struct.

Yes this will be heaps better than what we have now.

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: [PATCH] Fix ESP SA loading (by default)

[Alexey Dobriyan]

On Wed, Nov 05, 2008 at 01:31:48AM -0800, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Mon, 3 Nov 2008 09:04:31 +0800
> 
> > On Mon, Nov 03, 2008 at 03:16:43AM +0300, Alexey Dobriyan wrote:
> > >
> > > Keep in mind that the only error message is "line N: returned (null)"
> > > from setkey(8) or something like that and no SA created.
> > > 
> > > It took me full printk session to realize what's going on.
> > 
> > As our error passing really sucks, I'm happy to accept a patch
> > to crypto_alloc_tfm which prints out a message if it fails.
> 
> As we've discussed several times it's not "passing" errors
> that sucks, it's the fact that we use the same traditional
> UNIX error codes for a thousand different errors. :-)
> 
> I really think we should explore the idea where the current
> process can get tagged with a string when an error is going
> to be returned.  Something like:
> 
> 	const char *error_desc;
> 
> in the task_struct.
> 
> So when you return an error, you also can mark the task with
> some descriptive text that describes what is wrong.

rmmod in between and error_desc points to garbage. But that's for
somebody who is going to implement this. :-)

> A task is guarenteed that when an error returns from a system
> call and the very next system call they make is "sys_get_error"
> or whatever we'll call it, they will the correct value of
> current->error_desc
> 
> This way you don't just get "-EINVAL" returned from a
> complicated IPSEC configuration operation request.

It was ENOENT from crypto_alg_mod_lookup(), actually.

I think liberal printk additions are the way to go.

Re: [PATCH] Fix ESP SA loading (by default)

[David Miller]

From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Wed, 5 Nov 2008 12:54:28 +0300

> It was ENOENT from crypto_alg_mod_lookup(), actually.
> 
> I think liberal printk additions are the way to go.

Saying "type dmesg" to figure out why an IPSEC configuration fails
is not very user friendly, even for application developers.

About the rmmod thing, we can do something similar to how
we handle nesting of irq_regs().  Ie. there are things that
push and pop the stack of ->error_desc.  rmmod operations
could be one of those things

Re: [PATCH] Fix ESP SA loading (by default)

[Patrick McHardy]

David Miller wrote:
> From: Alexey Dobriyan <adobriyan@gmail.com>
> Date: Wed, 5 Nov 2008 12:54:28 +0300
> 
>> It was ENOENT from crypto_alg_mod_lookup(), actually.
>>
>> I think liberal printk additions are the way to go.
> 
> Saying "type dmesg" to figure out why an IPSEC configuration fails
> is not very user friendly, even for application developers.

Indeed. I also prefer the *error_desc, for netlink errors we could
then include the message in an extended error attribute or something
like that. Probably its even a necessity for userspace to be able to
associate errors properly in case multiple messages are sent at once.

> About the rmmod thing, we can do something similar to how
> we handle nesting of irq_regs().  Ie. there are things that
> push and pop the stack of ->error_desc.  rmmod operations
> could be one of those things

Alternatively we could simply strdup the error message (or copy it
to a reserved area to avoid running into memory allocation errors).
That avoids having to mess with foreign task structs when
unloading modules.

Re: [PATCH] Fix ESP SA loading (by default)

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 05 Nov 2008 11:03:18 +0100

> David Miller wrote:
> > About the rmmod thing, we can do something similar to how
> > we handle nesting of irq_regs().  Ie. there are things that
> > push and pop the stack of ->error_desc.  rmmod operations
> > could be one of those things
> 
> Alternatively we could simply strdup the error message (or copy it
> to a reserved area to avoid running into memory allocation errors).
> That avoids having to mess with foreign task structs when
> unloading modules.

Since the error message is "const char *" I don't see why
you'd need to strdup() it ever, just the pointer is fine.

Well, I guess we could run into problems with module unloading
and the strings being from that module hmmm...

Re: [PATCH] Fix ESP SA loading (by default)

[Patrick McHardy]

David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Wed, 05 Nov 2008 11:03:18 +0100
> 
>> David Miller wrote:
>>> About the rmmod thing, we can do something similar to how
>>> we handle nesting of irq_regs().  Ie. there are things that
>>> push and pop the stack of ->error_desc.  rmmod operations
>>> could be one of those things
>> Alternatively we could simply strdup the error message (or copy it
>> to a reserved area to avoid running into memory allocation errors).
>> That avoids having to mess with foreign task structs when
>> unloading modules.
> 
> Since the error message is "const char *" I don't see why
> you'd need to strdup() it ever, just the pointer is fine.
> 
> Well, I guess we could run into problems with module unloading
> and the strings being from that module hmmm...

Yes, thats what I thought Alexey was referring to.