From 2ca95166d916cc89bf2d7086aa664ef3757c95e4 Mon Sep 17 00:00:00 2001 From: Porcuiney Hairs Date: Wed, 13 Jan 2021 01:32:54 +0530 Subject: [PATCH 1/3] Java : add query to detect insecure loading of Dex File --- .../CWE/CWE-094/InsecureDexLoading.qhelp | 44 ++++++++ .../CWE/CWE-094/InsecureDexLoading.ql | 20 ++++ .../CWE/CWE-094/InsecureDexLoading.qll | 100 ++++++++++++++++++ .../CWE/CWE-094/InsecureDexLoadingBad.java | 32 ++++++ .../CWE/CWE-094/InsecureDexLoadingGood.java | 23 ++++ 5 files changed, 219 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.ql create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qll create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoadingBad.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoadingGood.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qhelp new file mode 100644 index 00000000000..feda3af3fc2 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qhelp @@ -0,0 +1,44 @@ + + + +

+Shared world writable storage spaces are not secure to load Dex libraries from. A malicious actor can replace a dex file with a maliciously crafted file +which when loaded by the app can lead to code execution. +

+ + + +

+ Loading a file from private storage instead of a world writable one can prevent this issue. + As the attacker cannot access files stored by the app in its private storage. +

+ + + +

+ The following example loads a Dex file from a shared world writable location. in this case, + since the `/sdcard` directory is on external storage, any one can read/write to the location. + bypassing all Android security policies. Hence, this is insecure. +

+ + +

+ The next example loads a Dex file stored inside the app's private storage. + This is not exploitable as nobody else except the app can access the data stored here. +

+ + + + +
  • + Android Documentation: + Data and file storage overview + . +
    • +
  • + Android Documentation: + DexClassLoader + . +
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.ql b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.ql new file mode 100644 index 00000000000..58d9844d38a --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.ql @@ -0,0 +1,20 @@ +/** + * @name Insecure loading of an Android Dex File + * @description Loading a DEX library located in a world-readable/ writable location such as + * a SD card can cause arbitary code execution vulnerabilities. + * @kind path-problem + * @problem.severity error + * @precision high + * @id java/android-insecure-dex-loading + * @tags security + * external/cwe/cwe-094 + */ + +import java +import InsecureDexLoading +import DataFlow::PathGraph + +from DataFlow::PathNode source, DataFlow::PathNode sink, InsecureDexConfiguration conf +where conf.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "Potential arbitary code execution due to $@.", + source.getNode(), "a value loaded from a world readable/writable source." diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qll b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qll new file mode 100644 index 00000000000..2a4b387be7e --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qll @@ -0,0 +1,100 @@ +import java +import semmle.code.java.dataflow.FlowSources + +/** + * A taint-tracking configuration fordetecting unsafe use of a + * `DexClassLoader` by an Android app. + */ +class InsecureDexConfiguration extends TaintTracking::Configuration { + InsecureDexConfiguration() { this = "Insecure Dex File Load" } + + override predicate isSource(DataFlow::Node source) { source instanceof InsecureDexSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof InsecureDexSink } + + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { + flowStep(pred, succ) + } +} + +/** A data flow source for insecure Dex class loading vulnerabilities. */ +abstract class InsecureDexSource extends DataFlow::Node { } + +/** A data flow sink for insecure Dex class loading vulnerabilities. */ +abstract class InsecureDexSink extends DataFlow::Node { } + +private predicate flowStep(DataFlow::Node pred, DataFlow::Node succ) { + // propagate from a `java.io.File` via the `File.getAbsolutePath` call. + exists(MethodAccess m | + m.getMethod().getDeclaringType() instanceof TypeFile and + m.getMethod().hasName("getAbsolutePath") and + m.getQualifier() = pred.asExpr() and + m = succ.asExpr() + ) + or + // propagate from a `java.io.File` via the `File.toString` call. + exists(MethodAccess m | + m.getMethod().getDeclaringType() instanceof TypeFile and + m.getMethod().hasName("toString") and + m.getQualifier() = pred.asExpr() and + m = succ.asExpr() + ) + or + // propagate to newly created `File` if the parent directory of the new `File` is tainted + exists(ConstructorCall cc | + cc.getConstructedType() instanceof TypeFile and + cc.getArgument(0) = pred.asExpr() and + cc = succ.asExpr() + ) +} + +/** + * An argument to a `DexClassLoader` call taken as a sink for + * insecure Dex class loading vulnerabilities. + */ +private class DexClassLoader extends InsecureDexSink { + DexClassLoader() { + exists(ConstructorCall cc | + cc.getConstructedType().hasQualifiedName("dalvik.system", "DexClassLoader") + | + this.asExpr() = cc.getArgument(0) + ) + } +} + +/** + * An `File` instance which reads from an SD card + * taken as a source for insecure Dex class loading vulnerabilities. + */ +private class ExternalFile extends InsecureDexSource { + ExternalFile() { + exists(ConstructorCall cc, Argument a | + cc.getConstructedType() instanceof TypeFile and + a = cc.getArgument(0) and + a.(CompileTimeConstantExpr).getStringValue().matches("%sdcard%") + | + this.asExpr() = a + ) + } +} + +/** + * A directory or file which may be stored in an world writable directory + * taken as a source for insecure Dex class loading vulnerabilities. + */ +private class ExternalStorageDirSource extends InsecureDexSource { + ExternalStorageDirSource() { + exists(Method m | + m.getDeclaringType().hasQualifiedName("android.os", "Environment") and + m.hasName("getExternalStorageDirectory") + or + m.getDeclaringType().hasQualifiedName("android.content", "Context") and + m.hasName([ + "getExternalFilesDir", "getExternalFilesDirs", "getExternalMediaDirs", + "getExternalCacheDir", "getExternalCacheDirs" + ]) + | + this.asExpr() = m.getAReference() + ) + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoadingBad.java b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoadingBad.java new file mode 100644 index 00000000000..d8fdd828f4f --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoadingBad.java @@ -0,0 +1,32 @@ + +import android.app.Application; +import android.content.Context; +import android.content.pm.PackageInfo; +import android.os.Bundle; + +import dalvik.system.DexClassLoader; +import dalvik.system.DexFile; + +public class InsecureDexLoading extends Application { + @Override + public void onCreate() { + super.onCreate(); + updateChecker(); + } + + private void updateChecker() { + try { + File file = new File("/sdcard/updater.apk"); + if (file.exists() && file.isFile() && file.length() <= 1000) { + DexClassLoader cl = new DexClassLoader(file.getAbsolutePath(), getCacheDir().getAbsolutePath(), null, + getClassLoader()); + int version = (int) cl.loadClass("my.package.class").getDeclaredMethod("myMethod").invoke(null); + if (Build.VERSION.SDK_INT < version) { + Toast.makeText(this, "Securely loaded Dex!", Toast.LENGTH_LONG).show(); + } + } + } catch (Exception e) { + // ignore + } + } +} \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoadingGood.java b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoadingGood.java new file mode 100644 index 00000000000..e45e3938f7b --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoadingGood.java @@ -0,0 +1,23 @@ +public class SecureDexLoading extends Application { + @Override + public void onCreate() { + super.onCreate(); + updateChecker(); + } + + private void updateChecker() { + try { + File file = new File(getCacheDir() + "/updater.apk"); + if (file.exists() && file.isFile() && file.length() <= 1000) { + DexClassLoader cl = new DexClassLoader(file.getAbsolutePath(), getCacheDir().getAbsolutePath(), null, + getClassLoader()); + int version = (int) cl.loadClass("my.package.class").getDeclaredMethod("myMethod").invoke(null); + if (Build.VERSION.SDK_INT < version) { + Toast.makeText(this, "Securely loaded Dex!", Toast.LENGTH_LONG).show(); + } + } + } catch (Exception e) { + // ignore + } + } +} \ No newline at end of file From 8687c5c14533ac35859cedca383217c65bce5539 Mon Sep 17 00:00:00 2001 From: porcupineyhairs <61983466+porcupineyhairs@users.noreply.github.com> Date: Sat, 10 Apr 2021 04:18:35 +0530 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Chris Smowton --- .../Security/CWE/CWE-094/InsecureDexLoading.qhelp | 12 ++++++------ .../Security/CWE/CWE-094/InsecureDexLoading.ql | 8 ++++---- .../Security/CWE/CWE-094/InsecureDexLoading.qll | 4 ++-- .../Security/CWE/CWE-094/InsecureDexLoadingBad.java | 4 ++-- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qhelp index feda3af3fc2..4a5555c1390 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qhelp @@ -2,29 +2,29 @@

    -Shared world writable storage spaces are not secure to load Dex libraries from. A malicious actor can replace a dex file with a maliciously crafted file +It is dangerous to load Dex libraries from shared world-writable storage spaces. A malicious actor can replace a dex file with a maliciously crafted file which when loaded by the app can lead to code execution.

    - Loading a file from private storage instead of a world writable one can prevent this issue. - As the attacker cannot access files stored by the app in its private storage. + Loading a file from private storage instead of a world-writable one can prevent this issue, + because the attacker cannot access files stored there.

    - The following example loads a Dex file from a shared world writable location. in this case, - since the `/sdcard` directory is on external storage, any one can read/write to the location. + The following example loads a Dex file from a shared world-writable location. in this case, + since the `/sdcard` directory is on external storage, anyone can read/write to the location. bypassing all Android security policies. Hence, this is insecure.

    The next example loads a Dex file stored inside the app's private storage. - This is not exploitable as nobody else except the app can access the data stored here. + This is not exploitable as nobody else except the app can access the data stored there.

    diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.ql b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.ql index 58d9844d38a..bae3ed63d70 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.ql @@ -1,7 +1,7 @@ /** * @name Insecure loading of an Android Dex File - * @description Loading a DEX library located in a world-readable/ writable location such as - * a SD card can cause arbitary code execution vulnerabilities. + * @description Loading a DEX library located in a world-writable location such as + * an SD card can lead to arbitrary code execution vulnerabilities. * @kind path-problem * @problem.severity error * @precision high @@ -16,5 +16,5 @@ import DataFlow::PathGraph from DataFlow::PathNode source, DataFlow::PathNode sink, InsecureDexConfiguration conf where conf.hasFlowPath(source, sink) -select sink.getNode(), source, sink, "Potential arbitary code execution due to $@.", - source.getNode(), "a value loaded from a world readable/writable source." +select sink.getNode(), source, sink, "Potential arbitrary code execution due to $@.", + source.getNode(), "a value loaded from a world-writable source." diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qll b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qll index 2a4b387be7e..0ee0954216e 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qll @@ -2,7 +2,7 @@ import java import semmle.code.java.dataflow.FlowSources /** - * A taint-tracking configuration fordetecting unsafe use of a + * A taint-tracking configuration detecting unsafe use of a * `DexClassLoader` by an Android app. */ class InsecureDexConfiguration extends TaintTracking::Configuration { @@ -63,7 +63,7 @@ private class DexClassLoader extends InsecureDexSink { } /** - * An `File` instance which reads from an SD card + * A `File` instance which reads from an SD card * taken as a source for insecure Dex class loading vulnerabilities. */ private class ExternalFile extends InsecureDexSource { diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoadingBad.java b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoadingBad.java index d8fdd828f4f..869b6bc571c 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoadingBad.java +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoadingBad.java @@ -22,11 +22,11 @@ public class InsecureDexLoading extends Application { getClassLoader()); int version = (int) cl.loadClass("my.package.class").getDeclaredMethod("myMethod").invoke(null); if (Build.VERSION.SDK_INT < version) { - Toast.makeText(this, "Securely loaded Dex!", Toast.LENGTH_LONG).show(); + Toast.makeText(this, "Loaded Dex!", Toast.LENGTH_LONG).show(); } } } catch (Exception e) { // ignore } } -} \ No newline at end of file +} From 11bf9827289b1103ead2af9f3d7c0ccc30e235c1 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Mon, 12 Apr 2021 14:36:42 +0100 Subject: [PATCH 3/3] Remove superfluous linebreaks in qhelp file --- .../Security/CWE/CWE-094/InsecureDexLoading.qhelp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qhelp index 4a5555c1390..216bdeaebf3 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureDexLoading.qhelp @@ -32,13 +32,11 @@ which when loaded by the app can lead to code execution.
  • Android Documentation: - Data and file storage overview - . + Data and file storage overview.
  • Android Documentation: - DexClassLoader - . + DexClassLoader.