On Tue, 18 Sep 2012 22:18:03 +0200
Christian Seiler <christ...@iwakd.de> wrote:
> Hi,
>
> Just a heads up:
>
> > Since the if uses >=, the - 1 is not needed and the MAXFDS'th
> > entry in the fds array can be used.
>
> This was from part of one of my patches regarding lxc-attach and it is
> NOT an off-by-one error, it is meant to be this way. The problem is
> that the array has to be traversed later (both for completing the
> attach operation or aborting it), see
> lxc_cgroup_(dispose|finish)_attach:
>
> | for (; *fds >= 0; fds++) {
>
> This loop will result in a potential buffer overflow with your change.
>
> Obviously, you could rewrite it to also check against the offset of
> the beginning of the array and compare that to MAXFDS in those loops,
> but that makes the traversal loop more complicated to read and
> whether the maximum number of cgroup controllers mounted that
> lxc-attach supports is 255 or 256 shouldn't matter much. On the other
> hand, this means the overflow wouldn't occur in practice - however,
> if at some later point somebody should change MAXFDS to 16 to save
> some memory, for example on embedded systems, then this could lead to
> undefined behaviour - and even potential security wholes if
> lxc-attach is installed as setuid root.
>
> But because this appears to be something that needs clarification,
> perhaps you could change your patch to just add a comment explaining
> the situation?
Ahh, yes I see. I think the name MAXFDS threw me off, its really a -1
terminated array with MAXFDS - 1 valid slots.
Do you think mallocing an fd_set and using FD_SET() and friends
would be better? The (dispose|finish) loops would visit FD_SETSIZE bits
with an FD_ISSET() test, which is more work than you have currently
with the early out, but we would probably save on the initialization
with FD_ZERO(). I don't know if lxc_cgroup_(dispose|finish)_attach is
performance critical.
Or I can just add a comment :)
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Lxc-devel mailing list
Lxc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lxc-devel
