<font face="Default Sans Serif,Verdana,Arial,Helvetica,sans-serif" size="2"><br>On 06/23/2016 about 01:26AM in some time zone, Andrew Jeffery wrote:<br>>On Wed, 2016-06-22 at 19:30 -0500, OpenBMC Patches wrote:<br>>> From: Milton Miller <<a target="_blank" href="mailto:miltonm@us.ibm.com">miltonm@us.ibm.com</a>><br>>> <br>>> Repeatedly strip trailing / and /. from whitelist entries and<br>>> fail if an entry includes /../ or ends with /.. or doesn't start<br>>> with a /. Also use the entries quoted to avoid any glob.<br>><br>>Right. Ending this particular sentence with a '/' is a bit subtle. I<br>>think you should quote your path bits: "/", "/.", "/../", "/.."; this<br>>way the sentence would end '... or doesn't start with a "/".'.<br><br>Ok done.<br><br>>> <br>>> It was noticed the save code was saving directories that ended in /<br>>> into a subdirectory of the last component name. This was traced<br>>> the the code creating the directory just stripping the last /<br>>> and then copying to the directory.<br>>> <br>>> Choose to sanitize the entry where possible for ease of use verses<br>>> a small performance penality.<br>>> <br>>> Signed-off-by: Milton Miller <<a target="_blank" href="mailto:miltonm@us.ibm.com">miltonm@us.ibm.com</a>><br>>> ---<br>>> .../obmc-phosphor-initfs/files/obmc-update.sh | 15<br>>+++++++++++++--<br>>> 1 file changed, 13 insertions(+), 2 deletions(-)<br>>> <br>>> diff --git<br>>a/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/ob<br>>mc-update.sh<br>>b/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/ob<br>>mc-update.sh<br>>> index 7120a18..bfacd00 100755<br>>> ---<br>>a/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/ob<br>>mc-update.sh<br>>> +++<br>>b/meta-phosphor/common/recipes-phosphor/obmc-phosphor-initfs/files/ob<br>>mc-update.sh<br>>> @@ -170,13 +170,24 @@ then<br>>> <br>>> while read f<br>>> do<br>>> - if ! test -e $upper/$f<br>>> + # Entries shall start with /, no trailing /.. or embedded /../<br>>> + if test "/${f#/}" != "$f" -o "${f%/..}" != "${f#*/../}"<br>><br>>Can we do this with `readlink -f` or `realpath` instead? Do we have<br>>them available in this environment? As in, test that the result<br>>before<br>>and after realpath are the same. You can tell realpath to not expand<br>>symlinks.<br>><br><br>ENODOC<br><br>There does not appear to be a man page available for realpath(1)<br>in either Debian wheezy or RHEL 6.4. The man page for readlink<br>is talking about expanding symlinks with -f saying it follows symlinks<br>recursively, no where does it talk about cleaning up the path, nor<br>returning the path of something not a symlink.<br><br>A qucik search turned up [1] which shows some of the history. <br>Also note it says busybox (which is the version we are using) <br>does not take options in its realpath command.<br><br>Also this is already peaking under the covers at the read-write <br>COW directory which is a sparse copy and not looking though <br>the combined file system. And this file subtree is not mounted <br>at / meaning any attempt to follow an absolute symlink will likely <br>find the wrong content and not necessarily fail.<br><br>So I am rejecting this suggestion.<br><br>[1] <a target="_blank" href="http://unix.stackexchange.com/questions/136494/whats-the-difference-between-realpath-and-readlink-f">http://unix.stackexchange.com/questions/136494/whats-the-difference-between-realpath-and-readlink-f</a><br><br>>> + then<br>>> + echo 1>&2 "WARNING: Skipping bad whitelist entry $f."<br>>> + continue<br>>> + fi<br>>> + if ! test -e "$upper/$f"<br>>> then<br>>> continue<br>>> fi<br>>> d="$save/$f"<br>>> + while test "${d%/}" != "${d%/.}"<br>>> + do<br>>> + d="${d%/.}"<br>>> + d="${d%/}"<br>>> + done<br>>> mkdir -p "${d%/*}"<br>>> - cp -rp $upper/$f "${d%/*}/"<br>>> + cp -rp "$upper/$f" "${d%/*}/"<br>><br>>Again the while loop and other bits can hopefully be replaced by<br>>`readlink -f` or `realpath`. The whole trailing-slash thing with `cp`<br>>always throws me.<br><br>While the test here for $d is absolute I'd rather not do it. And <br>right now this has at least two / (one in the string and one leading <br>the $f) meaning a simple equality check of $d and $(realpath $d) <br>will always fail. I could remove the slash but that implies the <br>first check, and again we are copying from a sparse directory and <br>want to copy a symlink not resolve it. I'd love to use rsync here <br>but that is not currently available.<br><br>If you object to the while loop I can reject the path if the conditions <br>on $f with similar substitutions are not equal (reordering the fix<br>commit). I don't anticipate the loop executing more than once<br>unless someone is testing corner cases and thought the convenience <br>of making the entry sane was better than the debug why the entry<br>didn't work.<br><br>Regarding the trailing / on cp, yes it gets me to and I always have<br>to confirm its working. But I did that for this script once.<br><br>milton</font><BR>