Maxim Sobolev wrote:
> 
> Maxim Sobolev wrote:
> >
> > Bruce Evans wrote:
> > >
> > > Revs.1.2-1.3 of tar/src/extract.c break pkg_add (not to mention probably
> > > thousands of user scripts that are no more careful than pkg_add) in
> > > -current and RELENG_4:
> >
> > Are you sure? My own investigation at the time of the commit showed
> > that old tar shipped with FreeBSD, was adjusting permissions of
> > extracting files when running as uid 0 according to current umask
> > settings, so that IMO 1.2-1.3 actually restored POLA, not broke it.

OK, further investigation shows that the problem is likely that unlike
the old one, the new tar doesn't preserve suid/sgid bits on
extraction, and it is what probably needs to be fixed instead.

> 
> Need evidence? Here it is:
> 
> # cvs co -D "10 months ago" src/gnu/usr.bin/tar
> [...]
> # cd src/gnu/usr.bin/tar
> # make
> [...]
> # mkdir foo
> # touch foo/bar
> # chmod 777 foo
> # chmod 777 foo/bar
> # ./tar cvf foo.tar foo
> foo/
> foo/bar
> # rm -rf foo
> # ./tar xvf foo.tar
> foo/
> foo/bar
> root@notebook# ls -l | grep foo
> drwxr-xr-x  2 root  wheel     512  1  19:01 foo/
> -rw-r--r--  1 root  wheel   10240  1  19:01 foo.tar
> root@notebook# ls -l foo
> total 0
> -rwxr-xr-x  1 root  wheel  0  1  19:01 bar*
> # umask
> 0022
> #
> 
> -Maxim
> 
> >
> > -Maxim
> >
> > >
> > > % RCS file: /home/ncvs/src/contrib/tar/src/extract.c,v
> > > % Working file: extract.c
> > > % head: 1.4
> > > % branch:
> > > % locks: strict
> > > % access list:
> > > % symbolic names:
> > > %       RELENG_4: 1.4.0.2
> > >         ^^^^^^^^
> > > %       TAR_v1_13_25: 1.1.1.1
> > > %       FSF: 1.1.1
> > > % keyword substitution: kv
> > > % total revisions: 6;   selected revisions: 6
> > > % description:
> > > % ...
> > > % ----------------------------
> > > % revision 1.3
> > > % date: 2002/06/07 06:02:35;  author: sobomax;  state: Exp;  lines: +1 -1
> > > % Disabling automatic --same-owner option when running as uid 0 along with
> > > % the --same-permissions was an overkill, so put it back. This is consistent
> > > % with what our old tar did.
> > > %
> > > % Suggested by: dillon
> > > % ----------------------------
> > > % revision 1.2
> > > % date: 2002/06/07 00:03:23;  author: sobomax;  state: Exp;  lines: +4 -0
> > > % IMO it was a quite ugly idea that if we are running as uid 0 then we can
> > > % safely ignore current umask(2) and assume that permissions should be set
> > > % right like in the archive. Not only it violates POLA, but introduces
> > >                                          ^^^^^^^^^^^^^
> > > % huge potential security vulnerability, particularly for ports, where
> > > % many popular archives come with 777 files and dirs.
> > > % ----------------------------
> > >
> > > Actually, it is the change violates POLA, and breaks everything that
> > > depends on the historical and still documented behaviour.  (The man
> > > page even says that (almost) all permissions are restored even in the
> > > !root case (it says this indirectly by saying that all attributes are
> > > restored if possible and not mentioning the umask or root).  The info
> > > page is better.)
> > >
> > > This bug showed up as breakage in mutt.  mutt uses a setgid utility
> > > named mutt_dotlock to lock /var/mail/*, so it fails to download mail
> > > if mutt_dotlock's setgid bit is lost on extraction.  It is probably
> > > another bug that mutt_dotlock attempts to create a temporary file in
> > > /var/mail instead of using flock().
> > >
> > > "Fixes":
> > >
> > > (1) Change pkg_add and thousands of user scripts to use tar -p.  This
> > > may reopen security holes closed by respecting the umask.
> > >
> > > %%%
> > > Index: extract.c
> > > ===================================================================
> > > RCS file: /home/ncvs/src/usr.sbin/pkg_install/add/extract.c,v
> > > retrieving revision 1.33
> > > diff -u -2 -r1.33 extract.c
> > > --- extract.c   11 May 2002 04:17:54 -0000      1.33
> > > +++ extract.c   1 Aug 2002 10:26:10 -0000
> > > @@ -33,5 +33,5 @@
> > >  #define PUSHOUT(todir) /* push out string */ \
> > >          if (where_count > (int)sizeof(STARTSTRING)-1) { \
> > > -                   strcat(where_args, "|tar --unlink -xf - -C "); \
> > > +                   strcat(where_args, "|tar --unlink -pxf - -C "); \
> > >                     strcat(where_args, todir); \
> > >                     if (system(where_args)) { \
> > > %%%
> > >
> > > (2) Restore standard gnu tar behaviour by backing out extract.c revs 1.2-1.3.
> > >
> > > %%%
> > > Index: extract.c
> > > ===================================================================
> > > RCS file: /home/ncvs/src/contrib/tar/src/extract.c,v
> > > retrieving revision 1.4
> > > diff -u -2 -r1.4 extract.c
> > > --- extract.c   3 Jul 2002 12:44:31 -0000       1.4
> > > +++ extract.c   1 Aug 2002 10:44:34 -0000
> > > @@ -113,7 +113,5 @@
> > >  {
> > >    we_are_root = geteuid () == 0;
> > > -#ifndef __FreeBSD__
> > >    same_permissions_option += we_are_root;
> > > -#endif
> > >    same_owner_option += we_are_root;
> > >    xalloc_fail_func = extract_finish;
> > > %%%
> > >
> > > Bruce
> >
> > To Unsubscribe: send mail to [EMAIL PROTECTED]
> > with "unsubscribe freebsd-current" in the body of the message
> 
> To Unsubscribe: send mail to [EMAIL PROTECTED]
> with "unsubscribe freebsd-current" in the body of the message

To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message

Reply via email to