2.2.x BUG & PATCH: recvmsg() does not check msg_controllen correctly

PATCH Philippe Troin 2 participants 10 messages Nov 3, 2000 – Nov 6, 2000

Subsystems (2)

Networking Sockets

lore

Share & tools

lore

Philippe Troin

author

2000-11-03 20:46 UTC #0 lore raw

I found this in all 2.2.x kernels, and it might possibly be present in 2.4.x too...

When receiving file descriptors via recvmsg(), scm_detach_fds() in net/core/scm.c can overflow user space data at msg_control if msg_controllen is less than sizeof(struct cmsghdr).

This is a security problem.

Attached is a patch to fix the problem and a little program to demonstrate the problem.

Phil.

net/core/scm.c +8 -0

diff -ruN linux.orig/net/core/scm.c linux/net/core/scm.c
@@ -216,6 +216,13 @@
 	int *cmfptr;
 	int err = 0, i;
 
+	/* Handle the case when we don't even have enough room for the header.
+	   Fixed Oct 29 2000 by Philippe Troin <[email protected]> */
+	if (msg->msg_controllen < sizeof(struct cmsghdr)) {
+		msg->msg_flags |= MSG_CTRUNC;
+		goto out;
+	}       
+
 	if (fdnum < fdmax)
 		fdmax = fdnum;

@@ -258,6 +265,7 @@
 	 * All of the files that fit in the message have had their
 	 * usage counts incremented, so we just free the list.
 	 */
+out:
 	__scm_destroy(scm);
 }

David S. Miller

2000-11-03 22:34 UTC #1 lore raw

The real bug is in the setting of MSG_TRUNC (which is the only side effect of your change). So the better fix is:

core/scm.c renamed from core/scm.c.~1~ +1 -1

@@ -251,7 +251,7 @@
 			msg->msg_controllen -= cmlen;
 		}
 	}
-	if (i < fdnum)
+	if (i < fdnum || (fdnum && fdmax <= 0))
 		msg->msg_flags |= MSG_CTRUNC;
 
 	/*

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Philippe Troin

author

2000-11-04 00:18 UTC #2 lore raw

In reply to Philippe Troin

Quoted 6 lines · David S. Miller

The real bug is in the setting of MSG_TRUNC (which is the only side effect of your change). So the better fix is:

core/scm.c line 257

Quoted 8 lines

core/scm.c renamed from core/scm.c.~1~ +1 -1

@@ -251,7 +251,7 @@
 			msg->msg_controllen -= cmlen;
 		}
 	}
-	if (i < fdnum)
+	if (i < fdnum || (fdnum && fdmax <= 0))
 		msg->msg_flags |= MSG_CTRUNC;
 
 	/*

Mmmh, no, if fdmax <= 0 (which happens when msg_controllen < sizeof(struct cmsghdr)), then alls fds are passed, eventually clobbering past ((char*)(msg_control)+m_controllen).

Run the little test case if you're not convinced... I stand by my patch :-)

Phil.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

David S. Miller

2000-11-04 00:53 UTC #3 lore raw

From: Philippe Troin <[email protected]>
Date: 03 Nov 2000 16:17:53 -0800

Mmmh, no, if fdmax <= 0 (which happens when msg_controllen < sizeof(struct cmsghdr)), then alls fds are passed, eventually clobbering past ((char*)(msg_control)+m_controllen).

Run the little test case if you're not convinced... I stand by my patch :-)

If fdmax <= 0, no iterations of the "for (i=0" loop will run. 'i' will therefore be left equal to zero. Therefore the next bit of code writing in the SOL_SOCKET/SCM_RIGHTS/etc. values will not run.

Next comes the test I modified, which will set MSG_CTRUNC.

Next scm_destroy(scm) is called which frees the list (this has to get called and is why I say your patch wasn't correct).

So where in this code are all the fds passed to the user in this case? I don't care what it actually does, I want to be shown why because as far as I see it doesn't do what you say it does.

Later,
David S. Miller
[email protected]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Philippe Troin

author

2000-11-04 03:53 UTC #4 lore raw

In reply to Philippe Troin

Quoted 15 lines · David S. Miller

From: Philippe Troin <[email protected]>
Date: 03 Nov 2000 16:17:53 -0800
Mmmh, no, if fdmax <= 0 (which happens when msg_controllen < sizeof(struct cmsghdr)), then alls fds are passed, eventually clobbering past ((char*)(msg_control)+m_controllen).
Run the little test case if you're not convinced... I stand by my patch :-)
If fdmax <= 0, no iterations of the "for (i=0" loop will run. 'i' will therefore be left equal to zero. Therefore the next bit of code writing in the SOL_SOCKET/SCM_RIGHTS/etc. values will not run.

Quoted 8 lines

Next comes the test I modified, which will set MSG_CTRUNC.
Next scm_destroy(scm) is called which frees the list (this has to get called and is why I say your patch wasn't correct).
So where in this code are all the fds passed to the user in this case? I don't care what it actually does, I want to be shown why because as far as I see it doesn't do what you say it does.

Well, you should have ran my little test case...
No really :-)

All your explanations make sense.
When I re-read the code in scm.c, I had trouble figuring out why it did not work before my patch and why it worked after...

Here it is:

	int fdmax = (msg->msg_controllen - sizeof(struct cmsghdr))/sizeof(int);

But, msg->msg_controllen is of type __kernel_size_t, which is unsigned int (on i386).
Which means that if msg_controllen < sizeof(struct cmsghdr), then fdmax is somewhere around 0x40000000, courtesy of the int->unsigned int C promotion...
Ooops...

Yes I agree, mixing signed and unsigned arithmetic is evil... Doesn't gcc have a flag for unsafe signed/unsigned mixtures ?

Would you consider this patch (or a variant) for inclusion ?

Phil.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

David S. Miller

2000-11-04 05:07 UTC #5 lore raw

From: Philippe Troin <[email protected]>
Date: 03 Nov 2000 19:53:04 -0800

Yes I agree, mixing signed and unsigned arithmetic is evil... Doesn't gcc have a flag for unsafe signed/unsigned mixtures ?

Would you consider this patch (or a variant) for inclusion ?

I would accept a patch which made the code set fdmax <= 0 when (msg->msg_controllen < (sizeof(struct cmsghdr) + sizeof(int))) because it is the sole reason this bug exists at all.

Philippe Troin

author

2000-11-05 02:41 UTC #6 lore raw

In reply to Philippe Troin

Quoted 11 lines · David S. Miller

From: Philippe Troin <[email protected]>
Date: 03 Nov 2000 19:53:04 -0800
Yes I agree, mixing signed and unsigned arithmetic is evil... Doesn't gcc have a flag for unsafe signed/unsigned mixtures ?
Would you consider this patch (or a variant) for inclusion ?
I would accept a patch which made the code set fdmax <= 0 when (msg->msg_controllen < (sizeof(struct cmsghdr) + sizeof(int))) because it is the sole reason this bug exists at all.

How about this one ?

Phil.

net/core/scm.c +6 -2

diff -ruN linux.orig/net/core/scm.c linux/net/core/scm.c
@@ -210,12 +210,16 @@
 {
 	struct cmsghdr *cm = (struct cmsghdr*)msg->msg_control;
 
-	int fdmax = (msg->msg_controllen - sizeof(struct cmsghdr))/sizeof(int);
+	int fdmax = 0;
 	int fdnum = scm->fp->count;
 	struct file **fp = scm->fp->fp;
 	int *cmfptr;
 	int err = 0, i;
 
+	if (msg->msg_controllen > sizeof(struct cmsghdr))
+		fdmax = (msg->msg_controllen - sizeof(struct cmsghdr))
+			/sizeof(int);
+
 	if (fdnum < fdmax)
 		fdmax = fdnum;

@@ -251,7 +255,7 @@
 			msg->msg_controllen -= cmlen;
 		}
 	}
-	if (i < fdnum)
+	if (i < fdnum || (fdnum && fdmax == 0))
 		msg->msg_flags |= MSG_CTRUNC;
 
 	/*

David S. Miller

2000-11-06 03:47 UTC #7 lore raw

From: Philippe Troin <[email protected]>
Date: 04 Nov 2000 18:40:30 -0800

How about this one ?

Yep, this works, applied.

Please send plain-ASCII patches in the future though :-(

Philippe Troin

author

2000-11-06 04:07 UTC #8 lore raw

In reply to Philippe Troin

Quoted 1 line · David S. Miller

Yep, this works, applied.

I assume this will go in 2.2.18 right ? Alan ?
Did 2.4.x exhibit the same problem ?

Quoted 1 line

Please send plain-ASCII patches in the future though :-(

Sorry, I'll remember that...

Phil.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

David S. Miller

2000-11-06 04:12 UTC #9 lore raw

From: Philippe Troin <[email protected]>
Date: 05 Nov 2000 20:07:07 -0800

Quoted 1 line · David S. Miller

Yep, this works, applied.

I assume this will go in 2.2.18 right ? Alan ?

It has been sent to Alan already for inclusion.

Did 2.4.x exhibit the same problem ?

Yes, patch queued for when Linus makes a the next pre-patch.