| Message ID | 20081101043737.GA1621@x200.localdomain |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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,
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; -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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
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(+) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html