[PATCH] autocache patche -- resend, updated
Michael E Brown
Michael_E_Brown at dell.com
Mon May 29 20:46:14 UTC 2006
On Fri, 2006-05-26 at 13:04 +0200, Enrico Scholz wrote:
> Michael_E_Brown at dell.com (Michael E Brown) writes:
>
> > Here is a resend/consolidation of the autocache patches from last week.
> > I have combined the patches sent last week. It now defaults to gzip
> > format. I have also made the following fixes:
>
> > + check_dir_allowed (rootsdir, argv[2]);
>
> When you do security checks, then please make them correctly. Everything
> else will give a wrong feeling about security:
Enrico,
I welcome the feedback, thanks much.
>
> * a
>
> | check_dir(<dir>);
> | operate_in_dir(<dir>); // tar ...
Dir always has to be under /var/lib/mock/. It is not possible for
unprivileged users to create symlinks here.
I suppose the symlink attack could be done by somebody in the mock
group.
1) mock -r CFG some.src.rpm #to force creation of builddir with
ownership by me.
2) cd /var/lib/mock/CFG/root/builddir/
3) ln -s / blastroot
4) mock-helper unpack /var/lib/mock/CFG/root/builddir/blastroot
my-bad.tar.gz
I believe that the fix for this would be to add a check to ensure that
the tarball is always sourced from /var/lib/mock/root-cache/ and that
the perms are correct. Feedback?
>
> opens always a window for symlink attacks. Better do
>
> | chdirSafe(<dir);
> | operate_in_dir("."); // tar ...
>
> The security of 'tar' operations is another question; extraction can
> be made secure by extracting into a private dir and doing an atomic
> rename(2) then. ATM, I do not see a way how to implement tarball
> creation securely.
Make sure we are always tarring /var/lib/mock/CFG/root and always place
the tar under /var/lib/mock/root-cache.
Users in group 'mock' would not have sufficient perms to do anything
harmful under this dir.
>
> * you should check permissions of the tar-file; else, user can provide
> e.g. a public writable /dev/kmem device in the tarball
>
> Ok, the question remains whether such checks (inclusive check_dir_allowed())
> are needed overall. 'mock' gives practically root rights to everybody in the
> mock.
Personally, I think they should be there. It makes things much more
difficult, though.
Suggested resolution:
For a patch, I could change both pack and unpack to only take the file
name and config name. It would then always look for or create the tar
under /var/lib/mock/root-cache/, and it would only ever tar or untar
to /var/lib/mock/CFG/root.
--
Michael
More information about the Fedora-buildsys-list
mailing list