bin/93915: pkg_add behaves improperly when unpacking over symlinks

[Jason Heiss]

>Number:         93915
>Category:       bin
>Synopsis:       pkg_add behaves improperly when unpacking over symlinks
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Mon Feb 27 21:20:03 GMT 2006
>Closed-Date:
>Last-Modified:
>Originator:     Jason Heiss
>Release:        6.0
>Organization:
Yahoo!
>Environment:
>Description:
The behavior of pkg_add when unpacking over symlinks changed for the
worse between 4.x and 6.x (I believe specifically it changed in 5.3 when
bsdtar was made the default tar).

Assume you have a package containing only the following file (i.e. the
package does not specifically list /usr/local as a component of the
package):

/usr/local/bin/foo

And assume one of the following two directory structures exist prior to
installing that package:

/usr/local -> /opt/local
/opt/local/bin/

/usr/local/bin/foo -> /usr/local/bin/bar

When unpacking over the first directory structure I would expect foo to
end up in /opt/local/bin/foo.  Because the package does not specifically
say that /usr/local should be a directory I would expect pkg_add to
traverse the symlink.  (I can imagine arguments against this behavior,
which I'll discuss in more detail later.)

When unpacking over the second directory structure I would expect the
existing /usr/local/bin/foo symlink to be removed, and the
/usr/local/bin/foo from the package to be installed.  Because
/usr/local/bin/foo is specifically listed as a component of the package
I would expect any existing file in that location to be removed and
replaced by the file in the package.

Both of these are the behaviors you see in older versions of FreeBSD.

However, in current versions when unpacking over the first directory
structure the /usr/local symlink is removed and the directory structure
within the package is created.  That definitely violates the "principal
of least astonishment", and also is a unexpected/undocumented change in
behavior.  I believe it is sufficiently unexpected as to qualify as a
bug.

I believe the reason for this change in behavior is the replacement of
the underlying tar command in the 5.3 release, and more specifically the
old GNU tar and bsdtar handling the --unlink option differently.  In
October 1998 the --unlink option was added to the options passed to tar by
pkg_add when unpacking files in their final destination (see the PUSHOUT
macro in src/usr.sbin/pkg_install/add/extract.c).  The CVS commit
message gives no reason for the change, and I'm not clear what the
--unlink option does in the old GNU tar (it seems to have no effect in
either of the example situations I gave earlier).

With bsdtar the --unlink option results in the behavior I am reporting,
where the /usr/local symlink is removed.  This appears to be the
documented and intended behavior of the --unlink option in bsdtar, so I
don't consider that a bug in bsdtar.  However, I don't believe pkg_add
should be exhibiting this behavior, which implies that pkg_add should
not specify the --unlink option when tar==bsdtar.  The question then is
can you simply remove the --unlink flag (reverse the change of October
1998) and have acceptable behavior?  I believe the answer is no, but I
can imagine debate on this point (as mentioned earlier).  The two
reasonable behaviors would be:

1) Silently traverse any symlinks in the path.  This was the behavior of
   older versions of FreeBSD, and I believe this to be the most
   reasonable behavior for pkg_add.
2) Error out if unpacking any files requires traversing a symlink.  This
   is the behavior of bsdtar in the absence of the --unlink flag.

When bsdtar is confronted with the first example directory structure and
you have not specified the --unlink flag it will simply refuse to follow
the symlink and unpack the file.  (See the security_problem function in
src/usr.bin/tar/read.c)  This is behavior 2 above, and is the easiest to
achieve by simply removing the --unlink flag in extract.c.

In order to achieve behavior 1 we have to suppress the symlink traversal
detection of the security_problem function within bsdtar.  The only way
to suppress that behavior is through the -P flag, which has other
unwanted side effects.  I think the only way to achieve this behavior
would be to add a new option to bsdtar which is a subset of the -P
behavior, just suppressing the symlink detection.

In summary, I recommend that the --unlink flag to tar be immediately
removed from the PUSHOUT macro of extract.c within pkg_add.  That will
removed a very unexpected and unacceptable behavior.  A possible
remaining improvement would be to replace that with a flag to tar which
disables symlink traversal detection, such a flag does not currently
exist in bsdtar.

>How-To-Repeat:

>Fix:
This should be applied to any release where tar==bsdtar:

Index: src/usr.sbin/pkg_install/add/extract.c
===================================================================
RCS file: /home/ncvs/src/usr.sbin/pkg_install/add/extract.c,v
retrieving revision 1.44
diff -u -r1.44 extract.c
--- src/usr.sbin/pkg_install/add/extract.c      7 Jan 2006 22:10:57 -0000
1.44
+++ src/usr.sbin/pkg_install/add/extract.c      27 Feb 2006 21:15:11 -0000
@@ -34,7 +34,7 @@
 
 #define PUSHOUT(todir) /* push out string */ \
     if (where_count > (int)sizeof(STARTSTRING)-1) { \
-       strcat(where_args, "|/usr/bin/tar --unlink -xpf - -C "); \
+       strcat(where_args, "|/usr/bin/tar -xpf - -C "); \
        strcat(where_args, todir); \
        if (system(where_args)) { \
            cleanup(0); \

>Release-Note:
>Audit-Trail:
>Unformatted: