[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20140623231748.GC4249@sherlock>
Date: Mon, 23 Jun 2014 16:17:48 -0700
From: Andrew Gaul <gaul@...che.org>
To: oss-security@...ts.openwall.com, Kurt Seifried <kseifried@...hat.com>
Subject: Re: TMP flaw in rackspace jclouds?
[bcc: jclouds private list]
Since oss-security@...ts.openwall.com is a public list I reported the
issue and submitted a pull request:
https://issues.apache.org/jira/browse/JCLOUDS-612
https://github.com/jclouds/jclouds/pull/420
On Thu, Jun 19, 2014 at 03:55:49PM -0700, Andrew Gaul wrote:
> [bcc: jclouds private list]
>
> I attached a patch which changes ScriptBuilder to use the "mktemp -d"
> approach that Ignasi suggested. I verified this against the ec2 live
> tests, specifically testCreateAndRunAService, as well as the
> scriptbuilder unit tests. I encourage someone more familiar with
> compute to test this since I have limited experience with those
> services.
>
> The code runs on the target node as Ignasi describes and thus it poses
> no risk to the master jclouds application. However, users could run
> untrusted software on their target nodes and we should address this in
> the next minor release. I estimate a low severity to this issue and
> prefer to continue discussion on the public bug tracker. Does anyone
> have a different understanding of this flaw?
>
> On Thu, Jun 19, 2014 at 09:32:25AM +0200, Ignasi Barrera wrote:
> > Take into account that the "statement" list will be rendered to a String,
> > composed with other script fragments into a final bash script, uploaded to
> > a node, and executed there locally as a bash script.
> >
> > That code won't be executed in the machine running jclouds, but as a bash
> > script in the provisioned node, so the name of the temporal directory
> > should better be generated in the script itself. A good approach would be
> > to directly use the "mktemp"command.
> > El 19/06/2014 06:36, "Andrew Gaul" <gaul@...che.org> escribió:
> >
> > > Kurt, thank you for bringing this flaw to my attention and I will
> > > address it tomorrow. I do not have a security background; can you
> > > estimate the severity and whether we can continue discussion on the
> > > public bug tracker? For now I have bcc the Apache jclouds private
> > > mailing list. Also note that jclouds is an Apache project not a
> > > Rackspace project and the canonical URLs are:
> > >
> > > https://github.com/jclouds/jclouds
> > > https://issues.apache.org/jira/browse/JCLOUDS
> > >
> > > On Wed, Jun 18, 2014 at 08:52:59PM -0600, Kurt Seifried wrote:
> > > > -----BEGIN PGP SIGNED MESSAGE-----
> > > > Hash: SHA1
> > > >
> > > > https://github.com/rackspace/jclouds/
> > > >
> > > > So CC'ing Andrew, he's a consistent contributor, I can't file an issue
> > > > in Github (no link to it) so posting here and CC'ing him.
> > > >
> > > >
> > > https://github.com/rackspace/jclouds/blob/master/scriptbuilder/src/main/java/org/jclouds/scriptbuilder/domain/Statements.java
> > > >
> > > > public static Statement extractTargzAndFlattenIntoDirectory(URI tgz,
> > > > String dest) {
> > > > return new StatementList(ImmutableSet.<Statement> builder()
> > > > .add(exec("mkdir /tmp/$$"))
> > > > .add(extractTargzIntoDirectory(tgz, "/tmp/$$"))
> > > > .add(exec("mkdir -p " + dest))
> > > > .add(exec("mv /tmp/$$/*/* " + dest))
> > > > .add(exec("rm -rf /tmp/$$")).build());
> > > > }
> > > >
> > > >
> > > > This is insecure, $$ == PID == predictable
> > > >
> > > > http://kurt.seifried.org/2012/03/14/creating-temporary-files-securely/
> > > >
> > > > use java.io.File.createTempFile() ? some interesting info at
> > > >
> > > http://www.veracode.com/blog/2009/01/how-boring-flaws-become-interesting/
> > > >
> > > > for directories there is a helpful posting at
> > > >
> > > http://stackoverflow.com/questions/617414/create-a-temporary-directory-in-java
> > > >
> > > > Thanks.
> > > >
> > > >
> > > > - --
> > > > Kurt Seifried -- Red Hat -- Product Security -- Cloud
> > > > PGP A90B F995 7350 148F 66BF 7554 160D 4553 5E26 7993
> > > > -----BEGIN PGP SIGNATURE-----
> > > > Version: GnuPG v1
> > > > Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> > > >
> > > > iQIcBAEBAgAGBQJTolCLAAoJEBYNRVNeJnmTrVYQAJ5glkD/0Ha5+F99Qj9ioNmm
> > > > ZnO4G6TqKctfiqW/X02wMocKLMRV8q5WI/nvs71hCoK5HaVmbtNrV71wE0omHLjB
> > > > smzFz6d8qZaTcOHdvgbSlWEGPjcVnESo0F3K0vgK2L/LtB5mgny6pHDn+c/cqrgt
> > > > Er4n+U3oXlkon/ksW+drWpKOpmGOhn7c4fbE45ci6KnzDbbGpGHF0fZL3lSEfJR0
> > > > 0D/HQzKIAJpI7VvZU8+/d/MHasndgJoAHmUCkTBYU55Vf5eYsm+xWZ1Mt46IyAap
> > > > crMTCHHE1GVUAexYbMxy+lohHbpl+pB/d////LzesJjByRSv87r+1oLhdwank3P9
> > > > Fz1h3sq57JyLFQIcpm4TS7xh3TaByFGCiA5G/mR+CkuS6sZEapSkviu/x7ygmOdG
> > > > cJKM+5CogeE1P1PWsoQ41JcSwfuWAfc5IODvkjLb3MfyoXJRaKcBVdVcdHBUK4BA
> > > > 7xcD9SbDsujxHOJLknFaO22uTtlrDS4yXJaNal6L9P7DCsSSrxG1PmmE+t5qrtYw
> > > > HQoz+RuOMhY/2FWJqOxa7ru99rIQmxxpWgoknUlT+yYJRfoub0kpibyJLBLy2SEx
> > > > xmdqe/i9nHCsGAworK4bEL2vLvsNBiJgdSHlzg7E5POI1tbveE12fIUmSgrgV+zO
> > > > WjPZ/O4oOj0FVWoeyQUN
> > > > =SUf5
> > > > -----END PGP SIGNATURE-----
> > >
> > > --
> > > Andrew Gaul
> > > http://gaul.org/
> > >
>
> --
> Andrew Gaul
> http://gaul.org/
> commit d7321e980b9e66fe95aea340d58ae26baef878d8
> Author: Andrew Gaul <gaul@...che.org>
> Date: Thu Jun 19 14:20:26 2014 -0700
>
> Securely create temporary directories
>
> This commit addresses a potential security issue where an attacker
> could hijack the ScriptBuilder payload by predicting the temporary
> directory name.
>
> diff --git a/compute/src/test/resources/initscript_with_jetty.sh b/compute/src/test/resources/initscript_with_jetty.sh
> index fca9683..741b6d2 100644
> --- a/compute/src/test/resources/initscript_with_jetty.sh
> +++ b/compute/src/test/resources/initscript_with_jetty.sh
> @@ -227,11 +227,11 @@ END_OF_JCLOUDS_SCRIPT
> installOpenJDK || return 1
> iptables -I INPUT 1 -p tcp --dport 8080 -j ACCEPT
> iptables-save
> - mkdir /tmp/$$
> - curl -q -s -S -L --connect-timeout 10 --max-time 600 --retry 20 -X GET http://archive.eclipse.org/jetty/8.1.8.v20121106/dist/jetty-distribution-8.1.8.v20121106.tar.gz |(mkdir -p /tmp/$$ &&cd /tmp/$$ &&tar -xpzf -)
> + export TAR_TEMP="$(mktemp -d)"
> + curl -q -s -S -L --connect-timeout 10 --max-time 600 --retry 20 -X GET http://archive.eclipse.org/jetty/8.1.8.v20121106/dist/jetty-distribution-8.1.8.v20121106.tar.gz |(mkdir -p "${TAR_TEMP}" &&cd "${TAR_TEMP}" &&tar -xpzf -)
> mkdir -p /usr/local/jetty
> - mv /tmp/$$/*/* /usr/local/jetty
> - rm -rf /tmp/$$
> + mv "${TAR_TEMP}"/*/* /usr/local/jetty
> + rm -rf "${TAR_TEMP}"
> chown -R web /usr/local/jetty
>
> END_OF_JCLOUDS_SCRIPT
> diff --git a/scriptbuilder/src/main/java/org/jclouds/scriptbuilder/domain/Statements.java b/scriptbuilder/src/main/java/org/jclouds/scriptbuilder/domain/Statements.java
> index e752e71..aa64d1b 100644
> --- a/scriptbuilder/src/main/java/org/jclouds/scriptbuilder/domain/Statements.java
> +++ b/scriptbuilder/src/main/java/org/jclouds/scriptbuilder/domain/Statements.java
> @@ -187,11 +187,12 @@ public class Statements {
> */
> public static Statement extractTargzAndFlattenIntoDirectory(URI tgz, String dest) {
> return new StatementList(ImmutableSet.<Statement> builder()
> - .add(exec("mkdir /tmp/$$"))
> - .add(extractTargzIntoDirectory(tgz, "/tmp/$$"))
> + .add(exec("export TAR_TEMP=\"$(mktemp -d)\""))
> + .add(extractTargzIntoDirectory(tgz, "\"${TAR_TEMP}\""))
> .add(exec("mkdir -p " + dest))
> - .add(exec("mv /tmp/$$/*/* " + dest))
> - .add(exec("rm -rf /tmp/$$")).build());
> + .add(exec("mv \"${TAR_TEMP}\"/*/* " + dest))
> + .add(exec("rm -rf \"${TAR_TEMP}\""))
> + .build());
> }
>
> public static Statement extractTargzIntoDirectory(URI targz, String directory) {
> diff --git a/scriptbuilder/src/test/java/org/jclouds/scriptbuilder/domain/StatementsTest.java b/scriptbuilder/src/test/java/org/jclouds/scriptbuilder/domain/StatementsTest.java
> index dbea7ab..72fc452 100644
> --- a/scriptbuilder/src/test/java/org/jclouds/scriptbuilder/domain/StatementsTest.java
> +++ b/scriptbuilder/src/test/java/org/jclouds/scriptbuilder/domain/StatementsTest.java
> @@ -51,11 +51,11 @@ public class StatementsTest {
> "/usr/local/maven");
> assertEquals(
> save.render(OsFamily.UNIX),
> - "mkdir /tmp/$$\n" +
> - "curl -q -s -S -L --connect-timeout 10 --max-time 600 --retry 20 -X GET http://www.us.apache.org/dist/maven/binaries/apache-maven-3.0.4-bin.tar.gz |(mkdir -p /tmp/$$ &&cd /tmp/$$ &&tar -xpzf -)\n" +
> + "export TAR_TEMP=\"$(mktemp -d)\"\n" +
> + "curl -q -s -S -L --connect-timeout 10 --max-time 600 --retry 20 -X GET http://www.us.apache.org/dist/maven/binaries/apache-maven-3.0.4-bin.tar.gz |(mkdir -p \"${TAR_TEMP}\" &&cd \"${TAR_TEMP}\" &&tar -xpzf -)\n" +
> "mkdir -p /usr/local/maven\n" +
> - "mv /tmp/$$/*/* /usr/local/maven\n" +
> - "rm -rf /tmp/$$\n");
> + "mv \"${TAR_TEMP}\"/*/* /usr/local/maven\n" +
> + "rm -rf \"${TAR_TEMP}\"\n");
> }
>
>
> diff --git a/scriptbuilder/src/test/java/org/jclouds/scriptbuilder/statements/ruby/InstallRubyGemsTest.java b/scriptbuilder/src/test/java/org/jclouds/scriptbuilder/statements/ruby/InstallRubyGemsTest.java
> index 0f17bb2..81128fe 100644
> --- a/scriptbuilder/src/test/java/org/jclouds/scriptbuilder/statements/ruby/InstallRubyGemsTest.java
> +++ b/scriptbuilder/src/test/java/org/jclouds/scriptbuilder/statements/ruby/InstallRubyGemsTest.java
> @@ -85,10 +85,10 @@ public class InstallRubyGemsTest {
> private static String installRubyGems(String version) {
> String script = "if ! hash gem 2>/dev/null; then\n"
> + "(\n"
> - + "mkdir /tmp/$$\n"
> + + "export TAR_TEMP=\"$(mktemp -d)\"\n"
> + "curl -q -s -S -L --connect-timeout 10 --max-time 600 --retry 20 -X GET http://production.cf.rubygems.org/rubygems/rubygems-"
> - + version + ".tgz |(mkdir -p /tmp/$$ &&cd /tmp/$$ &&tar -xpzf -)\n" + "mkdir -p /tmp/rubygems\n"
> - + "mv /tmp/$$/*/* /tmp/rubygems\n" + "rm -rf /tmp/$$\n" + "cd /tmp/rubygems\n"
> + + version + ".tgz |(mkdir -p \"${TAR_TEMP}\" &&cd \"${TAR_TEMP}\" &&tar -xpzf -)\n" + "mkdir -p /tmp/rubygems\n"
> + + "mv \"${TAR_TEMP}\"/*/* /tmp/rubygems\n" + "rm -rf \"${TAR_TEMP}\"\n" + "cd /tmp/rubygems\n"
> + "ruby setup.rb --no-format-executable\n" //
> + "rm -fr /tmp/rubygems\n" + //
> ")\n" + //
> diff --git a/scriptbuilder/src/test/resources/test_install_rubygems.sh b/scriptbuilder/src/test/resources/test_install_rubygems.sh
> index c9363d2..169250c 100644
> --- a/scriptbuilder/src/test/resources/test_install_rubygems.sh
> +++ b/scriptbuilder/src/test/resources/test_install_rubygems.sh
> @@ -1,10 +1,10 @@
> if ! hash gem 2>/dev/null; then
> (
> -mkdir /tmp/$$
> -curl -q -s -S -L --connect-timeout 10 --max-time 600 --retry 20 -X GET http://production.cf.rubygems.org/rubygems/rubygems-1.8.10.tgz |(mkdir -p /tmp/$$ &&cd /tmp/$$ &&tar -xpzf -)
> +export TAR_TEMP="$(mktemp -d)"
> +curl -q -s -S -L --connect-timeout 10 --max-time 600 --retry 20 -X GET http://production.cf.rubygems.org/rubygems/rubygems-1.8.10.tgz |(mkdir -p "${TAR_TEMP}" &&cd "${TAR_TEMP}" &&tar -xpzf -)
> mkdir -p /tmp/rubygems
> -mv /tmp/$$/*/* /tmp/rubygems
> -rm -rf /tmp/$$
> +mv "${TAR_TEMP}"/*/* /tmp/rubygems
> +rm -rf "${TAR_TEMP}"
> cd /tmp/rubygems
> ruby setup.rb --no-format-executable
> rm -fr /tmp/rubygems
> diff --git a/scriptbuilder/src/test/resources/test_install_rubygems_scriptbuilder.sh b/scriptbuilder/src/test/resources/test_install_rubygems_scriptbuilder.sh
> index 1c4bb5f..3df852e 100644
> --- a/scriptbuilder/src/test/resources/test_install_rubygems_scriptbuilder.sh
> +++ b/scriptbuilder/src/test/resources/test_install_rubygems_scriptbuilder.sh
> @@ -86,11 +86,11 @@ END_OF_JCLOUDS_SCRIPT
> trap 'echo $?>$INSTANCE_HOME/rc' 0 1 2 3 15
> if ! hash gem 2>/dev/null; then
> (
> - mkdir /tmp/$$
> - curl -q -s -S -L --connect-timeout 10 --max-time 600 --retry 20 -X GET http://production.cf.rubygems.org/rubygems/rubygems-1.8.10.tgz |(mkdir -p /tmp/$$ &&cd /tmp/$$ &&tar -xpzf -)
> + export TAR_TEMP="$(mktemp -d)"
> + curl -q -s -S -L --connect-timeout 10 --max-time 600 --retry 20 -X GET http://production.cf.rubygems.org/rubygems/rubygems-1.8.10.tgz |(mkdir -p "${TAR_TEMP}" &&cd "${TAR_TEMP}" &&tar -xpzf -)
> mkdir -p /tmp/rubygems
> - mv /tmp/$$/*/* /tmp/rubygems
> - rm -rf /tmp/$$
> + mv "${TAR_TEMP}"/*/* /tmp/rubygems
> + rm -rf "${TAR_TEMP}"
> cd /tmp/rubygems
> ruby setup.rb --no-format-executable
> rm -fr /tmp/rubygems
--
Andrew Gaul
http://gaul.org/
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
