From 4f897e0b37ab70f5fcb3cfc7caf95318c0f3aec8 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 11 May 2021 10:49:01 +0200 Subject: [PATCH] Add dataflow to track exported android components to getIntent calls --- .../semmle/code/java/dataflow/FlowSources.qll | 31 ++++++++++++- .../security/CWE-749/AndroidManifest.xml | 5 +++ .../security/CWE-749/BaseActivity.java | 14 ++++++ .../security/CWE-749/SafeActivity4.java | 36 ++++++++++++++++ .../security/CWE-749/UnsafeActivity4.java | 8 ++-- .../security/CWE-749/UnsafeActivity5.java | 43 +++++++++++++++++++ 6 files changed, 130 insertions(+), 7 deletions(-) create mode 100644 java/ql/test/query-tests/security/CWE-749/BaseActivity.java create mode 100644 java/ql/test/query-tests/security/CWE-749/SafeActivity4.java create mode 100644 java/ql/test/query-tests/security/CWE-749/UnsafeActivity5.java diff --git a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll index 3d3cac9ca4b6..fc4a951f4dc7 100644 --- a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll +++ b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll @@ -223,7 +223,7 @@ class ReverseDNSMethod extends Method { /** Android `Intent` that may have come from a hostile application. */ class AndroidIntentInput extends DataFlow::Node { - Type receiverType; + RefType receiverType; AndroidIntentInput() { exists(MethodAccess ma, AndroidGetIntentMethod m | @@ -242,7 +242,34 @@ class AndroidIntentInput extends DataFlow::Node { /** Exported Android `Intent` that may have come from a hostile application. */ class ExportedAndroidIntentInput extends RemoteFlowSource, AndroidIntentInput { - ExportedAndroidIntentInput() { receiverType.(ExportableAndroidComponent).isExported() } + ExportedAndroidIntentInput() { + receiverType.(ExportableAndroidComponent).isExported() + or + // This generates spurious results when both an unexported and exported activity extend a base class + // that provides data from the intent with a helper method. + // + // exists(ExportableAndroidComponent exported | exported.isExported() | + // exported.getASourceSupertype*() = receiverType + // ) + // or + exists(DataFlow::Node sink, ExportedAndroidComponentFlowConfig cfg | cfg.hasFlowTo(sink) | + sink.asExpr() = this.asExpr().(MethodAccess).getQualifier() + ) + } override string getSourceType() { result = "Exported Android intent source" } } + +private class ExportedAndroidComponentFlowConfig extends DataFlow2::Configuration { + ExportedAndroidComponentFlowConfig() { this = "FlowSources::ExportedAndroidComponentFlowConfig" } + + override predicate isSource(DataFlow2::Node src) { + src.asExpr().getType().(ExportableAndroidComponent).isExported() + } + + override predicate isSink(DataFlow2::Node sink) { + exists(AndroidGetIntentMethod m, MethodAccess ma | ma.getMethod() = m | + sink = DataFlow::getInstanceArgument(ma) + ) + } +} diff --git a/java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml index b215e4d34669..91413466f4dd 100755 --- a/java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml @@ -42,9 +42,14 @@ + + + + + diff --git a/java/ql/test/query-tests/security/CWE-749/BaseActivity.java b/java/ql/test/query-tests/security/CWE-749/BaseActivity.java new file mode 100644 index 000000000000..81536581a0a3 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-749/BaseActivity.java @@ -0,0 +1,14 @@ +package com.example.app; + +import android.app.Activity; + +public abstract class BaseActivity extends Activity { + + protected String getIntentUrl() { + return getIntent().getStringExtra("url"); + } + + protected String getBundleUrl() { + return getIntent().getExtras().getString("url"); + } +} diff --git a/java/ql/test/query-tests/security/CWE-749/SafeActivity4.java b/java/ql/test/query-tests/security/CWE-749/SafeActivity4.java new file mode 100644 index 000000000000..54f52f26dfc6 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-749/SafeActivity4.java @@ -0,0 +1,36 @@ +package com.example.app; + +import android.os.Bundle; + +import android.webkit.WebSettings; +import android.webkit.WebView; +import android.webkit.WebViewClient; + +public class SafeActivity4 extends BaseActivity { + // Test onCreate with both JavaScript and cross-origin resource access enabled while taking + // remote user inputs from bundle extras. + // The Activity is implicitly not exported. + public void onCreate(Bundle savedInstanceState) { + super.onCreate(savedInstanceState); + setContentView(-1); + + WebView wv = (WebView) findViewById(-1); + WebSettings webSettings = wv.getSettings(); + + webSettings.setJavaScriptEnabled(true); + webSettings.setAllowFileAccessFromFileURLs(true); + + wv.setWebViewClient(new WebViewClient() { + @Override + public boolean shouldOverrideUrlLoading(WebView view, String url) { + view.loadUrl(url); + return true; + } + }); + + String thisUrl = getIntentUrl(); + wv.loadUrl(thisUrl); // Safe + thisUrl = getBundleUrl(); + wv.loadUrl(thisUrl); // Safe + } +} diff --git a/java/ql/test/query-tests/security/CWE-749/UnsafeActivity4.java b/java/ql/test/query-tests/security/CWE-749/UnsafeActivity4.java index 992e753f38d0..24a5a69639eb 100644 --- a/java/ql/test/query-tests/security/CWE-749/UnsafeActivity4.java +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeActivity4.java @@ -14,10 +14,6 @@ public class UnsafeActivity4 extends Activity { * remote user inputs from bundle extras. * * The Activity is explicitly exported. - * - * Note this case of invoking a utility method that takes an Activity and then calls - * `a.getIntent().getStringExtra(...)` is not yet detected thus is beyond what the query is - * capable of. */ public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); @@ -38,7 +34,9 @@ public boolean shouldOverrideUrlLoading(WebView view, String url) { }); String thisUrl = IntentUtils.getIntentUrl(this); + wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess thisUrl = IntentUtils.getBundleUrl(this); - wv.loadUrl(thisUrl); // $ MISSING: hasUnsafeAndroidAccess + wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess + } } diff --git a/java/ql/test/query-tests/security/CWE-749/UnsafeActivity5.java b/java/ql/test/query-tests/security/CWE-749/UnsafeActivity5.java new file mode 100644 index 000000000000..16c521c40494 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeActivity5.java @@ -0,0 +1,43 @@ +package com.example.app; + +import android.os.Bundle; + +import android.webkit.WebSettings; +import android.webkit.WebView; +import android.webkit.WebViewClient; + +public class UnsafeActivity5 extends BaseActivity { + /* + * Test onCreate with both JavaScript and cross-origin resource access enabled while taking + * remote user inputs from bundle extras. + * + * The Activity is explicitly exported. + * + * Note this case of invoking a helper method from a base class that then calls to + * `getIntent().getStringExtra(...)` is not yet detected thus is beyond what the query is + * capable of. + */ + public void onCreate(Bundle savedInstanceState) { + super.onCreate(savedInstanceState); + setContentView(-1); + + WebView wv = (WebView) findViewById(-1); + WebSettings webSettings = wv.getSettings(); + + webSettings.setJavaScriptEnabled(true); + webSettings.setAllowFileAccessFromFileURLs(true); + + wv.setWebViewClient(new WebViewClient() { + @Override + public boolean shouldOverrideUrlLoading(WebView view, String url) { + view.loadUrl(url); + return true; + } + }); + + String thisUrl = getIntentUrl(); + wv.loadUrl(thisUrl); // $ MISSING: hasUnsafeAndroidAccess + thisUrl = getBundleUrl(); + wv.loadUrl(thisUrl); // $ MISSING: hasUnsafeAndroidAccess + } +}