procmail
[Top] [All Lists]

Re: review this recipe for security holes, please

1997-09-12 21:00:11
eli(_at_)NetUSA(_dot_)Net (Eli The Bearded) writes:
Anyone see serious security holes in this? Assume most recent
procmail version.

      ARCHIVEDIR=/full/path/to/www/directory/

Drop the trailing slash: you're inserting it down below, and it'll
choke if you ever end up on Apollo's DomainOS where double slashes
are network references.


...
      # The idea is to catch only safe files names, and those may
      # not have a ".." in them. For simplicity don't allow them
      # to end with a "." either.
      * $ ^To:[^/]+\+\//([^$SHELLMETAS\$.]|[^.]\.[^.])+

So 8bit characters are fine in filenames, as are spaces, tabs, and
other control characters?  Also, the above lets through shell meta
characters that are right next to a period.  As dozens of bugtraq
announcement have told us again and again, don't try to exclude unsafe
characters -- include only safe ones:

        * ^To: [^/]+\+\//[-a-z0-9_+./@=:,%]+
        * ! MATCH ?? /\.

The second of those conditions will protect you from 'dot files',
include "/../".  Really paranoid people would just exclude periods
entirely.


      {
        FILE=$MATCH

        # Get the directory portion
        :0
        * FILE ?? ^^\/.*/
        { DIR=$ARCHIVEDIR/$MATCH }

DIR will have a trailing slash which apparently will choke mkdir() on
some OSes (they treat it like "/.").  Since DFAs have no lookahead, this
requires two regexps.  Also, what if the file is to appear in $ARCHIVEDIR
itself?  There won't be a second slash and the match will fail, so we need
an else clause:

        :0
        * FILE  ?? ^^\/.*/
        * MATCH ?? ^^.*[^/]
        { DIR = $ARCHIVEDIR/$MATCH }
        :0 E
        { DIR = $ARCHIVEDIR }


        # Ensure it exists; the 'h' flag is probably not useful
        :0ihc
        * ? test ! -d $DIR
        | mkdir $DIR

Depending on you intentions, you might want to give mkdir the -p flag
to create all intervening directories, instead of failing if all but
the last component don't exist.


        # Get a lock for the following two actions.
        LOCK=$ARCHIVEDIR/$MATCH.lock

You mean:
        LOCKFILE = $ARCHIVEDIR/$FILE.$LOCKEXT

MATCH contains the directory part at this point.


        # Ensure the file does not exist; the 'h' flag...
        :0ihc
        | rm -f $ARCHIVEDIR/$MATCH

        # Save the (raw) body only in the file.
        :0br
        $ARCHIVEDIR/$MATCH

Those should all be $ARCHIVEDIR/$FILE, but otherwise it looks good.


        # Release the lock
        LOCK

        LOCKFILE


Philip Guenther

<Prev in Thread] Current Thread [Next in Thread>