[PATCH 0/2] Security improvements for xt_SYSRQ

[PATCH 0/2] Security improvements for xt_SYSRQ

[John Haxby]

Hello All,

These two patches are something I promised a long time ago and never
actually got around to.

The first patch is just housekeeping: it uses %pI4 and %pI6c for
address formatting in when the debug option is turned on.  Actually,
it's not just housekeeping: the IPv6 sysrq trigger never worked
because of some bad pointer arithmetic.  I also show the destination
IP (or IPv6) address in the debug output because that helps you when
debugging the remote sysrq script for the second patch.

The second patch removed a long standing issue I have had with
xt_SYSRQ.  Someone who doesn't carefully make sure all the hosts with
xt_SYSRQ rules have different, difficult to guess passwords runs the
risk of an attacker replaying a request to every host on the network
"just on the off chance".  The hash now includes the destination IP
address to make this kind of opportunistic hack less likely.

jch

John Haxby (2):
  Use %pI4/%pI6c instead of NIPQUAD_FMT/NIP6_FMT and make IPv6 work
  Improve security for xt_SYSRQ

 extensions/libxt_SYSRQ.man |   17 +++++++++++------
 extensions/xt_SYSRQ.c      |   27 ++++++++++++++++-----------
 2 files changed, 27 insertions(+), 17 deletions(-)

-- 
1.7.5.4

[PATCH 1/2] Use %pI4/%pI6c instead of NIPQUAD_FMT/NIP6_FMT and make IPv6 work

[John Haxby]

IPv6 sysrq never worked because of bad pointer arithmetic.  The
debug output now uses %pI4 nd %pI6c and also records the destination
address as well as the source.
---
 extensions/xt_SYSRQ.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/extensions/xt_SYSRQ.c b/extensions/xt_SYSRQ.c
index d8c0100..723e813 100644
--- a/extensions/xt_SYSRQ.c
+++ b/extensions/xt_SYSRQ.c
@@ -220,9 +220,9 @@ sysrq_tg4(struct sk_buff **pskb, const struct xt_action_param *par)
 
 	if (sysrq_debug)
 		printk(KERN_INFO KBUILD_MODNAME
-		       ": " NIPQUAD_FMT ":%u -> :%u len=%u\n",
-		       NIPQUAD(iph->saddr), htons(udph->source),
-		       htons(udph->dest), len);
+		       ": %pI4:%u -> %pI4:%u len=%u\n",
+		       &iph->saddr, htons(udph->source),
+		       &iph->daddr, htons(udph->dest), len);
 	return sysrq_tg((void *)udph + sizeof(struct udphdr), len);
 }
 
@@ -250,10 +250,10 @@ sysrq_tg6(struct sk_buff **pskb, const struct xt_action_param *par)
 
 	if (sysrq_debug)
 		printk(KERN_INFO KBUILD_MODNAME
-		       ": " NIP6_FMT ":%hu -> :%hu len=%u\n",
-		       NIP6(iph->saddr), ntohs(udph->source),
-		       ntohs(udph->dest), len);
-	return sysrq_tg(udph + sizeof(struct udphdr), len);
+		       ": [%pI6c]:%hu -> [%pI6c]:%hu len=%u\n",
+		       &iph->saddr, ntohs(udph->source),
+		       &iph->daddr, ntohs(udph->dest), len);
+	return sysrq_tg((void *)udph + sizeof(struct udphdr), len);
 }
 #endif
 
-- 
1.7.5.4

[PATCH 2/2] Improve security for xt_SYSRQ

[John Haxby]

The xt_SYSRQ hash now includes the destination IPv4 or IPv6 address
which makes it harder to replay a request to many different machines
in the hope that some of them are using the same password.
---
 extensions/libxt_SYSRQ.man |   17 +++++++++++------
 extensions/xt_SYSRQ.c      |   13 +++++++++----
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/extensions/libxt_SYSRQ.man b/extensions/libxt_SYSRQ.man
index 89878e0..5470066 100644
--- a/extensions/libxt_SYSRQ.man
+++ b/extensions/libxt_SYSRQ.man
@@ -36,6 +36,8 @@ The SYSRQ password can be changed through
 .IP
 echo \-n "password" >/sys/module/xt_SYSRQ/parameters/password
 .PP
+The module will not respond to sysrq requests until a password has been set.
+.PP
 Alternatively, the password may be specified at modprobe time, but this is
 insecure as people can possible see it through ps(1). You can use an option
 line in e.g. /etc/modprobe.d/xt_sysrq if it is properly guarded, that is, only
@@ -60,12 +62,13 @@ password="password"
 seqno="$(date +%s)"
 salt="$(dd bs=12 count=1 if=/dev/urandom 2>/dev/null |
     openssl enc \-base64)"
+ipaddr=10.10.25.7
 req="$sysrq_key,$seqno,$salt"
-req="$req,$(echo \-n "$req,$password" | sha1sum | cut \-c1\-40)"
+req="$req,$(echo \-n "$req,$ipaddr,$password" | sha1sum | cut \-c1\-40)"
 
-echo "$req" | socat stdin udp\-sendto:10.10.25.7:9
+echo "$req" | socat stdin udp\-sendto:$ipaddr:9
 # or
-echo "$req" | netcat \-uw1 10.10.25.7 9
+echo "$req" | netcat \-uw1 $ipaddr 9
 .fi
 .PP
 See the Linux docs for possible sysrq keys. Important ones are: re(b)oot,
@@ -73,8 +76,10 @@ power(o)ff, (s)ync filesystems, (u)mount and remount readonly. More than one
 sysrq key can be used at once, but bear in mind that, for example, a sync may
 not complete before a subsequent reboot or poweroff.
 .PP
+An IPv4 address should have no leading zeros, an IPv6 address should
+be in the form recommended by RFC 5952.  The debug option will log the
+correct form of the address.
+.PP
 The hashing scheme should be enough to prevent mis-use of SYSRQ in many
 environments, but it is not perfect: take reasonable precautions to
-protect your machines.  Most importantly ensure that each machine has a
-different password; there is scant protection for a SYSRQ packet being
-applied to a machine that happens to have the same password.
+protect your machines.
diff --git a/extensions/xt_SYSRQ.c b/extensions/xt_SYSRQ.c
index 723e813..1befcdf 100644
--- a/extensions/xt_SYSRQ.c
+++ b/extensions/xt_SYSRQ.c
@@ -4,6 +4,8 @@
  *
  *	Based upon the ipt_SYSRQ idea by Marek Zalem <marek [at] terminus sk>
  *
+ *	Security additions John Haxby <john.haxby [at] oracle com>
+ *
  *	This program is free software; you can redistribute it and/or
  *	modify it under the terms of the GNU General Public License
  *	version 2 or 3 as published by the Free Software Foundation.
@@ -58,13 +60,13 @@ static char *sysrq_hexdigest;
  * is a series of sysrq requests; <seqno> is a sequence number that must be
  * greater than the last sequence number; <salt> is some random bytes; and
  * <hash> is the hash of everything up to and including the preceding ","
- * together with the password.
+ * together with "<dstaddr>,<password>".
  *
  * For example
  *
  *   salt=$RANDOM
  *   req="s,$(date +%s),$salt"
- *   echo "$req,$(echo -n $req,secret | sha1sum | cut -c1-40)"
+ *   echo "$req,$(echo -n $req,10.10.25.1,secret | sha1sum | cut -c1-40)"
  *
  * You will want a better salt and password than that though :-)
  */
@@ -121,7 +123,6 @@ static unsigned int sysrq_tg(const void *pdata, uint16_t len)
 	sg_init_table(sg, 2);
 #endif
 	sg_set_buf(&sg[0], data, n);
-	strcpy(sysrq_digest_password, sysrq_password);
 	i = strlen(sysrq_digest_password);
 	sg_set_buf(&sg[1], sysrq_digest_password, i);
 	ret = crypto_hash_digest(&desc, sg, n + i, sysrq_digest);
@@ -223,6 +224,7 @@ sysrq_tg4(struct sk_buff **pskb, const struct xt_action_param *par)
 		       ": %pI4:%u -> %pI4:%u len=%u\n",
 		       &iph->saddr, htons(udph->source),
 		       &iph->daddr, htons(udph->dest), len);
+	sprintf(sysrq_digest_password, "%pI4,%s", &iph->daddr, sysrq_password);
 	return sysrq_tg((void *)udph + sizeof(struct udphdr), len);
 }
 
@@ -253,6 +255,7 @@ sysrq_tg6(struct sk_buff **pskb, const struct xt_action_param *par)
 		       ": [%pI6c]:%hu -> [%pI6c]:%hu len=%u\n",
 		       &iph->saddr, ntohs(udph->source),
 		       &iph->daddr, ntohs(udph->dest), len);
+	sprintf(sysrq_digest_password, "%pI6c,%s", &iph->daddr, sysrq_password);
 	return sysrq_tg((void *)udph + sizeof(struct udphdr), len);
 }
 #endif
@@ -340,7 +343,9 @@ static int __init sysrq_crypto_init(void)
 	sysrq_hexdigest = kmalloc(2 * sysrq_digest_size + 1, GFP_KERNEL);
 	if (sysrq_hexdigest == NULL)
 		goto fail;
-	sysrq_digest_password = kmalloc(sizeof(sysrq_password), GFP_KERNEL);
+	sysrq_digest_password =
+	    kmalloc(sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255") +
+		    sizeof(sysrq_password), GFP_KERNEL);
 	if (sysrq_digest_password == NULL)
 		goto fail;
 	do_gettimeofday(&now);
-- 
1.7.5.4

Re: [PATCH 0/2] Security improvements for xt_SYSRQ

[John Haxby]

On 24/06/11 14:14, John Haxby wrote:
> These two patches are something I promised a long time ago and never
> actually got around to.
> [snip]

I'd like to ask, again, if we can include these upstream.

Last time I asked, Patrick McHardy not unreasonably suggested that
rather than being part of iptables (xtables) this would be better as a
standalone module.   This seemed like a good idea: putting the sysrq
handler in an encapsulation socket gets it running in BH context so that
it even works when the machine is mostly wedged and you can still use
iptables for filtering to protect the destination address and port.

This worked beautifully until I tried to extend it to cover IPv6: IPv6
doesn't have an encapsulation socket (and probably shouldn't have).  I
really don't want to provide an IPv4-only solution: that won't even work
today in some environments and it will certainly look rather lame before
long.

So can we have xt_SYSRQ upstream please?  Pretty please? :-)

jch

Re: [PATCH 0/2] Security improvements for xt_SYSRQ

[Jan Engelhardt]

On Friday 2011-06-24 15:30, John Haxby wrote:

>So can we have xt_SYSRQ upstream please?  Pretty please? :-)

Yes; the discussion resurfaced already due to
http://thread.gmane.org/gmane.linux.network/199222/focus=199316
I just need to prepare a patch again.

NF Maintainer availability is somewhat suboptimal, so there is elevated 
wait time.

Re: [PATCH 1/2] Use %pI4/%pI6c instead of NIPQUAD_FMT/NIP6_FMT and make IPv6 work

[Jan Engelhardt]

On Friday 2011-06-24 15:14, John Haxby wrote:

>IPv6 sysrq never worked because of bad pointer arithmetic.  The
>debug output now uses %pI4 nd %pI6c and also records the destination
>address as well as the source.

Xt-a supports >= 2.6.29, but %pI was only introduced in 2.6.32.
So the format subchange will not go into Xt-a. (Though the kernel patch 
I shall produce shall use them then.)

Re: [PATCH 2/2] Improve security for xt_SYSRQ

[Jan Engelhardt]

On Friday 2011-06-24 15:14, John Haxby wrote:

>The xt_SYSRQ hash now includes the destination IPv4 or IPv6 address
>which makes it harder to replay a request to many different machines
>in the hope that some of them are using the same password.

Taken.