Diff: for hashing of carp password
Hi,
This diff implements the hashing of the carp password before using it
inside of the Kernel. It fix the problem that passwords like
12345678901234567890 and 12345678901234567890XXX are equal for carp.
But It breaks the compatibility with older Versions. Maybe you need to
increase the protocol number?
bluhm@ have an other idea to solve this problem: ifconfig could XOR
every 20 Byte long Chuck of the Passwort. This would not break the
compatibility of setups with less than 20 char password.
Just tell me every thing thats wrong with that diff and I will fix it.
bye,
Jan
Index: ifconfig.c
===
RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
retrieving revision 1.264
diff -u -p -r1.264 ifconfig.c
--- ifconfig.c 31 May 2013 19:56:06 - 1.264
+++ ifconfig.c 2 Jul 2013 10:12:53 -
@@ -101,6 +101,7 @@
#include string.h
#include unistd.h
#include ifaddrs.h
+#include sha1.h
#include brconfig.h
@@ -3383,6 +3384,7 @@ void
setcarp_passwd(const char *val, int d)
{
struct carpreq carpr;
+ SHA1_CTX sha;
bzero(carpr, sizeof(struct carpreq));
ifr.ifr_data = (caddr_t)carpr;
@@ -3390,8 +3392,9 @@ setcarp_passwd(const char *val, int d)
if (ioctl(s, SIOCGVH, (caddr_t)ifr) == -1)
err(1, SIOCGVH);
- /* XXX Should hash the password into the key here, perhaps? */
- strlcpy((char *)carpr.carpr_key, val, CARP_KEY_LEN);
+ SHA1Init(sha);
+ SHA1Update(sha, val, strlen(val));
+ SHA1Final((char *)carpr.carpr_key, sha);
if (ioctl(s, SIOCSVH, (caddr_t)ifr) == -1)
err(1, SIOCSVH);
Re: Diff: for hashing of carp password
On Tue, Jul 02, 2013 at 12:27:54PM +0200, Jan Klemkow wrote: Hi, This diff implements the hashing of the carp password before using it inside of the Kernel. It fix the problem that passwords like 12345678901234567890 and 12345678901234567890XXX are equal for carp. But It breaks the compatibility with older Versions. Maybe you need to increase the protocol number? bluhm@ have an other idea to solve this problem: ifconfig could XOR every 20 Byte long Chuck of the Passwort. This would not break the compatibility of setups with less than 20 char password. I would not do anyhing about that as it breaks compatibility without gain. Perhaps the XXX comment should be removed. What is actually wrong is that long passwords cannot be replaced completely with short passwords. ioctl(SIOCGVH) fills the carpr_key with the old value. strlcpy() overwrites only the beginning of the key. Jan, can you please test this? ok? bluhm Index: sbin/ifconfig/ifconfig.c === RCS file: /data/mirror/openbsd/cvs/src/sbin/ifconfig/ifconfig.c,v retrieving revision 1.264 diff -u -p -u -p -r1.264 ifconfig.c --- sbin/ifconfig/ifconfig.c31 May 2013 19:56:06 - 1.264 +++ sbin/ifconfig/ifconfig.c2 Jul 2013 11:01:38 - @@ -3390,7 +3390,7 @@ setcarp_passwd(const char *val, int d) if (ioctl(s, SIOCGVH, (caddr_t)ifr) == -1) err(1, SIOCGVH); - /* XXX Should hash the password into the key here, perhaps? */ + bzero(carpr.carpr_key, CARP_KEY_LEN); strlcpy((char *)carpr.carpr_key, val, CARP_KEY_LEN); if (ioctl(s, SIOCSVH, (caddr_t)ifr) == -1)
Re: Diff: for hashing of carp password
Your change is working. I've test this Bug with following case. FIRST: router-a: cat /etc/hostname.carp0 10.0.0.1 vhid 1 carpnodes 1:0,2:100 pass aaa balancing ip router-b: cat /etc/hostname.carp0 10.0.0.1 vhid 1 carpnodes 1:0,2:100 pass bbb balancing ip THAN: router-a: cat /etc/hostname.carp0 10.0.0.1 vhid 1 carpnodes 1:0,2:100 pass a balancing ip router-b: cat /etc/hostname.carp0 10.0.0.1 vhid 1 carpnodes 1:0,2:100 pass a balancing ip After this I get an carp0: incorrect hash. With our diffs, it is fixed. But, I would prefer my diff, cause your Diff don't fix the length problem. In my opinion it is a serious Bug, that password parts over 20 character are ignored. Even for the Hash-MAC code in carp, it looks not very secure, if we make shifting with a lot of zeros in cases of an small password. I would prefer the hashing version. bye, Jan On Tue, Jul 02, 2013 at 01:04:49PM +0200, Alexander Bluhm wrote: On Tue, Jul 02, 2013 at 12:27:54PM +0200, Jan Klemkow wrote: Hi, This diff implements the hashing of the carp password before using it inside of the Kernel. It fix the problem that passwords like 12345678901234567890 and 12345678901234567890XXX are equal for carp. But It breaks the compatibility with older Versions. Maybe you need to increase the protocol number? bluhm@ have an other idea to solve this problem: ifconfig could XOR every 20 Byte long Chuck of the Passwort. This would not break the compatibility of setups with less than 20 char password. I would not do anyhing about that as it breaks compatibility without gain. Perhaps the XXX comment should be removed. What is actually wrong is that long passwords cannot be replaced completely with short passwords. ioctl(SIOCGVH) fills the carpr_key with the old value. strlcpy() overwrites only the beginning of the key. Jan, can you please test this? ok? bluhm Index: sbin/ifconfig/ifconfig.c === RCS file: /data/mirror/openbsd/cvs/src/sbin/ifconfig/ifconfig.c,v retrieving revision 1.264 diff -u -p -u -p -r1.264 ifconfig.c --- sbin/ifconfig/ifconfig.c 31 May 2013 19:56:06 - 1.264 +++ sbin/ifconfig/ifconfig.c 2 Jul 2013 11:01:38 - @@ -3390,7 +3390,7 @@ setcarp_passwd(const char *val, int d) if (ioctl(s, SIOCGVH, (caddr_t)ifr) == -1) err(1, SIOCGVH); - /* XXX Should hash the password into the key here, perhaps? */ + bzero(carpr.carpr_key, CARP_KEY_LEN); strlcpy((char *)carpr.carpr_key, val, CARP_KEY_LEN); if (ioctl(s, SIOCSVH, (caddr_t)ifr) == -1)
Re: Diff: for hashing of carp password
On Tue, Jul 02, 2013 at 02:20:05PM +0200, Jan Klemkow wrote: Your change is working. I've test this Bug with following case. FIRST: router-a: cat /etc/hostname.carp0 10.0.0.1 vhid 1 carpnodes 1:0,2:100 pass aaa balancing ip router-b: cat /etc/hostname.carp0 10.0.0.1 vhid 1 carpnodes 1:0,2:100 pass bbb balancing ip THAN: router-a: cat /etc/hostname.carp0 10.0.0.1 vhid 1 carpnodes 1:0,2:100 pass a balancing ip router-b: cat /etc/hostname.carp0 10.0.0.1 vhid 1 carpnodes 1:0,2:100 pass a balancing ip After this I get an carp0: incorrect hash. With our diffs, it is fixed. But, I would prefer my diff, cause your Diff don't fix the length problem. In my opinion it is a serious Bug, that password parts over 20 character are ignored. Even for the Hash-MAC code in carp, it looks not very secure, if we make shifting with a lot of zeros in cases of an small password. Thanks for testing. I would prefer the hashing version. I don't. If you want to get a secret into the kernel, ifconfig should just store it into the kernel. Your hash would obfuscate the algorithm and brake compatibility. What could be done is to implement a hexkey like in wpakey. This would allow to use the whole 20 byte information of the passphrase. Although I don't think that it is worth it. bluhm bye, Jan On Tue, Jul 02, 2013 at 01:04:49PM +0200, Alexander Bluhm wrote: On Tue, Jul 02, 2013 at 12:27:54PM +0200, Jan Klemkow wrote: Hi, This diff implements the hashing of the carp password before using it inside of the Kernel. It fix the problem that passwords like 12345678901234567890 and 12345678901234567890XXX are equal for carp. But It breaks the compatibility with older Versions. Maybe you need to increase the protocol number? bluhm@ have an other idea to solve this problem: ifconfig could XOR every 20 Byte long Chuck of the Passwort. This would not break the compatibility of setups with less than 20 char password. I would not do anyhing about that as it breaks compatibility without gain. Perhaps the XXX comment should be removed. What is actually wrong is that long passwords cannot be replaced completely with short passwords. ioctl(SIOCGVH) fills the carpr_key with the old value. strlcpy() overwrites only the beginning of the key. Jan, can you please test this? ok? bluhm Index: sbin/ifconfig/ifconfig.c === RCS file: /data/mirror/openbsd/cvs/src/sbin/ifconfig/ifconfig.c,v retrieving revision 1.264 diff -u -p -u -p -r1.264 ifconfig.c --- sbin/ifconfig/ifconfig.c31 May 2013 19:56:06 - 1.264 +++ sbin/ifconfig/ifconfig.c2 Jul 2013 11:01:38 - @@ -3390,7 +3390,7 @@ setcarp_passwd(const char *val, int d) if (ioctl(s, SIOCGVH, (caddr_t)ifr) == -1) err(1, SIOCGVH); - /* XXX Should hash the password into the key here, perhaps? */ + bzero(carpr.carpr_key, CARP_KEY_LEN); strlcpy((char *)carpr.carpr_key, val, CARP_KEY_LEN); if (ioctl(s, SIOCSVH, (caddr_t)ifr) == -1)
