[PATCH] cipso: don't follow a NULL pointer when setsockopt() is called

[PATCH] cipso: don't follow a NULL pointer when setsockopt() is called

[Paul Moore]

As reported by Alan Cox, and verified by Lin Ming, when a user
attempts to add a CIPSO option to a socket using the CIPSO_V4_TAG_LOCAL
tag the kernel dies a terrible death when it attempts to follow a NULL
pointer (the skb argument to cipso_v4_validate() is NULL when called via
the setsockopt() syscall).

This patch fixes this by first checking to ensure that the skb is
non-NULL before using it to find the incoming network interface.  In
the unlikely case where the skb is NULL and the user attempts to add
a CIPSO option with the _TAG_LOCAL tag we return an error as this is
not something we want to allow.

A simple reproducer, kindly supplied by Lin Ming, although you must
have the CIPSO DOI #3 configure on the system first or you will be
caught early in cipso_v4_validate():

	#include <sys/types.h>
	#include <sys/socket.h>
	#include <linux/ip.h>
	#include <linux/in.h>
	#include <string.h>

	struct local_tag {
		char type;
		char length;
		char info[4];
	};

	struct cipso {
		char type;
		char length;
		char doi[4];
		struct local_tag local;
	};

	int main(int argc, char **argv)
	{
		int sockfd;
		struct cipso cipso = {
			.type = IPOPT_CIPSO,
			.length = sizeof(struct cipso),
			.local = {
				.type = 128,
				.length = sizeof(struct local_tag),
			},
		};

		memset(cipso.doi, 0, 4);
		cipso.doi[3] = 3;

		sockfd = socket(AF_INET, SOCK_DGRAM, 0);
		#define SOL_IP 0
		setsockopt(sockfd, SOL_IP, IP_OPTIONS,
			&cipso, sizeof(struct cipso));

		return 0;
	}

CC: Lin Ming <mlin@ss.pku.edu.cn>
Reported-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Paul Moore <pmoore@redhat.com>
---
 net/ipv4/cipso_ipv4.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index c48adc5..667c1d4 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -1725,8 +1725,10 @@ int cipso_v4_validate(const struct sk_buff *skb, unsigned char **option)
 		case CIPSO_V4_TAG_LOCAL:
 			/* This is a non-standard tag that we only allow for
 			 * local connections, so if the incoming interface is
-			 * not the loopback device drop the packet. */
-			if (!(skb->dev->flags & IFF_LOOPBACK)) {
+			 * not the loopback device drop the packet. Further,
+			 * there is no legitimate reason for setting this from
+			 * userspace so reject it if skb is NULL. */
+			if (skb == NULL || !(skb->dev->flags & IFF_LOOPBACK)) {
 				err_offset = opt_iter;
 				goto validate_return_locked;
 			}

Re: [PATCH] cipso: don't follow a NULL pointer when setsockopt() is called

[Paul Moore]

On Tuesday, July 17, 2012 05:07:47 PM Paul Moore wrote:
> As reported by Alan Cox, and verified by Lin Ming, when a user
> attempts to add a CIPSO option to a socket using the CIPSO_V4_TAG_LOCAL
> tag the kernel dies a terrible death when it attempts to follow a NULL
> pointer (the skb argument to cipso_v4_validate() is NULL when called via
> the setsockopt() syscall).
> 
> This patch fixes this by first checking to ensure that the skb is
> non-NULL before using it to find the incoming network interface.  In
> the unlikely case where the skb is NULL and the user attempts to add
> a CIPSO option with the _TAG_LOCAL tag we return an error as this is
> not something we want to allow.

...

> CC: Lin Ming <mlin@ss.pku.edu.cn>
> Reported-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Signed-off-by: Paul Moore <pmoore@redhat.com>

Argh, I just realized I forgot to CC the stable folks.

David, if you don't queue this up for them, let me know and I'll resend it to 
stable once it hits Linus' tree.

> ---
>  net/ipv4/cipso_ipv4.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index c48adc5..667c1d4 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1725,8 +1725,10 @@ int cipso_v4_validate(const struct sk_buff *skb,
> unsigned char **option) case CIPSO_V4_TAG_LOCAL:
>  			/* This is a non-standard tag that we only allow for
>  			 * local connections, so if the incoming interface is
> -			 * not the loopback device drop the packet. */
> -			if (!(skb->dev->flags & IFF_LOOPBACK)) {
> +			 * not the loopback device drop the packet. Further,
> +			 * there is no legitimate reason for setting this from
> +			 * userspace so reject it if skb is NULL. */
> +			if (skb == NULL || !(skb->dev->flags & IFF_LOOPBACK)) {
>  				err_offset = opt_iter;
>  				goto validate_return_locked;
>  			}
-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] cipso: don't follow a NULL pointer when setsockopt() is called

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Tue, 17 Jul 2012 17:24:50 -0400

> David, if you don't queue this up for them, let me know and I'll resend it to 
> stable once it hits Linus' tree.

I will be sure to queue it up to -stable when I apply it, as I always
do for appropriate patches.

Re: [PATCH] cipso: don't follow a NULL pointer when setsockopt() is called

[Paul Moore]

On Tuesday, July 17, 2012 02:31:37 PM David Miller wrote:
> From: Paul Moore <pmoore@redhat.com>
> Date: Tue, 17 Jul 2012 17:24:50 -0400
> 
> > David, if you don't queue this up for them, let me know and I'll resend it
> > to stable once it hits Linus' tree.
> 
> I will be sure to queue it up to -stable when I apply it, as I always
> do for appropriate patches.

Okay, thanks.  I just wanted to make sure it hit -stable one way or another.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] cipso: don't follow a NULL pointer when setsockopt() is called

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Tue, 17 Jul 2012 17:07:47 -0400

> As reported by Alan Cox, and verified by Lin Ming, when a user
> attempts to add a CIPSO option to a socket using the CIPSO_V4_TAG_LOCAL
> tag the kernel dies a terrible death when it attempts to follow a NULL
> pointer (the skb argument to cipso_v4_validate() is NULL when called via
> the setsockopt() syscall).
> 
> This patch fixes this by first checking to ensure that the skb is
> non-NULL before using it to find the incoming network interface.  In
> the unlikely case where the skb is NULL and the user attempts to add
> a CIPSO option with the _TAG_LOCAL tag we return an error as this is
> not something we want to allow.
> 
> A simple reproducer, kindly supplied by Lin Ming, although you must
> have the CIPSO DOI #3 configure on the system first or you will be
> caught early in cipso_v4_validate():
 ...
> CC: Lin Ming <mlin@ss.pku.edu.cn>
> Reported-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Signed-off-by: Paul Moore <pmoore@redhat.com>

Applied and queued up for -stable, thanks Paul.