oss-sec mailing list archives

Previous By Date Next
Previous By Thread Next

Re: TMP flaw in rackspace jclouds?

From: Andrew Gaul <gaul () apache org>
Date: Mon, 23 Jun 2014 16:17:48 -0700
[bcc: jclouds private list]

Since oss-security () lists 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 () apache 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 () apache 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/
Previous By Date Next
Previous By Thread Next

Current thread: