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.
The real bug is in the setting of MSG_TRUNC (which is the only side
effect of your change). So the better fix is:
--- net/core/scm.c.~1~ Tue Jun 15 09:19:30 1999
+++ net/core/scm.c Fri Nov 3 14:18:06 2000
@@ -251,7 +251,7 @@
msg->msg_controllen -= cmlen;
}
}
- if (i < fdnum)
+ if (i < fdnum || (fdnum && fdmax <= 0))
msg->msg_flags |= MSG_CTRUNC;
/*
"David S. Miller" <[email protected]> writes:
> The real bug is in the setting of MSG_TRUNC (which is the only side
> effect of your change). So the better fix is:
>
> --- net/core/scm.c.~1~ Tue Jun 15 09:19:30 1999
> +++ net/core/scm.c Fri Nov 3 14:18:06 2000
> @@ -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.
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]
"David S. Miller" <[email protected]> writes:
> 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.
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.
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.
Later,
David S. Miller
[email protected]
"David S. Miller" <[email protected]> writes:
> 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.
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 :-(
Later,
David S. Miller
[email protected]
"David S. Miller" <[email protected]> writes:
> Yep, this works, applied.
I assume this will go in 2.2.18 right ? Alan ?
Did 2.4.x exhibit the same problem ?
> Please send plain-ASCII patches in the future though :-(
Sorry, I'll remember that...
Phil.
From: Philippe Troin <[email protected]>
Date: 05 Nov 2000 20:07:07 -0800
"David S. Miller" <[email protected]> writes:
> 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.
Later,
David S. Miller
[email protected]