[Buildroot] [PATCH 2/2] openvmtools: add patch to defend from lsb path walking attack

[Romain Naour]

Hi Karoly,

Le 25/02/2015 23:14, Karoly Kasza a écrit :
> Add patch to defend lsb from path walking attack.
> Originally from Debian.
> 
> Signed-off-by: Karoly Kasza <kaszak at gmail.com>
> ---
>  package/openvmtools/0009-lsb.patch |  106 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 106 insertions(+)
>  create mode 100644 package/openvmtools/0009-lsb.patch
> 
> diff --git a/package/openvmtools/0009-lsb.patch b/package/openvmtools/0009-lsb.patch
> new file mode 100644
> index 0000000..54c3232
> --- /dev/null
> +++ b/package/openvmtools/0009-lsb.patch
> @@ -0,0 +1,106 @@
> +Upsteam Debian patch to defend openvmtools against path walking.
> +
> +Original description:
> +
> +From 3a9f2297a82b9c109e894b5f8ea17753e68830ac Mon Sep 17 00:00:00 2001
> +From: "VMware, Inc" <>
> +Date: Tue, 17 Sep 2013 20:39:34 -0700
> +Subject: [PATCH] Harden HostinfoOSData against $PATH attacks.
> +
> +We are doing a popen("lsb_release... ") when attempting to
> +determine host details in hostinfoPosix.c. Using popen means that
> +$PATH is walked when looking for the lsb_release binary, and that
> +may give an attacker the ability to run a malicious version of
> +lsb_release.
> +
> +This change does two things,
> +
> +a) Hard code the path to lsb_release. I've searched around
> +   the web and I believe the path is always "/usr/bin/lsb_release"
> +   so let's not leave this up to chance.
> +
> +b) Stop running HostinfoGetCmdOutput with elevated privileges. Drop
> +   to non-root when possible. If someone sneaks in a new call to
> +   HostinfoGetCmdOutput and doesn't use a full path, then we will
> +   hopefully avoid a firedrill. I'm only applying this to Linux
> +   because the Fusion build barfed when I tried to compile with
> +   without the vmx86_linux.
> +
> +I think either (a) or (b) would be enough but I'm doing both,
> +because each individually is correct. Also note that in the blog
> +post by Tavis Ormandy calls out doing (a) as not enough,
> +   http://blog.cmpxchg8b.com/2013/08/security-debianisms.html
> +His example uses a bash feature that allows functions to be
> +exported. I haven't been able to get that to work on my Ubuntu
> +machine.
> +
> +To test I'm manually run Linux WS and Fusion and verified that
> +the logs look correct.
> +
> +Signed-off-by: Dmitry Torokhov <dtor at vmware.com>
> +
> +Signed-off-by: Karoly Kasza <kaszak at gmail.com>
> +
> +---
> + open-vm-tools/lib/misc/hostinfoPosix.c | 23 +++++++++++++++++++----
> + 1 file changed, 19 insertions(+), 4 deletions(-)
> +
> +--- a/lib/misc/hostinfoPosix.c
> ++++ b/lib/misc/hostinfoPosix.c
> +@@ -800,17 +800,27 @@ out:
> + static char *
> + HostinfoGetCmdOutput(const char *cmd)  // IN:
> + {
> ++   Bool isSuperUser = FALSE;
> +    DynBuf db;
> +    FILE *stream;
> +    char *out = NULL;
> + 
> ++   /*
> ++    * Attempt to lower privs, because we use popen and an attacker
> ++    * may control $PATH.
> ++    */
> ++   if (vmx86_linux && Id_IsSuperUser()) {
> ++      Id_EndSuperUser(getuid());
> ++      isSuperUser = TRUE;
> ++   }
> ++
> +    DynBuf_Init(&db);
> + 
> +    stream = Posix_Popen(cmd, "r");
> +    if (stream == NULL) {
> +       Warning("Unable to get output of command \"%s\"\n", cmd);
> + 
> +-      return NULL;
> ++      goto exit;
> +    }
> + 
> +    for (;;) {
> +@@ -844,11 +854,16 @@ HostinfoGetCmdOutput(const char *cmd)  /
> +    if (DynBuf_Get(&db)) {
> +       out = (char *) DynBuf_AllocGet(&db);
> +    }
> +- closeIt:
> +-   DynBuf_Destroy(&db);
> + 
> ++ closeIt:
> +    pclose(stream);
> + 
> ++ exit:
> ++   DynBuf_Destroy(&db);
> ++
> ++   if (isSuperUser) {
> ++      Id_BeginSuperUser();
> ++   }
> +    return out;
> + }
> + 
> +@@ -967,7 +982,7 @@ HostinfoOSData(void)
> +        * Try to get OS detailed information from the lsb_release command.
> +        */
> + 
> +-      lsbOutput = HostinfoGetCmdOutput("lsb_release -sd 2>/dev/null");
> ++      lsbOutput = HostinfoGetCmdOutput("/usr/bin/lsb_release -sd 2>/dev/null");
> +       if (!lsbOutput) {
> +          int i;
> + 
> 

Although we don't have a lsb package in Buildroot, this patch looks good.

Reviewed-by: Romain Naour <romain.naour at openwide.fr>

Best regards,
Romain