[PATCH] autocache patche -- resend, updated

[Michael E Brown]

	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:
	-- integrate the mock permfix patch. This checks if cache was created
with UID != current user UID and adjusts permissions appropriately.
	-- Integrate a couple of small fixes regarding permissions.
_prep_install() happens before the above fix, so I have changed two
chown calls from config['chrootuser'] to use config['chrootuid'] (as
well as corresponding group/gid). This ensures that cached yum.conf is
always up-to-date. Previously, a cached yum.conf would never be updated
until the cache expired and was rebuilt. This change means cached builds
will never use wrong configuration.
	-- Use the -l option to tar when creating cache. This prevents tar from
saving /proc or /dev/pts/
	-- cmdline usability: --rebuildcache implies --autocache

Feedback and comments welcome...

Usage:
	Add --autocache to cmdline, or config_opts['use_cache'] = 1 to config
file to enable caching. Cache is automatically built and saved
to /var/lib/mock/root-cache/ on a per-config basis. Cache will be
rebuilt by default every 15 days, or when --rebuildcache is specified on
cmdline. This option is meant to be self-administering and low
overhead. 

--
Michael

[seth vidal]

On Tue, 2006-05-23 at 00:06 -0500, Michael E Brown wrote:
>  	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:
>  	-- integrate the mock permfix patch. This checks if cache was created
>  with UID != current user UID and adjusts permissions appropriately.
>  	-- Integrate a couple of small fixes regarding permissions.
>  _prep_install() happens before the above fix, so I have changed two
>  chown calls from config['chrootuser'] to use config['chrootuid'] (as
>  well as corresponding group/gid). This ensures that cached yum.conf is
>  always up-to-date. Previously, a cached yum.conf would never be updated
>  until the cache expired and was rebuilt. This change means cached builds
>  will never use wrong configuration.
>  	-- Use the -l option to tar when creating cache. This prevents tar from
>  saving /proc or /dev/pts/
>  	-- cmdline usability: --rebuildcache implies --autocache
>  
>  Feedback and comments welcome...
>  
>  Usage:
>  	Add --autocache to cmdline, or config_opts['use_cache'] = 1 to config
>  file to enable caching. Cache is automatically built and saved
>  to /var/lib/mock/root-cache/ on a per-config basis. Cache will be
>  rebuilt by default every 15 days, or when --rebuildcache is specified on
>  cmdline. This option is meant to be self-administering and low
>  overhead. 
>   
merged. Thank you.

-sv

[Enrico Scholz]

Michael_E_Brown(a)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:

* a

  | check_dir(<dir>);
  | operate_in_dir(<dir>);      // tar ...

  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.

* 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.



Enrico

[Michael E Brown]

On Fri, 2006-05-26 at 13:04 +0200, Enrico Scholz wrote:
>  Michael_E_Brown(a)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

[Enrico Scholz]

Michael_E_Brown(a)dell.com (Michael E Brown) writes:

> > * 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. 
/var/lib/mock is writable by everybody in the 'mock' group.


>  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? 
right thing would be, to open the tarball in the helper, check it and
pipe it into tar's stdin. See

                http://ensc.de/mock/mock-0.4-cache.diff

It protects against all symlink attacks until the 'tar' run.


> >   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. 
I mean the following attack

|   tar                                       attacker
| 
| check whether 'etc' is a dir
|                                           rm -rf etc
|                                           ln -s /etc etc
| chdir('etc')
| pack content

I simple do not know, whether the 'chdir' in tar is done in a secure
way (e.g. lstat(dir, &exp_st) && chdir(dir) && stat(".",
&cur_st) &&
compare(exp_st, cur_st))




Enrico