[PATCH openbmc v6 17/18] initfs: update: Sanitze whitelist directory entries

[Andrew Jeffery]

On Sun, 2016-06-26 at 00:12 +0000, Milton Miller II wrote:
> 
> On 06/23/2016 about 01:26AM in some time zone, Andrew Jeffery wrote:
> >On Wed, 2016-06-22 at 19:30 -0500, OpenBMC Patches wrote:
> >> From: Milton Miller <miltonm at us.ibm.com>
> >> 
> >> Repeatedly strip trailing / and /. from whitelist entries and
> >> fail if an entry includes /../ or ends with /.. or doesn't start
> >> with a /.  Also use the entries quoted to avoid any glob.
> >
> >Right. Ending this particular sentence with a '/' is a bit subtle. I
> >think you should quote your path bits: "/", "/.", "/../", "/.."; this
> >way the sentence would end '... or doesn't start with a "/".'.
> 
> Ok done.
> 
> >> 
> >> It was noticed the save code was saving directories that ended in /
> >> into a subdirectory of the last component name.  This was traced
> >> the the code creating the directory just stripping the last /
> >> and then copying to the directory.
> >> 
> >> Choose to sanitize the entry where possible for ease of use verses
> >> a small performance penality.
> >> 
> >> Signed-off-by: Milton Miller <miltonm at us.ibm.com>
> >> ---
> >>  .../obmc-phosphor-initfs/files/obmc-update.sh             | 15
> >+++++++++++++--
> >>  1 file changed, 13 insertions(+), 2 deletions(-)
> >> 
> >> diff --git
> >a/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/ob
> >mc-update.sh
> >b/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/ob
> >mc-update.sh
> >> index 7120a18..bfacd00 100755
> >> ---
> >a/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/ob
> >mc-update.sh
> >> +++
> >b/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/ob
> >mc-update.sh
> >> @@ -170,13 +170,24 @@ then
> >>  
> >>   while read f
> >>   do
> >> - if ! test -e $upper/$f
> >> + # Entries shall start with /, no trailing /.. or embedded /../
> >> + if test "/${f#/}" != "$f" -o "${f%/..}" != "${f#*/../}"
> >
> >Can we do this with `readlink -f` or `realpath` instead? Do we have
> >them available in this environment? As in, test that the result
> >before
> >and after realpath are the same. You can tell realpath to not expand
> >symlinks.
> >
> 
> ENODOC
> 
> There does not appear to be a man page available for realpath(1)
> in either Debian wheezy or RHEL 6.4.

Okay, as suggested in your link[1] it looks like debian ship their own
realpath and RHEL 6.4 is probably too old for GNU realpath. GNU
realpath has:

       -s, --strip, --no-symlinks
              don't expand symlinks

So:

    $ realpath -s /etc/../etc/alternatives/cc
    /etc/alternatives/cc
    $ ls -l !$
    ls -l /etc/../etc/alternatives/cc
    lrwxrwxrwx 1 root root 12 Dec  7  2015 /etc/../etc/alternatives/cc -> /usr/bin/gcc

>   The man page for readlink
> is talking about expanding symlinks with -f saying it follows symlinks
> recursively, no where does it talk about cleaning up the path, nor
> returning the path of something not a symlink.

Yes, readlink -f was probably a poor suggestion if we don't want to
resolve symlinks.

> 
> A qucik search turned up [1] which shows some of the history.  
> Also note it says busybox (which is the version we are using) 
> does not take options in its realpath command.

Right, this is the killer bit, that busybox realpath can't do -s.

> 
> Also this is already peaking under the covers at the read-write 
> COW directory which is a sparse copy and not looking though 
> the combined file system.  And this file subtree is not mounted 
> at / meaning any attempt to follow an absolute symlink will likely 
> find the wrong content and not necessarily fail.
> 
> So I am rejecting this suggestion.

Given the above, I'm okay with that.

> 
> [1] http://unix.stackexchange.com/questions/136494/whats-the-difference-between-realpath-and-readlink-f
> 
> >> + then
> >> + echo 1>&2 "WARNING: Skipping bad whitelist entry $f."
> >> + continue
> >> + fi
> >> + if ! test -e "$upper/$f"
> >>   then
> >>   continue
> >>   fi
> >>   d="$save/$f"
> >> + while test "${d%/}" != "${d%/.}"
> >> + do
> >> + d="${d%/.}"
> >> + d="${d%/}"
> >> + done
> >>   mkdir -p "${d%/*}"
> >> - cp -rp $upper/$f "${d%/*}/"
> >> + cp -rp "$upper/$f" "${d%/*}/"
> >
> >Again the while loop and other bits can hopefully be replaced by
> >`readlink -f` or `realpath`. The whole trailing-slash thing with `cp`
> >always throws me.
> 
> While the test here for $d is absolute I'd rather not do it.  And 
> right now this has at least two / (one in the string and one leading 
> the $f) meaning a simple equality check of $d and $(realpath $d) 
> will always fail.

If we know everything in the whitelist is absolute could we take a
different approach to the path construction?

>   I could remove the slash but that implies the 
> first check,

Isn't it instead that because of the first check we now know that the
whitelist path is absolute, so we can validly removing the leading '/'
to construct the necessary path?

>  and again we are copying from a sparse directory and 
> want to copy a symlink not resolve it. 

Sure; as above, realpath -s would have avoided the resolution but as
you point out we can't use it (busybox).

>   I'd love to use rsync here 
> but that is not currently available.

right.

> 
> If you object to the while loop I can reject the path if the conditions 
> on $f with similar substitutions are not equal (reordering the fix
> commit). I don't anticipate the loop executing more than once
> unless someone is testing corner cases and thought the convenience 
> of making the entry sane was better than the debug why the entry
> didn't work.
> 

What you've got sounds preferable. The path string hackery is
unfortunate but sounds unavoidable.

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160627/a908885d/attachment.sig>