From 672705831f4333decb4e3e925f7b6ad379e58bb9 Mon Sep 17 00:00:00 2001 From: Ian Wienand Date: Fri, 8 Apr 2016 15:46:21 +1000 Subject: [PATCH] Add a best-effort sudo safety check As motivation for this; we have had two breakouts of dib in recent memory. One was a failure to unmount through symlinks in the core code (I335316019ef948758392b03e91f9869102a472b9) and the other was removing host keys on the build-system (Ib01d71ff9415a0ae04d963f6e380aab9ac2260ce). For the most part, dib runs unprivileged. Bits of the core code are hopefully well tested (modulo bugs like the first one!). We give free reign inside the chroot (although there is still some potential there for adverse external affects via bind mounts). Where we could be a bit safer (and could have prevented at least the second of these breakouts) is with some better checking that the "sudo" calls *outside* the chroot at least looked sane. This adds a basic check that we're using chroot or image paths when calling sudo in those parts of elements that run *outside* the chroot. Various files are updated to accomodate this check; mostly by just ignoring it for existing code (I have not audited these calls). Nobody is pretending this type of checking makes dib magically safe, or removes the issues with it needing to do things as root during the build. But this can help find egregious errors like the key removal. Change-Id: I161a5aea1d29dcdc7236f70d372c53246ec73749 --- bin/dib-lint | 28 ++++++++++++++++++- doc/source/developer/developing_elements.rst | 27 ++++++++++++++++++ .../99-override-default-apt-sources | 2 +- .../cleanup.d/99-extract-kernel-and-ramdisk | 2 ++ elements/bootloader/cleanup.d/51-bootloader | 2 ++ elements/dpkg/extra-data.d/01-copy-apt-keys | 4 +-- .../ironic-agent/cleanup.d/99-ramdisk-create | 2 ++ elements/iso/cleanup.d/100-build-iso | 2 ++ .../block-device.d/10-partitioning-sfdisk | 2 ++ .../pypi/extra-data.d/00-mount-pypi-mirror | 4 +-- elements/rhel/root.d/10-rhel-cloud-image | 2 ++ .../extra-data.d/98-source-repositories | 1 + .../ubuntu-core/root.d/10-cache-ubuntu-image | 2 ++ elements/vm/block-device.d/10-partition | 2 ++ elements/yum-minimal/root.d/08-yum-chroot | 3 ++ 15 files changed, 79 insertions(+), 6 deletions(-) diff --git a/bin/dib-lint b/bin/dib-lint index 248778499..bedf1da3e 100755 --- a/bin/dib-lint +++ b/bin/dib-lint @@ -54,7 +54,7 @@ excluded() { } error() { - echo "ERROR: $1" + echo -e "ERROR: $1" rc=1 } @@ -146,6 +146,32 @@ for i in $(find elements -type f \ fi fi fi + + + # check that sudo calls in phases run outside the chroot look + # "safe"; meaning that they seem to operate within the chroot + # somehow. This is not fool-proof, but catches egregious errors, + # and makes you think about it if you're doing something outside + # the box. + if ! excluded safe_sudo; then + if [[ $(dirname $i) =~ (root.d|extra-data.d|block-device.d|finalise.d|cleanup.d) ]]; then + while read LINE + do + if [[ $LINE =~ "sudo " ]]; then + # messy regex ahead! Don't match: + # - explicitly ignored + # - basic comments + # - install-packages ... sudo ... + # - any of the paths passed into the out-of-chroot elements + if [[ $LINE =~ (dib-lint: safe_sudo|^#|install-packages|TARGET_ROOT|IMAGE_BLOCK_DEVICE|TMP_MOUNT_PATH|TMP_IMAGE_PATH) ]]; then + continue + fi + error "$i : potentially unsafe sudo\n -- $LINE" + fi + done < $i + fi + fi + done for i in $(find elements -type f -and -name '*.rst' -or -type f -executable); do diff --git a/doc/source/developer/developing_elements.rst b/doc/source/developer/developing_elements.rst index a999a22db..0cee5946e 100644 --- a/doc/source/developer/developing_elements.rst +++ b/doc/source/developer/developing_elements.rst @@ -425,3 +425,30 @@ example if one were building tripleo-images, the variable would be set like: export ELEMENTS_PATH=tripleo-image-elements/elements disk-image-create rhel7 cinder-api + +Linting +------- + +You should always run ``bin/dib-lint`` over your elements. It will +warn you of common issues. + +sudo +"""" + +Using ``sudo`` outside the chroot environment can cause breakout +issues where you accidentally modify parts of the host +system. ``dib-lint`` will warn if it sees ``sudo`` calls that do not +use the path arguments given to elements running outside the chroot. + +To disable the error for a call you know is safe, add + +:: + + # dib-lint: safe_sudo + +to the end of the ``sudo`` command line. To disable the check for an +entire file, add + +:: + + # dib-lint: disable=safe_sudo diff --git a/elements/apt-sources/extra-data.d/99-override-default-apt-sources b/elements/apt-sources/extra-data.d/99-override-default-apt-sources index 7381693b8..94fa6e9b2 100755 --- a/elements/apt-sources/extra-data.d/99-override-default-apt-sources +++ b/elements/apt-sources/extra-data.d/99-override-default-apt-sources @@ -21,5 +21,5 @@ DIB_APT_SOURCES=`readlink -f $DIB_APT_SOURCES` # copy the sources.list to cloudimg pushd $TMP_MOUNT_PATH/etc/apt/ -sudo cp -f $DIB_APT_SOURCES sources.list +sudo cp -f $DIB_APT_SOURCES sources.list # dib-lint: safe_sudo popd diff --git a/elements/baremetal/cleanup.d/99-extract-kernel-and-ramdisk b/elements/baremetal/cleanup.d/99-extract-kernel-and-ramdisk index 7c4a68571..d1667ee6d 100755 --- a/elements/baremetal/cleanup.d/99-extract-kernel-and-ramdisk +++ b/elements/baremetal/cleanup.d/99-extract-kernel-and-ramdisk @@ -14,6 +14,8 @@ # License for the specific language governing permissions and limitations # under the License. +# dib-lint: disable=safe_sudo + if [ ${DIB_DEBUG_TRACE:-0} -gt 0 ]; then set -x fi diff --git a/elements/bootloader/cleanup.d/51-bootloader b/elements/bootloader/cleanup.d/51-bootloader index 40a3b1ad4..28202f8bf 100755 --- a/elements/bootloader/cleanup.d/51-bootloader +++ b/elements/bootloader/cleanup.d/51-bootloader @@ -15,6 +15,8 @@ # License for the specific language governing permissions and limitations # under the License. +# dib-lint: disable=safe_sudo + if [ ${DIB_DEBUG_TRACE:-1} -gt 0 ]; then set -x fi diff --git a/elements/dpkg/extra-data.d/01-copy-apt-keys b/elements/dpkg/extra-data.d/01-copy-apt-keys index 668135c7f..52f4ff790 100755 --- a/elements/dpkg/extra-data.d/01-copy-apt-keys +++ b/elements/dpkg/extra-data.d/01-copy-apt-keys @@ -31,9 +31,9 @@ if [ -e ${DIR} ]; then echo "${DIR} already exists!" exit 1 fi -sudo mkdir -p ${DIR} +sudo mkdir -p ${DIR} # dib-lint: safe_sudo # Copy to DIR for KEY in $(find ${DIB_ADD_APT_KEYS} -type f); do - sudo cp -L ${KEY} ${DIR} + sudo cp -L ${KEY} ${DIR} # dib-lint: safe_sudo done diff --git a/elements/ironic-agent/cleanup.d/99-ramdisk-create b/elements/ironic-agent/cleanup.d/99-ramdisk-create index 9a185da17..d64332dc5 100755 --- a/elements/ironic-agent/cleanup.d/99-ramdisk-create +++ b/elements/ironic-agent/cleanup.d/99-ramdisk-create @@ -1,5 +1,7 @@ #!/bin/bash +# dib-lint: disable=safe_sudo + if [ "${DIB_DEBUG_TRACE:-0}" -gt 0 ]; then set -x fi diff --git a/elements/iso/cleanup.d/100-build-iso b/elements/iso/cleanup.d/100-build-iso index 5430a0831..4832c9444 100755 --- a/elements/iso/cleanup.d/100-build-iso +++ b/elements/iso/cleanup.d/100-build-iso @@ -14,6 +14,8 @@ # License for the specific language governing permissions and limitations # under the License. +# dib-lint: disable=safe_sudo + if [ ${DIB_DEBUG_TRACE:-1} -gt 0 ]; then set -x fi diff --git a/elements/partitioning-sfdisk/block-device.d/10-partitioning-sfdisk b/elements/partitioning-sfdisk/block-device.d/10-partitioning-sfdisk index 447f23a0e..a529e0a47 100755 --- a/elements/partitioning-sfdisk/block-device.d/10-partitioning-sfdisk +++ b/elements/partitioning-sfdisk/block-device.d/10-partitioning-sfdisk @@ -1,5 +1,7 @@ #!/bin/bash +# dib-lint: disable=safe_sudo + if [ ${DIB_DEBUG_TRACE:-0} -gt 0 ]; then set -x fi diff --git a/elements/pypi/extra-data.d/00-mount-pypi-mirror b/elements/pypi/extra-data.d/00-mount-pypi-mirror index 7d2923f4e..570a7711a 100755 --- a/elements/pypi/extra-data.d/00-mount-pypi-mirror +++ b/elements/pypi/extra-data.d/00-mount-pypi-mirror @@ -10,6 +10,6 @@ MIRROR_SOURCE=$DIB_IMAGE_CACHE/pypi/mirror/ if [ -d "$MIRROR_SOURCE" ]; then MIRROR_TARGET=$TMP_MOUNT_PATH/tmp/pypi - sudo mkdir -p $MIRROR_SOURCE $MIRROR_TARGET - sudo mount --bind $MIRROR_SOURCE $MIRROR_TARGET + sudo mkdir -p $MIRROR_SOURCE $MIRROR_TARGET # dib-lint: safe_sudo + sudo mount --bind $MIRROR_SOURCE $MIRROR_TARGET # dib-lint: safe_sudo fi diff --git a/elements/rhel/root.d/10-rhel-cloud-image b/elements/rhel/root.d/10-rhel-cloud-image index 1571249fd..909949da9 100755 --- a/elements/rhel/root.d/10-rhel-cloud-image +++ b/elements/rhel/root.d/10-rhel-cloud-image @@ -1,5 +1,7 @@ #!/bin/bash +# dib-lint: disable=safe_sudo + if [ ${DIB_DEBUG_TRACE:-0} -gt 0 ]; then set -x fi diff --git a/elements/source-repositories/extra-data.d/98-source-repositories b/elements/source-repositories/extra-data.d/98-source-repositories index 4770bea60..76d874ad2 100755 --- a/elements/source-repositories/extra-data.d/98-source-repositories +++ b/elements/source-repositories/extra-data.d/98-source-repositories @@ -1,5 +1,6 @@ #!/bin/bash +# dib-lint: disable=safe_sudo if [ ${DIB_DEBUG_TRACE:-0} -gt 1 ]; then set -x fi diff --git a/elements/ubuntu-core/root.d/10-cache-ubuntu-image b/elements/ubuntu-core/root.d/10-cache-ubuntu-image index 550fdc2e8..10faddfa1 100755 --- a/elements/ubuntu-core/root.d/10-cache-ubuntu-image +++ b/elements/ubuntu-core/root.d/10-cache-ubuntu-image @@ -1,6 +1,8 @@ #!/bin/bash # These are useful, or at worst not harmful, for all images we build. +# dib-lint: disable=safe_sudo + if [ ${DIB_DEBUG_TRACE:-0} -gt 0 ]; then set -x fi diff --git a/elements/vm/block-device.d/10-partition b/elements/vm/block-device.d/10-partition index df0732edd..e1bc8ddff 100755 --- a/elements/vm/block-device.d/10-partition +++ b/elements/vm/block-device.d/10-partition @@ -1,5 +1,7 @@ #!/bin/bash +# dib-lint: disable=safe_sudo + if [ ${DIB_DEBUG_TRACE:-0} -gt 0 ]; then set -x fi diff --git a/elements/yum-minimal/root.d/08-yum-chroot b/elements/yum-minimal/root.d/08-yum-chroot index 2d0850814..6cf0dfaf2 100755 --- a/elements/yum-minimal/root.d/08-yum-chroot +++ b/elements/yum-minimal/root.d/08-yum-chroot @@ -14,6 +14,9 @@ # License for the specific language governing permissions and limitations # under the License. # + +# dib-lint: disable=safe_sudo + if [ "${DIB_DEBUG_TRACE:-0}" -gt 0 ]; then set -x fi