From 458b89bf5f88403c49021c21dd6a0bc6748d9c32 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 5 May 2021 11:57:01 +0200 Subject: [PATCH 01/25] Added Android stubs --- .../stubs/android/android/app/Activity.java | 38 ++++ .../stubs/android/android/content/Intent.java | 196 ++++++++++++++++++ .../test/stubs/android/android/os/Bundle.java | 87 ++++++++ .../android/android/util/AttributeSet.java | 74 +++++++ .../android/android/webkit/WebSettings.java | 31 +++ .../stubs/android/android/webkit/WebView.java | 178 ++++++++++++++++ .../android/android/webkit/WebViewClient.java | 39 ++++ .../stubs/android/com/android/internal/R.java | 13 ++ 8 files changed, 656 insertions(+) create mode 100644 java/ql/test/stubs/android/android/app/Activity.java create mode 100644 java/ql/test/stubs/android/android/content/Intent.java create mode 100644 java/ql/test/stubs/android/android/os/Bundle.java create mode 100644 java/ql/test/stubs/android/android/util/AttributeSet.java create mode 100644 java/ql/test/stubs/android/android/webkit/WebSettings.java create mode 100644 java/ql/test/stubs/android/android/webkit/WebView.java create mode 100644 java/ql/test/stubs/android/android/webkit/WebViewClient.java create mode 100644 java/ql/test/stubs/android/com/android/internal/R.java diff --git a/java/ql/test/stubs/android/android/app/Activity.java b/java/ql/test/stubs/android/android/app/Activity.java new file mode 100644 index 00000000000..0ad9be7975f --- /dev/null +++ b/java/ql/test/stubs/android/android/app/Activity.java @@ -0,0 +1,38 @@ +/* + * Copyright (C) 2006 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.app; + +import android.content.Intent; +import android.os.Bundle; + +public class Activity { + + public void onCreate(Bundle bundle) { + } + + public Intent getIntent() { + return null; + } + + public Object findViewById(int id) { + return null; + } + + public void setContentView(int view) { + } + +} diff --git a/java/ql/test/stubs/android/android/content/Intent.java b/java/ql/test/stubs/android/android/content/Intent.java new file mode 100644 index 00000000000..32b167509d3 --- /dev/null +++ b/java/ql/test/stubs/android/android/content/Intent.java @@ -0,0 +1,196 @@ +/* + * Copyright (C) 2006 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.content; + +import android.net.Uri; +import android.os.Bundle; + +import java.io.PrintWriter; +import java.net.URISyntaxException; +import java.util.Set; + +public class Intent implements Cloneable { + + public static Intent createChooser(Intent target, CharSequence title) { + return null; + } + + public Intent() { + } + + public Intent(Intent o) { + } + + @Override + public Object clone() { + return null; + } + + public Intent(String action) { + } + + public Intent(String action, Uri uri) { + } + + public Intent(Context packageContext, Class cls) { + } + + public Intent(String action, Uri uri, Context packageContext, Class cls) { + } + + public static Intent makeMainSelectorActivity(String selectorAction, String selectorCategory) { + return null; + } + + public static Intent getIntent(String uri) throws URISyntaxException { + return null; + } + + public static Intent getIntentOld(String uri) throws URISyntaxException { + return null; + } + + public static void printIntentArgsHelp(PrintWriter pw, String prefix) { + } + + public boolean hasCategory(String category) { + return false; + } + + public Set getCategories() { + return null; + } + + public int getContentUserHint() { + return 0; + } + + public String getLaunchToken() { + return null; + } + + public void setLaunchToken(String launchToken) { + } + + public boolean hasExtra(String name) { + return false; + } + + public boolean hasFileDescriptors() { + return false; + } + + public void setAllowFds(boolean allowFds) { + } + + public void setDefusable(boolean defusable) { + } + + public Object getExtra(String name) { + return null; + } + + public boolean getBooleanExtra(String name, boolean defaultValue) { + return false; + } + + public byte getByteExtra(String name, byte defaultValue) { + return 0; + } + + public short getShortExtra(String name, short defaultValue) { + return 0; + } + + public char getCharExtra(String name, char defaultValue) { + return '0'; + } + + public int getIntExtra(String name, int defaultValue) { + return 0; + } + + public long getLongExtra(String name, long defaultValue) { + return 0; + } + + public float getFloatExtra(String name, float defaultValue) { + return 0; + } + + public double getDoubleExtra(String name, double defaultValue) { + return 0; + } + + public String getStringExtra(String string) { + return null; + } + + public void removeUnsafeExtras() { + } + + public boolean canStripForHistory() { + return false; + } + + public Intent maybeStripForHistory() { + return null; + } + + public boolean isExcludingStopped() { + return false; + } + + public void removeCategory(String category) { + } + + public void prepareToLeaveUser(int userId) { + } + + public boolean filterEquals(Intent other) { + return false; + } + + public int filterHashCode() { + return 0; + } + + @Override + public String toString() { + return null; + } + + public String toInsecureString() { + return null; + } + + public String toInsecureStringWithClip() { + return null; + } + + public String toShortString(boolean secure, boolean comp, boolean extras, boolean clip) { + return null; + } + + public void toShortString(StringBuilder b, boolean secure, boolean comp, boolean extras, boolean clip) { + } + + public Bundle getExtras() { + return null; + } + +} diff --git a/java/ql/test/stubs/android/android/os/Bundle.java b/java/ql/test/stubs/android/android/os/Bundle.java new file mode 100644 index 00000000000..fa49a640f30 --- /dev/null +++ b/java/ql/test/stubs/android/android/os/Bundle.java @@ -0,0 +1,87 @@ +/* + * Copyright (C) 2007 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.os; + +public final class Bundle { + public Bundle() { + } + + public Bundle(ClassLoader loader) { + } + + public Bundle(int capacity) { + } + + public Bundle(Bundle b) { + } + + public static Bundle forPair(String key, String value) { + return null; + } + + public boolean setAllowFds(boolean allowFds) { + return false; + } + + public void setDefusable(boolean defusable) { + } + + public static Bundle setDefusable(Bundle bundle, boolean defusable) { + return null; + } + + @Override + public Object clone() { + return null; + } + + public Bundle deepCopy() { + return null; + } + + public void remove(String key) { + } + + public void putAll(Bundle bundle) { + } + + public int getSize() { + return 0; + } + + public boolean hasFileDescriptors() { + return false; + } + + public Bundle filterValues() { + return null; + } + + @Override + public synchronized String toString() { + return null; + } + + public synchronized String toShortString() { + return null; + } + + public String getString(String string) { + return null; + } + +} diff --git a/java/ql/test/stubs/android/android/util/AttributeSet.java b/java/ql/test/stubs/android/android/util/AttributeSet.java new file mode 100644 index 00000000000..27159b91485 --- /dev/null +++ b/java/ql/test/stubs/android/android/util/AttributeSet.java @@ -0,0 +1,74 @@ +/* + * Copyright (C) 2006 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.util; + +public interface AttributeSet { + public int getAttributeCount(); + + default String getAttributeNamespace (int index) { + return null; + } + + public String getAttributeName(int index); + + public String getAttributeValue(int index); + + public String getAttributeValue(String namespace, String name); + + public String getPositionDescription(); + + public int getAttributeNameResource(int index); + + public int getAttributeListValue(String namespace, String attribute, + String[] options, int defaultValue); + + public boolean getAttributeBooleanValue(String namespace, String attribute, + boolean defaultValue); + + public int getAttributeResourceValue(String namespace, String attribute, + int defaultValue); + + public int getAttributeIntValue(String namespace, String attribute, + int defaultValue); + + public int getAttributeUnsignedIntValue(String namespace, String attribute, + int defaultValue); + + public float getAttributeFloatValue(String namespace, String attribute, + float defaultValue); + + public int getAttributeListValue(int index, String[] options, int defaultValue); + + public boolean getAttributeBooleanValue(int index, boolean defaultValue); + + public int getAttributeResourceValue(int index, int defaultValue); + + public int getAttributeIntValue(int index, int defaultValue); + + public int getAttributeUnsignedIntValue(int index, int defaultValue); + + public float getAttributeFloatValue(int index, float defaultValue); + + public String getIdAttribute(); + + public String getClassAttribute(); + + public int getIdAttributeResourceValue(int defaultValue); + + public int getStyleAttribute(); + +} diff --git a/java/ql/test/stubs/android/android/webkit/WebSettings.java b/java/ql/test/stubs/android/android/webkit/WebSettings.java new file mode 100644 index 00000000000..495c2b0a390 --- /dev/null +++ b/java/ql/test/stubs/android/android/webkit/WebSettings.java @@ -0,0 +1,31 @@ +/* + * Copyright (C) 2007 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.webkit; + +// create a class derived from this, and return an instance of it in the +// WebViewProvider.getWebSettingsProvider() method implementation. +public abstract class WebSettings { + + public @interface CacheMode { + } + + public void setJavaScriptEnabled(boolean b) { + } + + public void setAllowUniversalAccessFromFileURLs(boolean b) { + } +} \ No newline at end of file diff --git a/java/ql/test/stubs/android/android/webkit/WebView.java b/java/ql/test/stubs/android/android/webkit/WebView.java new file mode 100644 index 00000000000..6f09c7448d4 --- /dev/null +++ b/java/ql/test/stubs/android/android/webkit/WebView.java @@ -0,0 +1,178 @@ +/* + * Copyright (C) 2008 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.webkit; + +import android.content.Context; +import android.util.AttributeSet; + +public class WebView { + public WebView(Context context) { + } + + public WebView(Context context, AttributeSet attrs) { + } + + public WebView(Context context, AttributeSet attrs, int defStyle) { + } + + public void setHorizontalScrollbarOverlay(boolean overlay) { + } + + public void setVerticalScrollbarOverlay(boolean overlay) { + } + + public boolean overlayHorizontalScrollbar() { + return false; + } + + public boolean overlayVerticalScrollbar() { + return false; + } + + public void savePassword(String host, String username, String password) { + } + + public void setHttpAuthUsernamePassword(String host, String realm, String username, String password) { + } + + public String[] getHttpAuthUsernamePassword(String host, String realm) { + return null; + } + + public void destroy() { + } + + public static void enablePlatformNotifications() { + } + + public static void disablePlatformNotifications() { + } + + public void loadUrl(String url) { + } + + public void loadData(String data, String mimeType, String encoding) { + } + + public void loadDataWithBaseURL(String baseUrl, String data, String mimeType, String encoding, String failUrl) { + } + + public void stopLoading() { + } + + public void reload() { + } + + public boolean canGoBack() { + return false; + } + + public void goBack() { + } + + public boolean canGoForward() { + return false; + } + + public void goForward() { + } + + public boolean canGoBackOrForward(int steps) { + return false; + } + + public void goBackOrForward(int steps) { + } + + public boolean pageUp(boolean top) { + return false; + } + + public boolean pageDown(boolean bottom) { + return false; + } + + public void clearView() { + } + + public float getScale() { + return 0; + } + + public void setInitialScale(int scaleInPercent) { + } + + public void invokeZoomPicker() { + } + + public String getUrl() { + return null; + } + + public String getTitle() { + return null; + } + + public int getProgress() { + return 0; + } + + public int getContentHeight() { + return 0; + } + + public void pauseTimers() { + } + + public void resumeTimers() { + } + + public void clearCache() { + } + + public void clearFormData() { + } + + public void clearHistory() { + } + + public void clearSslPreferences() { + } + + public static String findAddress(String addr) { + return null; + } + + public void addJavascriptInterface(Object obj, String interfaceName) { + } + + public boolean zoomIn() { + return false; + } + + public boolean zoomOut() { + return false; + } + + public WebSettings getSettings() { + return null; + } + + public void setWebViewClient(WebViewClient webViewClient) { + } + +} diff --git a/java/ql/test/stubs/android/android/webkit/WebViewClient.java b/java/ql/test/stubs/android/android/webkit/WebViewClient.java new file mode 100644 index 00000000000..ba874c389c6 --- /dev/null +++ b/java/ql/test/stubs/android/android/webkit/WebViewClient.java @@ -0,0 +1,39 @@ +/* + * Copyright (C) 2008 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.webkit; + +public class WebViewClient { + public boolean shouldOverrideUrlLoading(WebView view, String url) { + return false; + } + + public void onPageFinished(WebView view, String url) { + } + + public void onLoadResource(WebView view, String url) { + } + + public void onPageCommitVisible(WebView view, String url) { + } + + public void onReceivedError(WebView view, int errorCode, String description, String failingUrl) { + } + + public void onScaleChanged(WebView view, float oldScale, float newScale) { + } + +} diff --git a/java/ql/test/stubs/android/com/android/internal/R.java b/java/ql/test/stubs/android/com/android/internal/R.java new file mode 100644 index 00000000000..2fca344c2aa --- /dev/null +++ b/java/ql/test/stubs/android/com/android/internal/R.java @@ -0,0 +1,13 @@ +package com.android.internal; + +public class R { + + public static class id { + public static int my_webview; + } + + public static class layout { + public static int webview; + } + +} From be50e8f30cdec349eb5a6f680336561d9f51145f Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 5 May 2021 11:59:49 +0200 Subject: [PATCH 02/25] Moved from experimental to standard --- .../CWE/CWE-749/UnsafeAndroidAccess.java | 0 .../CWE/CWE-749/UnsafeAndroidAccess.qhelp | 0 .../CWE/CWE-749/UnsafeAndroidAccess.ql | 34 ++++++++++++ .../java/security/UnsafeAndroidAccess.qll} | 52 +++++-------------- 4 files changed, 47 insertions(+), 39 deletions(-) rename java/ql/src/{experimental => }/Security/CWE/CWE-749/UnsafeAndroidAccess.java (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp (100%) create mode 100644 java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql rename java/ql/src/{experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.ql => semmle/code/java/security/UnsafeAndroidAccess.qll} (61%) diff --git a/java/ql/src/experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.java b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.java similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.java rename to java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp rename to java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp diff --git a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql new file mode 100644 index 00000000000..831c2d0b01d --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql @@ -0,0 +1,34 @@ +/** + * @name Unsafe resource fetching in Android webview + * @description JavaScript rendered inside WebViews can access any protected + * application file and web resource from any origin + * @kind path-problem + * @problem.severity warning + * @precision medium + * @id java/android/unsafe-android-webview-fetch + * @tags security + * external/cwe/cwe-749 + * external/cwe/cwe-079 + */ + +import java +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.security.UnsafeAndroidAccess +import DataFlow::PathGraph + +/** + * Taint configuration tracking flow from untrusted inputs to `loadUrl` or `postUrl` calls. + */ +class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration { + FetchUntrustedResourceConfiguration() { this = "FetchUntrustedResourceConfiguration" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof FetchUntrustedResourceSink } +} + +from DataFlow::PathNode source, DataFlow::PathNode sink, FetchUntrustedResourceConfiguration conf +where conf.hasFlowPath(source, sink) +select sink.getNode().(FetchUntrustedResourceSink).getMethodAccess(), source, sink, + "Unsafe resource fetching in Android webview due to $@.", source.getNode(), + sink.getNode().(FetchUntrustedResourceSink).getSinkType() diff --git a/java/ql/src/experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.ql b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll similarity index 61% rename from java/ql/src/experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.ql rename to java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll index 24755e64f13..e741992de35 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.ql +++ b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll @@ -1,26 +1,12 @@ -/** - * @name Unsafe resource fetching in Android webview - * @description JavaScript rendered inside WebViews can access any protected - * application file and web resource from any origin - * @kind path-problem - * @problem.severity warning - * @precision medium - * @id java/android/unsafe-android-webview-fetch - * @tags security - * external/cwe/cwe-749 - * external/cwe/cwe-079 - */ - import java -import semmle.code.java.frameworks.android.Intent import semmle.code.java.frameworks.android.WebView -import semmle.code.java.dataflow.FlowSources -import DataFlow::PathGraph +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.ExternalFlow /** * Methods allowing any-local-file and cross-origin access in the WebSettings class */ -class CrossOriginAccessMethod extends Method { +private class CrossOriginAccessMethod extends Method { CrossOriginAccessMethod() { this.getDeclaringType() instanceof TypeWebSettings and ( @@ -33,7 +19,7 @@ class CrossOriginAccessMethod extends Method { /** * `setJavaScriptEnabled` method for the webview */ -class AllowJavaScriptMethod extends Method { +private class AllowJavaScriptMethod extends Method { AllowJavaScriptMethod() { this.getDeclaringType() instanceof TypeWebSettings and this.hasName("setJavaScriptEnabled") @@ -43,7 +29,7 @@ class AllowJavaScriptMethod extends Method { /** * Holds if a call to `v.setJavaScriptEnabled(true)` exists */ -predicate isJSEnabled(Variable v) { +private predicate isJSEnabled(Variable v) { exists(MethodAccess jsa | v.getAnAccess() = jsa.getQualifier() and jsa.getMethod() instanceof AllowJavaScriptMethod and @@ -54,7 +40,7 @@ predicate isJSEnabled(Variable v) { /** * Fetch URL method call on the `android.webkit.WebView` object */ -class FetchResourceMethodAccess extends MethodAccess { +private class FetchResourceMethodAccess extends MethodAccess { FetchResourceMethodAccess() { this.getMethod().getDeclaringType() instanceof TypeWebView and this.getMethod().hasName(["loadUrl", "postUrl"]) @@ -64,12 +50,14 @@ class FetchResourceMethodAccess extends MethodAccess { /** * Holds if `ma` loads URL `sink` */ -predicate fetchResource(FetchResourceMethodAccess ma, Expr sink) { sink = ma.getArgument(0) } +private predicate fetchResource(FetchResourceMethodAccess ma, Expr sink) { + sink = ma.getArgument(0) +} /** * A URL argument to a `loadUrl` or `postUrl` call, considered as a sink. */ -class UrlResourceSink extends DataFlow::ExprNode { +private class UrlResourceSink extends DataFlow::ExprNode { UrlResourceSink() { fetchResource(_, this.getExpr()) } /** Gets the fetch method that fetches this sink URL. */ @@ -103,18 +91,10 @@ class UrlResourceSink extends DataFlow::ExprNode { } } -/** - * Taint configuration tracking flow from untrusted inputs to `loadUrl` or `postUrl` calls. - */ -class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration { - FetchUntrustedResourceConfiguration() { this = "FetchUntrustedResourceConfiguration" } - - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node sink) { - sink instanceof UrlResourceSink and +class FetchUntrustedResourceSink extends UrlResourceSink { + FetchUntrustedResourceSink() { exists(VarAccess webviewVa, MethodAccess getSettingsMa, Variable v | - sink.(UrlResourceSink).getMethodAccess().getQualifier() = webviewVa and + this.getMethodAccess().getQualifier() = webviewVa and getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and webviewVa.getVariable().getAnAccess() = getSettingsMa.getQualifier() and v.getAnAssignedValue() = getSettingsMa and @@ -122,9 +102,3 @@ class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration { ) } } - -from DataFlow::PathNode source, DataFlow::PathNode sink, FetchUntrustedResourceConfiguration conf -where conf.hasFlowPath(source, sink) -select sink.getNode().(UrlResourceSink).getMethodAccess(), source, sink, - "Unsafe resource fetching in Android webview due to $@.", source.getNode(), - sink.getNode().(UrlResourceSink).getSinkType() From 9b78cee37a212360f236d28923a18a4aac28697a Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 5 May 2021 11:59:57 +0200 Subject: [PATCH 03/25] Add tests --- .../security/CWE-749/UnsafeAndroidAccess.java | 84 +++++++++++++++++++ .../CWE-749/UnsafeAndroidAccessTest.expected | 0 .../CWE-749/UnsafeAndroidAccessTest.ql | 28 +++++++ .../test/query-tests/security/CWE-749/options | 1 + 4 files changed, 113 insertions(+) create mode 100644 java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java create mode 100644 java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.expected create mode 100644 java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql create mode 100644 java/ql/test/query-tests/security/CWE-749/options diff --git a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java new file mode 100644 index 00000000000..bca4c6a0753 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java @@ -0,0 +1,84 @@ +import android.app.Activity; +import android.os.Bundle; +import android.webkit.WebSettings; +import android.webkit.WebView; +import android.webkit.WebViewClient; +import com.android.internal.R; + +public class UnsafeAndroidAccess extends Activity { + public void onCreate(Bundle savedInstanceState) { + super.onCreate(savedInstanceState); + setContentView(R.layout.webview); + { + WebView wv = (WebView) findViewById(R.id.my_webview); + WebSettings webSettings = wv.getSettings(); + + webSettings.setJavaScriptEnabled(true); + webSettings.setAllowUniversalAccessFromFileURLs(true); + + wv.setWebViewClient(new WebViewClient() { + @Override + public boolean shouldOverrideUrlLoading(WebView view, String url) { + view.loadUrl(url); + return true; + } + }); + + String thisUrl = getIntent().getExtras().getString("url"); + wv.loadUrl(thisUrl); // hasUnsafeAndroidAccess + } + + { + WebView wv = (WebView) findViewById(R.id.my_webview); + WebSettings webSettings = wv.getSettings(); + + webSettings.setJavaScriptEnabled(true); + webSettings.setAllowUniversalAccessFromFileURLs(true); + + wv.setWebViewClient(new WebViewClient() { + @Override + public boolean shouldOverrideUrlLoading(WebView view, String url) { + view.loadUrl(url); + return true; + } + }); + + String thisUrl = getIntent().getStringExtra("url"); + wv.loadUrl(thisUrl); // hasUnsafeAndroidAccess + } + + { + WebView wv = (WebView) findViewById(-1); + WebSettings webSettings = wv.getSettings(); + + wv.setWebViewClient(new WebViewClient() { + @Override + public boolean shouldOverrideUrlLoading(WebView view, String url) { + view.loadUrl(url); + return true; + } + }); + + String thisUrl = getIntent().getExtras().getString("url"); // remote input + wv.loadUrl(thisUrl); // Safe + } + + { + WebView wv = (WebView) findViewById(-1); + WebSettings webSettings = wv.getSettings(); + + webSettings.setJavaScriptEnabled(true); + + wv.setWebViewClient(new WebViewClient() { + @Override + public boolean shouldOverrideUrlLoading(WebView view, String url) { + view.loadUrl(url); + return true; + } + }); + + wv.loadUrl("https://www.mycorp.com"); // Safe + } + } + +} \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.expected b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql new file mode 100644 index 00000000000..3bd5e8aae88 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql @@ -0,0 +1,28 @@ +import java +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.FlowSources +import TestUtilities.InlineExpectationsTest +import semmle.code.java.security.UnsafeAndroidAccess + +class Conf extends TaintTracking::Configuration { + Conf() { this = "qltest:cwe:jexl-injection" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof FetchUntrustedResourceSink } +} + +class UnsafeAndroidAccessTest extends InlineExpectationsTest { + UnsafeAndroidAccessTest() { this = "HasUnsafeAndroidAccess" } + + override string getARelevantTag() { result = "hasUnsafeAndroidAccess" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasUnsafeAndroidAccess" and + exists(DataFlow::Node src, DataFlow::Node sink, Conf conf | conf.hasFlow(src, sink) | + sink.getLocation() = location and + element = sink.toString() and + value = "" + ) + } +} diff --git a/java/ql/test/query-tests/security/CWE-749/options b/java/ql/test/query-tests/security/CWE-749/options new file mode 100644 index 00000000000..d6a9adcece3 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-749/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/android From 03ce8d689f6927bf71cb63582d27d8858154551b Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 5 May 2021 16:34:30 +0200 Subject: [PATCH 04/25] Refactored to use CSV sink model --- .../CWE/CWE-749/UnsafeAndroidAccess.ql | 7 +- .../java/security/UnsafeAndroidAccess.qll | 133 +++++++++--------- .../CWE-749/UnsafeAndroidAccessTest.ql | 2 +- 3 files changed, 71 insertions(+), 71 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql index 831c2d0b01d..1ac3f7f9731 100644 --- a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql +++ b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql @@ -24,11 +24,10 @@ class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSink(DataFlow::Node sink) { sink instanceof FetchUntrustedResourceSink } + override predicate isSink(DataFlow::Node sink) { sink instanceof UrlResourceSink } } from DataFlow::PathNode source, DataFlow::PathNode sink, FetchUntrustedResourceConfiguration conf where conf.hasFlowPath(source, sink) -select sink.getNode().(FetchUntrustedResourceSink).getMethodAccess(), source, sink, - "Unsafe resource fetching in Android webview due to $@.", source.getNode(), - sink.getNode().(FetchUntrustedResourceSink).getSinkType() +select sink.getNode(), source, sink, "Unsafe resource fetching in Android webview due to $@.", + source.getNode(), sink.getNode().(UrlResourceSink).getSinkType() diff --git a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll index e741992de35..952b827e51d 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll @@ -1,8 +1,75 @@ +/** + */ + import java import semmle.code.java.frameworks.android.WebView import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.ExternalFlow +/** + */ +abstract class UrlResourceSink extends DataFlow::Node { + /** + * Returns a description of this vulnerability, + */ + abstract string getSinkType(); +} + +/** + * A URL argument to a `loadUrl` or `postUrl` call, considered as a sink. + */ +private class DefaultUrlResourceSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "android.webkit;WebView;true;loadUrl;;;Argument[0];unsafe-android-access", + "android.webkit;WebView;true;postUrl;;;Argument[0];unsafe-android-access" + ] + } +} + +/** + * Cross-origin access enabled resource fetch. + * + * Specifically this looks for code like + * `webView.getSettings().setAllow[File|Universal]AccessFromFileURLs(true);` + */ +private class CrossOriginUrlResourceSink extends UrlResourceSink { + CrossOriginUrlResourceSink() { + sinkNode(this, "unsafe-android-access") and + exists(MethodAccess ma, MethodAccess getSettingsMa | + ma.getMethod() instanceof CrossOriginAccessMethod and + ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true and + ma.getQualifier().(VarAccess).getVariable().getAnAssignedValue() = getSettingsMa and + getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and + getSettingsMa.getQualifier().(VarAccess).getVariable().getAnAccess() = + this.asExpr().(Argument).getCall().getQualifier() + ) + } + + override string getSinkType() { + result = "user input vulnerable to cross-origin and sensitive resource disclosure attacks" + } +} + +/** + * JavaScript enabled resource fetch. + */ +private class JavaScriptEnabledUrlResourceSink extends UrlResourceSink { + JavaScriptEnabledUrlResourceSink() { + sinkNode(this, "unsafe-android-access") and + exists(VarAccess webviewVa, MethodAccess getSettingsMa, Variable v | + this.asExpr().(Argument).getCall().getQualifier() = webviewVa and + getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and + webviewVa.getVariable().getAnAccess() = getSettingsMa.getQualifier() and + v.getAnAssignedValue() = getSettingsMa and + isJSEnabled(v) + ) + } + + override string getSinkType() { result = "user input vulnerable to XSS attacks" } +} + /** * Methods allowing any-local-file and cross-origin access in the WebSettings class */ @@ -36,69 +103,3 @@ private predicate isJSEnabled(Variable v) { jsa.getArgument(0).(BooleanLiteral).getBooleanValue() = true ) } - -/** - * Fetch URL method call on the `android.webkit.WebView` object - */ -private class FetchResourceMethodAccess extends MethodAccess { - FetchResourceMethodAccess() { - this.getMethod().getDeclaringType() instanceof TypeWebView and - this.getMethod().hasName(["loadUrl", "postUrl"]) - } -} - -/** - * Holds if `ma` loads URL `sink` - */ -private predicate fetchResource(FetchResourceMethodAccess ma, Expr sink) { - sink = ma.getArgument(0) -} - -/** - * A URL argument to a `loadUrl` or `postUrl` call, considered as a sink. - */ -private class UrlResourceSink extends DataFlow::ExprNode { - UrlResourceSink() { fetchResource(_, this.getExpr()) } - - /** Gets the fetch method that fetches this sink URL. */ - FetchResourceMethodAccess getMethodAccess() { fetchResource(result, this.getExpr()) } - - /** - * Holds if cross-origin access is enabled for this resource fetch. - * - * Specifically this looks for code like - * `webView.getSettings().setAllow[File|Universal]AccessFromFileURLs(true);` - */ - predicate crossOriginAccessEnabled() { - exists(MethodAccess ma, MethodAccess getSettingsMa | - ma.getMethod() instanceof CrossOriginAccessMethod and - ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true and - ma.getQualifier().(VarAccess).getVariable().getAnAssignedValue() = getSettingsMa and - getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and - getSettingsMa.getQualifier().(VarAccess).getVariable().getAnAccess() = - getMethodAccess().getQualifier() - ) - } - - /** - * Returns a description of this vulnerability, assuming Javascript is enabled and - * the fetched URL is attacker-controlled. - */ - string getSinkType() { - if crossOriginAccessEnabled() - then result = "user input vulnerable to cross-origin and sensitive resource disclosure attacks" - else result = "user input vulnerable to XSS attacks" - } -} - -class FetchUntrustedResourceSink extends UrlResourceSink { - FetchUntrustedResourceSink() { - exists(VarAccess webviewVa, MethodAccess getSettingsMa, Variable v | - this.getMethodAccess().getQualifier() = webviewVa and - getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and - webviewVa.getVariable().getAnAccess() = getSettingsMa.getQualifier() and - v.getAnAssignedValue() = getSettingsMa and - isJSEnabled(v) - ) - } -} diff --git a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql index 3bd5e8aae88..7c35d1dd7ad 100644 --- a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql @@ -9,7 +9,7 @@ class Conf extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - override predicate isSink(DataFlow::Node sink) { sink instanceof FetchUntrustedResourceSink } + override predicate isSink(DataFlow::Node sink) { sink instanceof UrlResourceSink } } class UnsafeAndroidAccessTest extends InlineExpectationsTest { From c138ed3e4da9c57a3430ef186f2b75870506d57f Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 5 May 2021 16:51:15 +0200 Subject: [PATCH 05/25] QLDocs --- java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql | 2 +- .../src/semmle/code/java/security/UnsafeAndroidAccess.qll | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql index 1ac3f7f9731..6bc332d085c 100644 --- a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql +++ b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql @@ -17,7 +17,7 @@ import semmle.code.java.security.UnsafeAndroidAccess import DataFlow::PathGraph /** - * Taint configuration tracking flow from untrusted inputs to `loadUrl` or `postUrl` calls. + * Taint configuration tracking flow from untrusted inputs to a resource fetching call. */ class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration { FetchUntrustedResourceConfiguration() { this = "FetchUntrustedResourceConfiguration" } diff --git a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll index 952b827e51d..11c7b2e71e7 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll @@ -1,4 +1,5 @@ /** + * Provides classes to reason about Unsafe Resource Fetching vulnerabilities in Android. */ import java @@ -7,6 +8,9 @@ import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.ExternalFlow /** + * A sink that represents a method that fetches a web resource. + * + * Extend this class to add your own Unsafe Resource Fetching sinks. */ abstract class UrlResourceSink extends DataFlow::Node { /** @@ -15,9 +19,7 @@ abstract class UrlResourceSink extends DataFlow::Node { abstract string getSinkType(); } -/** - * A URL argument to a `loadUrl` or `postUrl` call, considered as a sink. - */ +/** CSV sink models representing methods susceptible to Unsafe Resource Fetching attacks. */ private class DefaultUrlResourceSinkModel extends SinkModelCsv { override predicate row(string row) { row = From a706046a1978b5193329b419c85bbad0dcd94be7 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 6 May 2021 09:17:53 +0200 Subject: [PATCH 06/25] Reestructured test --- .../security/CWE-749/UnsafeAndroidAccess.java | 128 ++++++++---------- 1 file changed, 58 insertions(+), 70 deletions(-) diff --git a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java index bca4c6a0753..e15e2ab9482 100644 --- a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java @@ -9,76 +9,64 @@ public class UnsafeAndroidAccess extends Activity { public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); setContentView(R.layout.webview); - { - WebView wv = (WebView) findViewById(R.id.my_webview); - WebSettings webSettings = wv.getSettings(); - - webSettings.setJavaScriptEnabled(true); - webSettings.setAllowUniversalAccessFromFileURLs(true); - - wv.setWebViewClient(new WebViewClient() { - @Override - public boolean shouldOverrideUrlLoading(WebView view, String url) { - view.loadUrl(url); - return true; - } - }); - - String thisUrl = getIntent().getExtras().getString("url"); - wv.loadUrl(thisUrl); // hasUnsafeAndroidAccess - } - - { - WebView wv = (WebView) findViewById(R.id.my_webview); - WebSettings webSettings = wv.getSettings(); - - webSettings.setJavaScriptEnabled(true); - webSettings.setAllowUniversalAccessFromFileURLs(true); - - wv.setWebViewClient(new WebViewClient() { - @Override - public boolean shouldOverrideUrlLoading(WebView view, String url) { - view.loadUrl(url); - return true; - } - }); - - String thisUrl = getIntent().getStringExtra("url"); - wv.loadUrl(thisUrl); // hasUnsafeAndroidAccess - } - - { - WebView wv = (WebView) findViewById(-1); - WebSettings webSettings = wv.getSettings(); - - wv.setWebViewClient(new WebViewClient() { - @Override - public boolean shouldOverrideUrlLoading(WebView view, String url) { - view.loadUrl(url); - return true; - } - }); - - String thisUrl = getIntent().getExtras().getString("url"); // remote input - wv.loadUrl(thisUrl); // Safe - } - - { - WebView wv = (WebView) findViewById(-1); - WebSettings webSettings = wv.getSettings(); - - webSettings.setJavaScriptEnabled(true); - - wv.setWebViewClient(new WebViewClient() { - @Override - public boolean shouldOverrideUrlLoading(WebView view, String url) { - view.loadUrl(url); - return true; - } - }); - - wv.loadUrl("https://www.mycorp.com"); // Safe - } + testJavaScriptEnabledWebView(); + testCrossOriginEnabledWebView(); + testSafeWebView(); } + private void testJavaScriptEnabledWebView() { + WebView wv = (WebView) findViewById(R.id.my_webview); + WebSettings webSettings = wv.getSettings(); + + webSettings.setJavaScriptEnabled(true); + + wv.setWebViewClient(new WebViewClient() { + @Override + public boolean shouldOverrideUrlLoading(WebView view, String url) { + view.loadUrl(url); + return true; + } + }); + + String thisUrl = getIntent().getExtras().getString("url"); + wv.loadUrl(thisUrl); // hasUnsafeAndroidAccess + wv.loadUrl("https://www.mycorp.com/" + thisUrl); // Safe + wv.loadUrl("https://www.mycorp.com"); // Safe + } + + private void testCrossOriginEnabledWebView() { + WebView wv = (WebView) findViewById(R.id.my_webview); + WebSettings webSettings = wv.getSettings(); + webSettings.setAllowUniversalAccessFromFileURLs(true); + + wv.setWebViewClient(new WebViewClient() { + @Override + public boolean shouldOverrideUrlLoading(WebView view, String url) { + view.loadUrl(url); + return true; + } + }); + + String thisUrl = getIntent().getStringExtra("url"); + wv.loadUrl(thisUrl); // hasUnsafeAndroidAccess + wv.loadUrl("https://www.mycorp.com/" + thisUrl); // hasUnsafeAndroidAccess + wv.loadUrl("https://www.mycorp.com"); // Safe + } + + private void testSafeWebView() { + WebView wv = (WebView) findViewById(-1); + + wv.setWebViewClient(new WebViewClient() { + @Override + public boolean shouldOverrideUrlLoading(WebView view, String url) { + view.loadUrl(url); + return true; + } + }); + + String thisUrl = getIntent().getExtras().getString("url"); + wv.loadUrl(thisUrl); // Safe + wv.loadUrl("https://www.mycorp.com/" + thisUrl); // Safe + wv.loadUrl("https://www.mycorp.com"); // Safe + } } \ No newline at end of file From 84504a88e406a5ec0a351cae12156b62ede7d291 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 6 May 2021 10:55:56 +0200 Subject: [PATCH 07/25] Fix tests by adding AndroidManifest.xml --- .../query-tests/security/CWE-749/AndroidManifest.xml | 11 +++++++++++ .../CWE-749/{ => app}/UnsafeAndroidAccess.java | 8 +++++--- 2 files changed, 16 insertions(+), 3 deletions(-) create mode 100644 java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml rename java/ql/test/query-tests/security/CWE-749/{ => app}/UnsafeAndroidAccess.java (91%) diff --git a/java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml new file mode 100644 index 00000000000..4f05c56c027 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml @@ -0,0 +1,11 @@ + + + + + + + + + + + \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java b/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java similarity index 91% rename from java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java rename to java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java index e15e2ab9482..38c1a6b3510 100644 --- a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java +++ b/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java @@ -1,3 +1,5 @@ +package app; + import android.app.Activity; import android.os.Bundle; import android.webkit.WebSettings; @@ -29,7 +31,7 @@ public class UnsafeAndroidAccess extends Activity { }); String thisUrl = getIntent().getExtras().getString("url"); - wv.loadUrl(thisUrl); // hasUnsafeAndroidAccess + wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess wv.loadUrl("https://www.mycorp.com/" + thisUrl); // Safe wv.loadUrl("https://www.mycorp.com"); // Safe } @@ -48,8 +50,8 @@ public class UnsafeAndroidAccess extends Activity { }); String thisUrl = getIntent().getStringExtra("url"); - wv.loadUrl(thisUrl); // hasUnsafeAndroidAccess - wv.loadUrl("https://www.mycorp.com/" + thisUrl); // hasUnsafeAndroidAccess + wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess + wv.loadUrl("https://www.mycorp.com/" + thisUrl); // Safe wv.loadUrl("https://www.mycorp.com"); // Safe } From e14294a2f72438816a503e11dd248103ec7579df Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 6 May 2021 11:20:37 +0200 Subject: [PATCH 08/25] Remove XSS sink since it's better handled in this query --- java/ql/src/semmle/code/java/security/XSS.qll | 1 - 1 file changed, 1 deletion(-) diff --git a/java/ql/src/semmle/code/java/security/XSS.qll b/java/ql/src/semmle/code/java/security/XSS.qll index 486e8053953..578e2fe86bc 100644 --- a/java/ql/src/semmle/code/java/security/XSS.qll +++ b/java/ql/src/semmle/code/java/security/XSS.qll @@ -36,7 +36,6 @@ private class DefaultXssSinkModel extends SinkModelCsv { [ "javax.servlet.http;HttpServletResponse;false;sendError;(int,String);;Argument[1];xss", "android.webkit;WebView;false;loadData;;;Argument[0];xss", - "android.webkit;WebView;false;loadUrl;;;Argument[0];xss", "android.webkit;WebView;false;loadDataWithBaseURL;;;Argument[1];xss" ] } From 1f1f85aeb5d3222c0606993fe0fcbe3a9a152ba6 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 6 May 2021 13:13:23 +0200 Subject: [PATCH 09/25] Add change note and fix some QLDocs --- java/change-notes/2021-05-06-unsafe-android-access-query.md | 2 ++ java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll | 2 +- .../query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 java/change-notes/2021-05-06-unsafe-android-access-query.md diff --git a/java/change-notes/2021-05-06-unsafe-android-access-query.md b/java/change-notes/2021-05-06-unsafe-android-access-query.md new file mode 100644 index 00000000000..428e6f0e282 --- /dev/null +++ b/java/change-notes/2021-05-06-unsafe-android-access-query.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The query "Unsafe resource fetching in Android webview" (`java/android/unsafe-android-webview-fetch`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3706) \ No newline at end of file diff --git a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll index 11c7b2e71e7..cd4e88790a5 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll @@ -8,7 +8,7 @@ import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.ExternalFlow /** - * A sink that represents a method that fetches a web resource. + * A sink that represents a method that fetches a web resource in Android. * * Extend this class to add your own Unsafe Resource Fetching sinks. */ diff --git a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql index 7c35d1dd7ad..6563286f5c7 100644 --- a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql @@ -5,7 +5,7 @@ import TestUtilities.InlineExpectationsTest import semmle.code.java.security.UnsafeAndroidAccess class Conf extends TaintTracking::Configuration { - Conf() { this = "qltest:cwe:jexl-injection" } + Conf() { this = "qltest:cwe:unsafe-android-access" } override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } From b69261727db034a68a7f4b65bf24f3933c52775d Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 6 May 2021 13:26:25 +0200 Subject: [PATCH 10/25] Add a new test for --- .../java/security/UnsafeAndroidAccess.qll | 5 +--- .../CWE-749/app/UnsafeAndroidAccess.java | 25 ++++++++++++++++--- .../android/android/webkit/WebSettings.java | 3 +++ 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll index cd4e88790a5..9c7b459e421 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll @@ -78,10 +78,7 @@ private class JavaScriptEnabledUrlResourceSink extends UrlResourceSink { private class CrossOriginAccessMethod extends Method { CrossOriginAccessMethod() { this.getDeclaringType() instanceof TypeWebSettings and - ( - this.hasName("setAllowUniversalAccessFromFileURLs") or - this.hasName("setAllowFileAccessFromFileURLs") - ) + this.hasName(["setAllowUniversalAccessFromFileURLs", "setAllowFileAccessFromFileURLs"]) } } diff --git a/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java b/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java index 38c1a6b3510..14577b43d0c 100644 --- a/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java +++ b/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java @@ -12,14 +12,14 @@ public class UnsafeAndroidAccess extends Activity { super.onCreate(savedInstanceState); setContentView(R.layout.webview); testJavaScriptEnabledWebView(); - testCrossOriginEnabledWebView(); + testUniversalFileAccessEnabledWebView(); + testFileAccessEnabledWebView(); testSafeWebView(); } private void testJavaScriptEnabledWebView() { WebView wv = (WebView) findViewById(R.id.my_webview); WebSettings webSettings = wv.getSettings(); - webSettings.setJavaScriptEnabled(true); wv.setWebViewClient(new WebViewClient() { @@ -36,7 +36,7 @@ public class UnsafeAndroidAccess extends Activity { wv.loadUrl("https://www.mycorp.com"); // Safe } - private void testCrossOriginEnabledWebView() { + private void testUniversalFileAccessEnabledWebView() { WebView wv = (WebView) findViewById(R.id.my_webview); WebSettings webSettings = wv.getSettings(); webSettings.setAllowUniversalAccessFromFileURLs(true); @@ -55,6 +55,25 @@ public class UnsafeAndroidAccess extends Activity { wv.loadUrl("https://www.mycorp.com"); // Safe } + private void testFileAccessEnabledWebView() { + WebView wv = (WebView) findViewById(R.id.my_webview); + WebSettings webSettings = wv.getSettings(); + webSettings.setAllowFileAccessFromFileURLs(true); + + wv.setWebViewClient(new WebViewClient() { + @Override + public boolean shouldOverrideUrlLoading(WebView view, String url) { + view.loadUrl(url); + return true; + } + }); + + String thisUrl = getIntent().getStringExtra("url"); + wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess + wv.loadUrl("https://www.mycorp.com/" + thisUrl); // Safe + wv.loadUrl("https://www.mycorp.com"); // Safe + } + private void testSafeWebView() { WebView wv = (WebView) findViewById(-1); diff --git a/java/ql/test/stubs/android/android/webkit/WebSettings.java b/java/ql/test/stubs/android/android/webkit/WebSettings.java index 495c2b0a390..ff1daf01404 100644 --- a/java/ql/test/stubs/android/android/webkit/WebSettings.java +++ b/java/ql/test/stubs/android/android/webkit/WebSettings.java @@ -28,4 +28,7 @@ public abstract class WebSettings { public void setAllowUniversalAccessFromFileURLs(boolean b) { } + + public void setAllowFileAccessFromFileURLs(boolean b) { + } } \ No newline at end of file From e2e65aca3c893a8a40ebb81d02342b40e0adc10b Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 7 May 2021 12:25:19 +0200 Subject: [PATCH 11/25] Add new sink for Android XSS --- java/ql/src/semmle/code/java/security/XSS.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/java/ql/src/semmle/code/java/security/XSS.qll b/java/ql/src/semmle/code/java/security/XSS.qll index 578e2fe86bc..36ad21cf120 100644 --- a/java/ql/src/semmle/code/java/security/XSS.qll +++ b/java/ql/src/semmle/code/java/security/XSS.qll @@ -36,7 +36,8 @@ private class DefaultXssSinkModel extends SinkModelCsv { [ "javax.servlet.http;HttpServletResponse;false;sendError;(int,String);;Argument[1];xss", "android.webkit;WebView;false;loadData;;;Argument[0];xss", - "android.webkit;WebView;false;loadDataWithBaseURL;;;Argument[1];xss" + "android.webkit;WebView;false;loadDataWithBaseURL;;;Argument[1];xss", + "android.webkit;WebView;false;evaluateJavascript;;;Argument[0];xss" ] } } From e6b7da192690d1e5c2e2d75905cd4ac60272f55c Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 7 May 2021 12:41:39 +0200 Subject: [PATCH 12/25] Add import for Android sinks in ExternalFlow --- java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 7073c57ff9c..a4cc02414aa 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -80,6 +80,7 @@ private module Frameworks { private import semmle.code.java.security.ResponseSplitting private import semmle.code.java.security.XSS private import semmle.code.java.security.LdapInjection + private import semmle.code.java.security.UnsafeAndroidAccess } private predicate sourceModelCsv(string row) { From dcee1daa312877ed0d419dc22bf963d7a151b32d Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 7 May 2021 13:17:04 +0200 Subject: [PATCH 13/25] Mark spurious test results --- .../security/CWE-749/app/UnsafeAndroidAccess.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java b/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java index 14577b43d0c..811d5f42f1b 100644 --- a/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java +++ b/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java @@ -32,7 +32,7 @@ public class UnsafeAndroidAccess extends Activity { String thisUrl = getIntent().getExtras().getString("url"); wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess - wv.loadUrl("https://www.mycorp.com/" + thisUrl); // Safe + wv.loadUrl("https://www.mycorp.com/" + thisUrl); // $ SPURIOUS: hasUnsafeAndroidAccess // Safe, needs sanitizer wv.loadUrl("https://www.mycorp.com"); // Safe } @@ -51,7 +51,7 @@ public class UnsafeAndroidAccess extends Activity { String thisUrl = getIntent().getStringExtra("url"); wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess - wv.loadUrl("https://www.mycorp.com/" + thisUrl); // Safe + wv.loadUrl("https://www.mycorp.com/" + thisUrl); // $ SPURIOUS: hasUnsafeAndroidAccess // Safe, needs sanitizer wv.loadUrl("https://www.mycorp.com"); // Safe } @@ -70,7 +70,7 @@ public class UnsafeAndroidAccess extends Activity { String thisUrl = getIntent().getStringExtra("url"); wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess - wv.loadUrl("https://www.mycorp.com/" + thisUrl); // Safe + wv.loadUrl("https://www.mycorp.com/" + thisUrl); // $ SPURIOUS: hasUnsafeAndroidAccess // Safe, needs sanitizer wv.loadUrl("https://www.mycorp.com"); // Safe } From 1f1a1bdb41f5b648e57d224a1c2334e5f2851873 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 7 May 2021 16:29:00 +0200 Subject: [PATCH 14/25] Remove unnecessary CWE reference --- java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp | 1 - 1 file changed, 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp index 3ac8ec67e59..165b9310464 100644 --- a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp +++ b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp @@ -23,7 +23,6 @@
  • - CWE-749 Fixing a File-based XSS Vulnerability OWASP - Testing WebView Protocol Handlers (MSTG-PLATFORM-5 and MSTG-PLATFORM-6)
    • From c70503142f4d184701f6d6d8b5cd42b522c28744 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 10 May 2021 09:45:59 +0200 Subject: [PATCH 15/25] Require JS enabled even when cross-origin access is enabled in the webviews --- .../java/security/UnsafeAndroidAccess.qll | 8 +++--- .../CWE-749/app/UnsafeAndroidAccess.java | 26 ++++++++++++++++++- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll index 9c7b459e421..e07094fe599 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll @@ -32,13 +32,11 @@ private class DefaultUrlResourceSinkModel extends SinkModelCsv { /** * Cross-origin access enabled resource fetch. - * - * Specifically this looks for code like - * `webView.getSettings().setAllow[File|Universal]AccessFromFileURLs(true);` + * + * It requires JavaScript to be enabled too to be considered a valid sink. */ -private class CrossOriginUrlResourceSink extends UrlResourceSink { +private class CrossOriginUrlResourceSink extends JavaScriptEnabledUrlResourceSink { CrossOriginUrlResourceSink() { - sinkNode(this, "unsafe-android-access") and exists(MethodAccess ma, MethodAccess getSettingsMa | ma.getMethod() instanceof CrossOriginAccessMethod and ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true and diff --git a/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java b/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java index 811d5f42f1b..60f0b1d2821 100644 --- a/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java +++ b/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java @@ -15,6 +15,7 @@ public class UnsafeAndroidAccess extends Activity { testUniversalFileAccessEnabledWebView(); testFileAccessEnabledWebView(); testSafeWebView(); + testCrossOriginEnabledJsDisabledWebView(); } private void testJavaScriptEnabledWebView() { @@ -39,6 +40,7 @@ public class UnsafeAndroidAccess extends Activity { private void testUniversalFileAccessEnabledWebView() { WebView wv = (WebView) findViewById(R.id.my_webview); WebSettings webSettings = wv.getSettings(); + webSettings.setJavaScriptEnabled(true); webSettings.setAllowUniversalAccessFromFileURLs(true); wv.setWebViewClient(new WebViewClient() { @@ -58,6 +60,7 @@ public class UnsafeAndroidAccess extends Activity { private void testFileAccessEnabledWebView() { WebView wv = (WebView) findViewById(R.id.my_webview); WebSettings webSettings = wv.getSettings(); + webSettings.setJavaScriptEnabled(true); webSettings.setAllowFileAccessFromFileURLs(true); wv.setWebViewClient(new WebViewClient() { @@ -90,4 +93,25 @@ public class UnsafeAndroidAccess extends Activity { wv.loadUrl("https://www.mycorp.com/" + thisUrl); // Safe wv.loadUrl("https://www.mycorp.com"); // Safe } -} \ No newline at end of file + + private void testCrossOriginEnabledJsDisabledWebView() { + WebView wv = (WebView) findViewById(-1); + WebSettings webSettings = wv.getSettings(); + webSettings.setAllowUniversalAccessFromFileURLs(true); + webSettings.setAllowFileAccessFromFileURLs(true); + + wv.setWebViewClient(new WebViewClient() { + @Override + public boolean shouldOverrideUrlLoading(WebView view, String url) { + view.loadUrl(url); + return true; + } + + }); + + String thisUrl = getIntent().getExtras().getString("url"); + wv.loadUrl(thisUrl); // Safe + wv.loadUrl("https://www.mycorp.com/" + thisUrl); // Safe + wv.loadUrl("https://www.mycorp.com"); // Safe + } +} From d99b5bfc66e335fa18587100a36c5c2b2598b550 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 10 May 2021 11:17:20 +0200 Subject: [PATCH 16/25] Reuse previous tests from experimental --- .../security/CWE-749/AndroidManifest.xml | 51 ---- .../CWE-749/UnsafeAndroidAccess.expected | 31 --- .../CWE-749/UnsafeAndroidAccess.qlref | 1 - .../query-tests/security/CWE-749/options | 1 - .../security/CWE-749/AndroidManifest.xml | 50 +++- .../security/CWE-749/IntentUtils.java | 0 .../security/CWE-749/SafeActivity1.java | 8 +- .../security/CWE-749/SafeActivity2.java | 8 +- .../security/CWE-749/SafeActivity3.java | 8 +- .../security/CWE-749/UnsafeActivity1.java | 8 +- .../security/CWE-749/UnsafeActivity2.java | 8 +- .../security/CWE-749/UnsafeActivity3.java | 8 +- .../security/CWE-749/UnsafeActivity4.java | 16 +- .../security/CWE-749/UnsafeAndroidAccess.java | 53 +++- .../UnsafeAndroidBroadcastReceiver.java | 7 +- .../CWE-749/app/UnsafeAndroidAccess.java | 117 -------- .../android/content/BroadcastReceiver.java | 255 ++++++++++++++++++ 17 files changed, 387 insertions(+), 243 deletions(-) delete mode 100755 java/ql/test/experimental/query-tests/security/CWE-749/AndroidManifest.xml delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.expected delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.qlref delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-749/options mode change 100644 => 100755 java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml rename java/ql/test/{experimental => }/query-tests/security/CWE-749/IntentUtils.java (100%) rename java/ql/test/{experimental => }/query-tests/security/CWE-749/SafeActivity1.java (75%) rename java/ql/test/{experimental => }/query-tests/security/CWE-749/SafeActivity2.java (78%) rename java/ql/test/{experimental => }/query-tests/security/CWE-749/SafeActivity3.java (78%) rename java/ql/test/{experimental => }/query-tests/security/CWE-749/UnsafeActivity1.java (76%) rename java/ql/test/{experimental => }/query-tests/security/CWE-749/UnsafeActivity2.java (74%) rename java/ql/test/{experimental => }/query-tests/security/CWE-749/UnsafeActivity3.java (77%) rename java/ql/test/{experimental => }/query-tests/security/CWE-749/UnsafeActivity4.java (69%) rename java/ql/test/{experimental => }/query-tests/security/CWE-749/UnsafeAndroidAccess.java (60%) rename java/ql/test/{experimental => }/query-tests/security/CWE-749/UnsafeAndroidBroadcastReceiver.java (82%) delete mode 100644 java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java create mode 100644 java/ql/test/stubs/android/android/content/BroadcastReceiver.java diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/AndroidManifest.xml b/java/ql/test/experimental/query-tests/security/CWE-749/AndroidManifest.xml deleted file mode 100755 index b215e4d3466..00000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-749/AndroidManifest.xml +++ /dev/null @@ -1,51 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.expected b/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.expected deleted file mode 100644 index 9e2b153dd64..00000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.expected +++ /dev/null @@ -1,31 +0,0 @@ -edges -| UnsafeActivity1.java:31:20:31:30 | getIntent(...) : Intent | UnsafeActivity1.java:32:14:32:20 | thisUrl | -| UnsafeActivity2.java:31:20:31:30 | getIntent(...) : Intent | UnsafeActivity2.java:32:14:32:20 | thisUrl | -| UnsafeActivity3.java:31:20:31:30 | getIntent(...) : Intent | UnsafeActivity3.java:32:14:32:20 | thisUrl | -| UnsafeAndroidAccess.java:31:20:31:30 | getIntent(...) : Intent | UnsafeAndroidAccess.java:32:14:32:20 | thisUrl | -| UnsafeAndroidAccess.java:54:20:54:30 | getIntent(...) : Intent | UnsafeAndroidAccess.java:55:14:55:20 | thisUrl | -| UnsafeAndroidAccess.java:96:20:96:30 | getIntent(...) : Intent | UnsafeAndroidAccess.java:97:14:97:20 | thisUrl | -| UnsafeAndroidBroadcastReceiver.java:16:41:16:53 | intent : Intent | UnsafeAndroidBroadcastReceiver.java:32:14:32:20 | thisUrl | -nodes -| UnsafeActivity1.java:31:20:31:30 | getIntent(...) : Intent | semmle.label | getIntent(...) : Intent | -| UnsafeActivity1.java:32:14:32:20 | thisUrl | semmle.label | thisUrl | -| UnsafeActivity2.java:31:20:31:30 | getIntent(...) : Intent | semmle.label | getIntent(...) : Intent | -| UnsafeActivity2.java:32:14:32:20 | thisUrl | semmle.label | thisUrl | -| UnsafeActivity3.java:31:20:31:30 | getIntent(...) : Intent | semmle.label | getIntent(...) : Intent | -| UnsafeActivity3.java:32:14:32:20 | thisUrl | semmle.label | thisUrl | -| UnsafeAndroidAccess.java:31:20:31:30 | getIntent(...) : Intent | semmle.label | getIntent(...) : Intent | -| UnsafeAndroidAccess.java:32:14:32:20 | thisUrl | semmle.label | thisUrl | -| UnsafeAndroidAccess.java:54:20:54:30 | getIntent(...) : Intent | semmle.label | getIntent(...) : Intent | -| UnsafeAndroidAccess.java:55:14:55:20 | thisUrl | semmle.label | thisUrl | -| UnsafeAndroidAccess.java:96:20:96:30 | getIntent(...) : Intent | semmle.label | getIntent(...) : Intent | -| UnsafeAndroidAccess.java:97:14:97:20 | thisUrl | semmle.label | thisUrl | -| UnsafeAndroidBroadcastReceiver.java:16:41:16:53 | intent : Intent | semmle.label | intent : Intent | -| UnsafeAndroidBroadcastReceiver.java:32:14:32:20 | thisUrl | semmle.label | thisUrl | -#select -| UnsafeActivity1.java:32:3:32:21 | loadUrl(...) | UnsafeActivity1.java:31:20:31:30 | getIntent(...) : Intent | UnsafeActivity1.java:32:14:32:20 | thisUrl | Unsafe resource fetching in Android webview due to $@. | UnsafeActivity1.java:31:20:31:30 | getIntent(...) | user input vulnerable to cross-origin and sensitive resource disclosure attacks | -| UnsafeActivity2.java:32:3:32:21 | loadUrl(...) | UnsafeActivity2.java:31:20:31:30 | getIntent(...) : Intent | UnsafeActivity2.java:32:14:32:20 | thisUrl | Unsafe resource fetching in Android webview due to $@. | UnsafeActivity2.java:31:20:31:30 | getIntent(...) | user input vulnerable to cross-origin and sensitive resource disclosure attacks | -| UnsafeActivity3.java:32:3:32:21 | loadUrl(...) | UnsafeActivity3.java:31:20:31:30 | getIntent(...) : Intent | UnsafeActivity3.java:32:14:32:20 | thisUrl | Unsafe resource fetching in Android webview due to $@. | UnsafeActivity3.java:31:20:31:30 | getIntent(...) | user input vulnerable to cross-origin and sensitive resource disclosure attacks | -| UnsafeAndroidAccess.java:32:3:32:21 | loadUrl(...) | UnsafeAndroidAccess.java:31:20:31:30 | getIntent(...) : Intent | UnsafeAndroidAccess.java:32:14:32:20 | thisUrl | Unsafe resource fetching in Android webview due to $@. | UnsafeAndroidAccess.java:31:20:31:30 | getIntent(...) | user input vulnerable to cross-origin and sensitive resource disclosure attacks | -| UnsafeAndroidAccess.java:55:3:55:21 | loadUrl(...) | UnsafeAndroidAccess.java:54:20:54:30 | getIntent(...) : Intent | UnsafeAndroidAccess.java:55:14:55:20 | thisUrl | Unsafe resource fetching in Android webview due to $@. | UnsafeAndroidAccess.java:54:20:54:30 | getIntent(...) | user input vulnerable to cross-origin and sensitive resource disclosure attacks | -| UnsafeAndroidAccess.java:97:3:97:21 | loadUrl(...) | UnsafeAndroidAccess.java:96:20:96:30 | getIntent(...) : Intent | UnsafeAndroidAccess.java:97:14:97:20 | thisUrl | Unsafe resource fetching in Android webview due to $@. | UnsafeAndroidAccess.java:96:20:96:30 | getIntent(...) | user input vulnerable to XSS attacks | -| UnsafeAndroidBroadcastReceiver.java:32:3:32:21 | loadUrl(...) | UnsafeAndroidBroadcastReceiver.java:16:41:16:53 | intent : Intent | UnsafeAndroidBroadcastReceiver.java:32:14:32:20 | thisUrl | Unsafe resource fetching in Android webview due to $@. | UnsafeAndroidBroadcastReceiver.java:16:41:16:53 | intent | user input vulnerable to cross-origin and sensitive resource disclosure attacks | diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.qlref b/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.qlref deleted file mode 100644 index abaed120e99..00000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.ql \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/options b/java/ql/test/experimental/query-tests/security/CWE-749/options deleted file mode 100644 index 43e25f608b6..00000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-749/options +++ /dev/null @@ -1 +0,0 @@ -// semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/google-android-9.0.0 diff --git a/java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml old mode 100644 new mode 100755 index 4f05c56c027..b215e4d3466 --- a/java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml @@ -1,11 +1,51 @@ - - - - + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + - \ No newline at end of file + + diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/IntentUtils.java b/java/ql/test/query-tests/security/CWE-749/IntentUtils.java similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-749/IntentUtils.java rename to java/ql/test/query-tests/security/CWE-749/IntentUtils.java diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/SafeActivity1.java b/java/ql/test/query-tests/security/CWE-749/SafeActivity1.java similarity index 75% rename from java/ql/test/experimental/query-tests/security/CWE-749/SafeActivity1.java rename to java/ql/test/query-tests/security/CWE-749/SafeActivity1.java index 9af651031cb..18759fa8cd1 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-749/SafeActivity1.java +++ b/java/ql/test/query-tests/security/CWE-749/SafeActivity1.java @@ -9,7 +9,9 @@ import android.webkit.WebView; import android.webkit.WebViewClient; public class SafeActivity1 extends Activity { - //Test onCreate with both JavaScript and cross-origin resource access enabled while taking remote user inputs from bundle extras + // Test onCreate with both JavaScript and cross-origin resource access enabled while taking + // remote user inputs from bundle extras. + // The Activity is explicitly not exported, even though it has an intent-filter. public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); setContentView(-1); @@ -29,6 +31,6 @@ public class SafeActivity1 extends Activity { }); String thisUrl = getIntent().getExtras().getString("url"); - wv.loadUrl(thisUrl); + wv.loadUrl(thisUrl); // Safe } -} \ No newline at end of file +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/SafeActivity2.java b/java/ql/test/query-tests/security/CWE-749/SafeActivity2.java similarity index 78% rename from java/ql/test/experimental/query-tests/security/CWE-749/SafeActivity2.java rename to java/ql/test/query-tests/security/CWE-749/SafeActivity2.java index b8959c657ca..be4d262e527 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-749/SafeActivity2.java +++ b/java/ql/test/query-tests/security/CWE-749/SafeActivity2.java @@ -9,7 +9,9 @@ import android.webkit.WebView; import android.webkit.WebViewClient; public class SafeActivity2 extends Activity { - //Test onCreate with both JavaScript and cross-origin resource access enabled while taking remote user inputs from bundle extras + // Test onCreate with both JavaScript and cross-origin resource access enabled while taking + // remote user inputs from bundle extras. + // The Activity is explicitly not exported. public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); setContentView(-1); @@ -29,6 +31,6 @@ public class SafeActivity2 extends Activity { }); String thisUrl = getIntent().getExtras().getString("url"); - wv.loadUrl(thisUrl); + wv.loadUrl(thisUrl); // Safe } -} \ No newline at end of file +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/SafeActivity3.java b/java/ql/test/query-tests/security/CWE-749/SafeActivity3.java similarity index 78% rename from java/ql/test/experimental/query-tests/security/CWE-749/SafeActivity3.java rename to java/ql/test/query-tests/security/CWE-749/SafeActivity3.java index 29ba013affb..8f4ed73cd4b 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-749/SafeActivity3.java +++ b/java/ql/test/query-tests/security/CWE-749/SafeActivity3.java @@ -9,7 +9,9 @@ import android.webkit.WebView; import android.webkit.WebViewClient; public class SafeActivity3 extends Activity { - //Test onCreate with both JavaScript and cross-origin resource access enabled while taking remote user inputs from bundle extras + // 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); @@ -29,6 +31,6 @@ public class SafeActivity3 extends Activity { }); String thisUrl = getIntent().getExtras().getString("url"); - wv.loadUrl(thisUrl); + wv.loadUrl(thisUrl); // Safe } -} \ No newline at end of file +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeActivity1.java b/java/ql/test/query-tests/security/CWE-749/UnsafeActivity1.java similarity index 76% rename from java/ql/test/experimental/query-tests/security/CWE-749/UnsafeActivity1.java rename to java/ql/test/query-tests/security/CWE-749/UnsafeActivity1.java index 3f97d7501b2..a7092839219 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeActivity1.java +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeActivity1.java @@ -9,7 +9,9 @@ import android.webkit.WebView; import android.webkit.WebViewClient; public class UnsafeActivity1 extends Activity { - //Test onCreate with both JavaScript and cross-origin resource access enabled while taking remote user inputs from bundle extras + // Test onCreate with both JavaScript and cross-origin resource access enabled while taking + // remote user inputs from bundle extras. + // The Activity is exported and has an intent-filter. public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); setContentView(-1); @@ -29,6 +31,6 @@ public class UnsafeActivity1 extends Activity { }); String thisUrl = getIntent().getExtras().getString("url"); - wv.loadUrl(thisUrl); + wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess } -} \ No newline at end of file +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeActivity2.java b/java/ql/test/query-tests/security/CWE-749/UnsafeActivity2.java similarity index 74% rename from java/ql/test/experimental/query-tests/security/CWE-749/UnsafeActivity2.java rename to java/ql/test/query-tests/security/CWE-749/UnsafeActivity2.java index c5b15c2ebba..5a70ba3d50b 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeActivity2.java +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeActivity2.java @@ -9,7 +9,9 @@ import android.webkit.WebView; import android.webkit.WebViewClient; public class UnsafeActivity2 extends Activity { - //Test onCreate with both JavaScript and cross-origin resource access enabled while taking remote user inputs from bundle extras + // Test onCreate with both JavaScript and cross-origin resource access enabled while taking + // remote user inputs from bundle extras. + // The Activity is implicitly exported because it has an intent-filter. public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); setContentView(-1); @@ -29,6 +31,6 @@ public class UnsafeActivity2 extends Activity { }); String thisUrl = getIntent().getExtras().getString("url"); - wv.loadUrl(thisUrl); + wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess } -} \ No newline at end of file +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeActivity3.java b/java/ql/test/query-tests/security/CWE-749/UnsafeActivity3.java similarity index 77% rename from java/ql/test/experimental/query-tests/security/CWE-749/UnsafeActivity3.java rename to java/ql/test/query-tests/security/CWE-749/UnsafeActivity3.java index 0fe6c342b5a..f7f88087d8f 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeActivity3.java +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeActivity3.java @@ -9,7 +9,9 @@ import android.webkit.WebView; import android.webkit.WebViewClient; public class UnsafeActivity3 extends Activity { - //Test onCreate with both JavaScript and cross-origin resource access enabled while taking remote user inputs from bundle extras + // Test onCreate with both JavaScript and cross-origin resource access enabled while taking + // remote user inputs from bundle extras. + // The Activity is explicitly exported. public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); setContentView(-1); @@ -29,6 +31,6 @@ public class UnsafeActivity3 extends Activity { }); String thisUrl = getIntent().getExtras().getString("url"); - wv.loadUrl(thisUrl); + wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess } -} \ No newline at end of file +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeActivity4.java b/java/ql/test/query-tests/security/CWE-749/UnsafeActivity4.java similarity index 69% rename from java/ql/test/experimental/query-tests/security/CWE-749/UnsafeActivity4.java rename to java/ql/test/query-tests/security/CWE-749/UnsafeActivity4.java index 4a4cb09c8f7..992e753f38d 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeActivity4.java +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeActivity4.java @@ -9,9 +9,15 @@ import android.webkit.WebView; import android.webkit.WebViewClient; public class UnsafeActivity4 extends Activity { - /** - * Test onCreate with both JavaScript and cross-origin resource access enabled while taking remote user inputs from bundle extras - * Note this case of invoking utility method that takes an Activity a then calls `a.getIntent().getStringExtra(...)` is not yet detected thus is beyond what the query is capable of. + /* + * 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 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); @@ -33,6 +39,6 @@ public class UnsafeActivity4 extends Activity { String thisUrl = IntentUtils.getIntentUrl(this); thisUrl = IntentUtils.getBundleUrl(this); - wv.loadUrl(thisUrl); + wv.loadUrl(thisUrl); // $ MISSING: hasUnsafeAndroidAccess } -} \ No newline at end of file +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.java b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java similarity index 60% rename from java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.java rename to java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java index ddddafd95d1..414a50f80c4 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.java +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java @@ -8,8 +8,10 @@ import android.webkit.WebSettings; import android.webkit.WebView; import android.webkit.WebViewClient; +// The Activity is implicitly exported because it has an intent-filter. public class UnsafeAndroidAccess extends Activity { - //Test onCreate with both JavaScript and cross-origin resource access enabled while taking remote user inputs from bundle extras + // Test onCreate with both JavaScript and cross-origin resource access enabled while taking + // remote user inputs from bundle extras public void testOnCreate1(Bundle savedInstanceState) { super.onCreate(savedInstanceState); setContentView(-1); @@ -29,10 +31,11 @@ public class UnsafeAndroidAccess extends Activity { }); String thisUrl = getIntent().getExtras().getString("url"); - wv.loadUrl(thisUrl); + wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess } - //Test onCreate with both JavaScript and cross-origin resource access enabled while taking remote user inputs from string extra + // Test onCreate with both JavaScript and cross-origin resource access enabled while taking + // remote user inputs from string extra public void testOnCreate2(Bundle savedInstanceState) { super.onCreate(savedInstanceState); setContentView(-1); @@ -52,10 +55,11 @@ public class UnsafeAndroidAccess extends Activity { }); String thisUrl = getIntent().getStringExtra("url"); - wv.loadUrl(thisUrl); + wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess } - //Test onCreate with both JavaScript and cross-origin resource access disabled by default while taking remote user inputs + // Test onCreate with both JavaScript and cross-origin resource access disabled by default while + // taking remote user inputs public void testOnCreate3(Bundle savedInstanceState) { super.onCreate(savedInstanceState); setContentView(-1); @@ -72,10 +76,11 @@ public class UnsafeAndroidAccess extends Activity { }); String thisUrl = getIntent().getStringExtra("url"); - wv.loadUrl(thisUrl); + wv.loadUrl(thisUrl); // Safe } - //Test onCreate with JavaScript enabled but cross-origin resource access disabled while taking remote user inputs + // Test onCreate with JavaScript enabled but cross-origin resource access disabled while taking + // remote user inputs public void testOnCreate4(Bundle savedInstanceState) { super.onCreate(savedInstanceState); setContentView(-1); @@ -94,10 +99,11 @@ public class UnsafeAndroidAccess extends Activity { }); String thisUrl = getIntent().getStringExtra("url"); - wv.loadUrl(thisUrl); + wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess } - //Test onCreate with both JavaScript and cross-origin resource access enabled while not taking remote user inputs + // Test onCreate with both JavaScript and cross-origin resource access enabled while not taking + // remote user inputs public void testOnCreate5(Bundle savedInstanceState) { super.onCreate(savedInstanceState); setContentView(-1); @@ -116,6 +122,31 @@ public class UnsafeAndroidAccess extends Activity { } }); - wv.loadUrl("https://www.mycorp.com"); + wv.loadUrl("https://www.mycorp.com"); // Safe } -} \ No newline at end of file + + // Test onCreate with both JavaScript and cross-origin resource access enabled while taking + // remote user inputs and concatenating them to a safe URL. + public void testOnCreate6(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 = getIntent().getStringExtra("url"); + // This should be considered safe - the query lacks a proper sanitizer for partial URLs. + wv.loadUrl("https://www.mycorp.com/" + thisUrl); // $ SPURIOUS: hasUnsafeAndroidAccess + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidBroadcastReceiver.java b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidBroadcastReceiver.java similarity index 82% rename from java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidBroadcastReceiver.java rename to java/ql/test/query-tests/security/CWE-749/UnsafeAndroidBroadcastReceiver.java index e49d0b7b518..c25a042ea89 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidBroadcastReceiver.java +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidBroadcastReceiver.java @@ -11,7 +11,8 @@ import android.webkit.WebView; import android.webkit.WebViewClient; public class UnsafeAndroidBroadcastReceiver extends BroadcastReceiver { - //Test onCreate with JavaScript enabled but cross-origin resource access disabled while taking remote user inputs + // Test onCreate with JavaScript enabled but cross-origin resource access disabled while taking + // remote user inputs @Override public void onReceive(Context context, Intent intent) { String thisUrl = intent.getStringExtra("url"); @@ -29,6 +30,6 @@ public class UnsafeAndroidBroadcastReceiver extends BroadcastReceiver { } }); - wv.loadUrl(thisUrl); + wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess } -} \ No newline at end of file +} diff --git a/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java b/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java deleted file mode 100644 index 60f0b1d2821..00000000000 --- a/java/ql/test/query-tests/security/CWE-749/app/UnsafeAndroidAccess.java +++ /dev/null @@ -1,117 +0,0 @@ -package app; - -import android.app.Activity; -import android.os.Bundle; -import android.webkit.WebSettings; -import android.webkit.WebView; -import android.webkit.WebViewClient; -import com.android.internal.R; - -public class UnsafeAndroidAccess extends Activity { - public void onCreate(Bundle savedInstanceState) { - super.onCreate(savedInstanceState); - setContentView(R.layout.webview); - testJavaScriptEnabledWebView(); - testUniversalFileAccessEnabledWebView(); - testFileAccessEnabledWebView(); - testSafeWebView(); - testCrossOriginEnabledJsDisabledWebView(); - } - - private void testJavaScriptEnabledWebView() { - WebView wv = (WebView) findViewById(R.id.my_webview); - WebSettings webSettings = wv.getSettings(); - webSettings.setJavaScriptEnabled(true); - - wv.setWebViewClient(new WebViewClient() { - @Override - public boolean shouldOverrideUrlLoading(WebView view, String url) { - view.loadUrl(url); - return true; - } - }); - - String thisUrl = getIntent().getExtras().getString("url"); - wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess - wv.loadUrl("https://www.mycorp.com/" + thisUrl); // $ SPURIOUS: hasUnsafeAndroidAccess // Safe, needs sanitizer - wv.loadUrl("https://www.mycorp.com"); // Safe - } - - private void testUniversalFileAccessEnabledWebView() { - WebView wv = (WebView) findViewById(R.id.my_webview); - WebSettings webSettings = wv.getSettings(); - webSettings.setJavaScriptEnabled(true); - webSettings.setAllowUniversalAccessFromFileURLs(true); - - wv.setWebViewClient(new WebViewClient() { - @Override - public boolean shouldOverrideUrlLoading(WebView view, String url) { - view.loadUrl(url); - return true; - } - }); - - String thisUrl = getIntent().getStringExtra("url"); - wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess - wv.loadUrl("https://www.mycorp.com/" + thisUrl); // $ SPURIOUS: hasUnsafeAndroidAccess // Safe, needs sanitizer - wv.loadUrl("https://www.mycorp.com"); // Safe - } - - private void testFileAccessEnabledWebView() { - WebView wv = (WebView) findViewById(R.id.my_webview); - 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 = getIntent().getStringExtra("url"); - wv.loadUrl(thisUrl); // $hasUnsafeAndroidAccess - wv.loadUrl("https://www.mycorp.com/" + thisUrl); // $ SPURIOUS: hasUnsafeAndroidAccess // Safe, needs sanitizer - wv.loadUrl("https://www.mycorp.com"); // Safe - } - - private void testSafeWebView() { - WebView wv = (WebView) findViewById(-1); - - wv.setWebViewClient(new WebViewClient() { - @Override - public boolean shouldOverrideUrlLoading(WebView view, String url) { - view.loadUrl(url); - return true; - } - }); - - String thisUrl = getIntent().getExtras().getString("url"); - wv.loadUrl(thisUrl); // Safe - wv.loadUrl("https://www.mycorp.com/" + thisUrl); // Safe - wv.loadUrl("https://www.mycorp.com"); // Safe - } - - private void testCrossOriginEnabledJsDisabledWebView() { - WebView wv = (WebView) findViewById(-1); - WebSettings webSettings = wv.getSettings(); - webSettings.setAllowUniversalAccessFromFileURLs(true); - webSettings.setAllowFileAccessFromFileURLs(true); - - wv.setWebViewClient(new WebViewClient() { - @Override - public boolean shouldOverrideUrlLoading(WebView view, String url) { - view.loadUrl(url); - return true; - } - - }); - - String thisUrl = getIntent().getExtras().getString("url"); - wv.loadUrl(thisUrl); // Safe - wv.loadUrl("https://www.mycorp.com/" + thisUrl); // Safe - wv.loadUrl("https://www.mycorp.com"); // Safe - } -} diff --git a/java/ql/test/stubs/android/android/content/BroadcastReceiver.java b/java/ql/test/stubs/android/android/content/BroadcastReceiver.java new file mode 100644 index 00000000000..1d73018c96d --- /dev/null +++ b/java/ql/test/stubs/android/android/content/BroadcastReceiver.java @@ -0,0 +1,255 @@ +/* + * Copyright (C) 2006 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package android.content; + +import android.os.Bundle; +/** + * Base class for code that will receive intents sent by sendBroadcast(). + * + *

    If you don't need to send broadcasts across applications, consider using + * this class with {@link android.support.v4.content.LocalBroadcastManager} instead + * of the more general facilities described below. This will give you a much + * more efficient implementation (no cross-process communication needed) and allow + * you to avoid thinking about any security issues related to other applications + * being able to receive or send your broadcasts. + * + *

    You can either dynamically register an instance of this class with + * {@link Context#registerReceiver Context.registerReceiver()} + * or statically publish an implementation through the + * {@link android.R.styleable#AndroidManifestReceiver <receiver>} + * tag in your AndroidManifest.xml. + * + *

    Note: + *    If registering a receiver in your + * {@link android.app.Activity#onResume() Activity.onResume()} + * implementation, you should unregister it in + * {@link android.app.Activity#onPause() Activity.onPause()}. + * (You won't receive intents when paused, + * and this will cut down on unnecessary system overhead). Do not unregister in + * {@link android.app.Activity#onSaveInstanceState(android.os.Bundle) Activity.onSaveInstanceState()}, + * because this won't be called if the user moves back in the history + * stack. + * + *

    There are two major classes of broadcasts that can be received:

    + *
      + *
    • Normal broadcasts (sent with {@link Context#sendBroadcast(Intent) + * Context.sendBroadcast}) are completely asynchronous. All receivers of the + * broadcast are run in an undefined order, often at the same time. This is + * more efficient, but means that receivers cannot use the result or abort + * APIs included here. + *
    • Ordered broadcasts (sent with {@link Context#sendOrderedBroadcast(Intent, String) + * Context.sendOrderedBroadcast}) are delivered to one receiver at a time. + * As each receiver executes in turn, it can propagate a result to the next + * receiver, or it can completely abort the broadcast so that it won't be passed + * to other receivers. The order receivers run in can be controlled with the + * {@link android.R.styleable#AndroidManifestIntentFilter_priority + * android:priority} attribute of the matching intent-filter; receivers with + * the same priority will be run in an arbitrary order. + *
    + * + *

    Even in the case of normal broadcasts, the system may in some + * situations revert to delivering the broadcast one receiver at a time. In + * particular, for receivers that may require the creation of a process, only + * one will be run at a time to avoid overloading the system with new processes. + * In this situation, however, the non-ordered semantics hold: these receivers still + * cannot return results or abort their broadcast.

    + * + *

    Note that, although the Intent class is used for sending and receiving + * these broadcasts, the Intent broadcast mechanism here is completely separate + * from Intents that are used to start Activities with + * {@link Context#startActivity Context.startActivity()}. + * There is no way for a BroadcastReceiver + * to see or capture Intents used with startActivity(); likewise, when + * you broadcast an Intent, you will never find or start an Activity. + * These two operations are semantically very different: starting an + * Activity with an Intent is a foreground operation that modifies what the + * user is currently interacting with; broadcasting an Intent is a background + * operation that the user is not normally aware of. + * + *

    The BroadcastReceiver class (when launched as a component through + * a manifest's {@link android.R.styleable#AndroidManifestReceiver <receiver>} + * tag) is an important part of an + * application's overall lifecycle.

    + * + *

    Topics covered here: + *

      + *
    1. Security + *
    2. Receiver Lifecycle + *
    3. Process Lifecycle + *
    + * + *
    + *

    Developer Guides

    + *

    For information about how to use this class to receive and resolve intents, read the + * Intents and Intent Filters + * developer guide.

    + *
    + * + * + *

    Security

    + * + *

    Receivers used with the {@link Context} APIs are by their nature a + * cross-application facility, so you must consider how other applications + * may be able to abuse your use of them. Some things to consider are: + * + *

      + *

    • The Intent namespace is global. Make sure that Intent action names and + * other strings are written in a namespace you own, or else you may inadvertantly + * conflict with other applications. + *

    • When you use {@link Context#registerReceiver(BroadcastReceiver, IntentFilter)}, + * any application may send broadcasts to that registered receiver. You can + * control who can send broadcasts to it through permissions described below. + *

    • When you publish a receiver in your application's manifest and specify + * intent-filters for it, any other application can send broadcasts to it regardless + * of the filters you specify. To prevent others from sending to it, make it + * unavailable to them with android:exported="false". + *

    • When you use {@link Context#sendBroadcast(Intent)} or related methods, + * normally any other application can receive these broadcasts. You can control who + * can receive such broadcasts through permissions described below. Alternatively, + * starting with {@link android.os.Build.VERSION_CODES#ICE_CREAM_SANDWICH}, you + * can also safely restrict the broadcast to a single application with + * {@link Intent#setPackage(String) Intent.setPackage} + *

    + * + *

    None of these issues exist when using + * {@link android.support.v4.content.LocalBroadcastManager}, since intents + * broadcast it never go outside of the current process. + * + *

    Access permissions can be enforced by either the sender or receiver + * of a broadcast. + * + *

    To enforce a permission when sending, you supply a non-null + * permission argument to + * {@link Context#sendBroadcast(Intent, String)} or + * {@link Context#sendOrderedBroadcast(Intent, String, BroadcastReceiver, android.os.Handler, int, String, Bundle)}. + * Only receivers who have been granted this permission + * (by requesting it with the + * {@link android.R.styleable#AndroidManifestUsesPermission <uses-permission>} + * tag in their AndroidManifest.xml) will be able to receive + * the broadcast. + * + *

    To enforce a permission when receiving, you supply a non-null + * permission when registering your receiver -- either when calling + * {@link Context#registerReceiver(BroadcastReceiver, IntentFilter, String, android.os.Handler)} + * or in the static + * {@link android.R.styleable#AndroidManifestReceiver <receiver>} + * tag in your AndroidManifest.xml. Only broadcasters who have + * been granted this permission (by requesting it with the + * {@link android.R.styleable#AndroidManifestUsesPermission <uses-permission>} + * tag in their AndroidManifest.xml) will be able to send an + * Intent to the receiver. + * + *

    See the Security and Permissions + * document for more information on permissions and security in general. + * + * + *

    Receiver Lifecycle

    + * + *

    A BroadcastReceiver object is only valid for the duration of the call + * to {@link #onReceive}. Once your code returns from this function, + * the system considers the object to be finished and no longer active. + * + *

    This has important repercussions to what you can do in an + * {@link #onReceive} implementation: anything that requires asynchronous + * operation is not available, because you will need to return from the + * function to handle the asynchronous operation, but at that point the + * BroadcastReceiver is no longer active and thus the system is free to kill + * its process before the asynchronous operation completes. + * + *

    In particular, you may not show a dialog or bind to a service from + * within a BroadcastReceiver. For the former, you should instead use the + * {@link android.app.NotificationManager} API. For the latter, you can + * use {@link android.content.Context#startService Context.startService()} to + * send a command to the service. + * + * + *

    Process Lifecycle

    + * + *

    A process that is currently executing a BroadcastReceiver (that is, + * currently running the code in its {@link #onReceive} method) is + * considered to be a foreground process and will be kept running by the + * system except under cases of extreme memory pressure. + * + *

    Once you return from onReceive(), the BroadcastReceiver is no longer + * active, and its hosting process is only as important as any other application + * components that are running in it. This is especially important because if + * that process was only hosting the BroadcastReceiver (a common case for + * applications that the user has never or not recently interacted with), then + * upon returning from onReceive() the system will consider its process + * to be empty and aggressively kill it so that resources are available for other + * more important processes. + * + *

    This means that for longer-running operations you will often use + * a {@link android.app.Service} in conjunction with a BroadcastReceiver to keep + * the containing process active for the entire time of your operation. + */ +public abstract class BroadcastReceiver { + + /** + * State for a result that is pending for a broadcast receiver. Returned + * by {@link BroadcastReceiver#goAsync() goAsync()} + * while in {@link BroadcastReceiver#onReceive BroadcastReceiver.onReceive()}. + * This allows you to return from onReceive() without having the broadcast + * terminate; you must call {@link #finish()} once you are done with the + * broadcast. This allows you to process the broadcast off of the main + * thread of your app. + * + *

    Note on threading: the state inside of this class is not itself + * thread-safe, however you can use it from any thread if you properly + * sure that you do not have races. Typically this means you will hand + * the entire object to another thread, which will be solely responsible + * for setting any results and finally calling {@link #finish()}. + */ + + public BroadcastReceiver() { + } + /** + * This method is called when the BroadcastReceiver is receiving an Intent + * broadcast. During this time you can use the other methods on + * BroadcastReceiver to view/modify the current result values. This method + * is always called within the main thread of its process, unless you + * explicitly asked for it to be scheduled on a different thread using + * {@link android.content.Context#registerReceiver(BroadcastReceiver, + * IntentFilter, String, android.os.Handler)}. When it runs on the main + * thread you should + * never perform long-running operations in it (there is a timeout of + * 10 seconds that the system allows before considering the receiver to + * be blocked and a candidate to be killed). You cannot launch a popup dialog + * in your implementation of onReceive(). + * + *

    If this BroadcastReceiver was launched through a <receiver> tag, + * then the object is no longer alive after returning from this + * function. This means you should not perform any operations that + * return a result to you asynchronously -- in particular, for interacting + * with services, you should use + * {@link Context#startService(Intent)} instead of + * {@link Context#bindService(Intent, ServiceConnection, int)}. If you wish + * to interact with a service that is already running, you can use + * {@link #peekService}. + * + *

    The Intent filters used in {@link android.content.Context#registerReceiver} + * and in application manifests are not guaranteed to be exclusive. They + * are hints to the operating system about how to find suitable recipients. It is + * possible for senders to force delivery to specific recipients, bypassing filter + * resolution. For this reason, {@link #onReceive(Context, Intent) onReceive()} + * implementations should respond only to known actions, ignoring any unexpected + * Intents that they may receive. + * + * @param context The Context in which the receiver is running. + * @param intent The Intent being received. + */ + public abstract void onReceive(Context context, Intent intent); +} \ No newline at end of file From 8553ca1019e0ca29ec8a6c5feea61876e9890d89 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 10 May 2021 15:42:20 +0200 Subject: [PATCH 17/25] Autoformatting --- java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll index e07094fe599..26c4cdb24a4 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll @@ -32,7 +32,7 @@ private class DefaultUrlResourceSinkModel extends SinkModelCsv { /** * Cross-origin access enabled resource fetch. - * + * * It requires JavaScript to be enabled too to be considered a valid sink. */ private class CrossOriginUrlResourceSink extends JavaScriptEnabledUrlResourceSink { From 3ec2c1308ef562dfd80f6cc3f2df1a1f9089dcce Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 17 Jun 2021 14:58:27 +0200 Subject: [PATCH 18/25] Add `RequestForgerySanitizer` --- java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql | 5 +++++ .../query-tests/security/CWE-749/UnsafeAndroidAccess.java | 2 +- .../security/CWE-749/UnsafeAndroidAccessTest.ql | 7 ++++++- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql index 6bc332d085c..34796fdc19b 100644 --- a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql +++ b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql @@ -13,6 +13,7 @@ import java import semmle.code.java.dataflow.FlowSources +import semmle.code.java.security.RequestForgeryConfig import semmle.code.java.security.UnsafeAndroidAccess import DataFlow::PathGraph @@ -25,6 +26,10 @@ class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { sink instanceof UrlResourceSink } + + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer instanceof RequestForgerySanitizer + } } from DataFlow::PathNode source, DataFlow::PathNode sink, FetchUntrustedResourceConfiguration conf diff --git a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java index 414a50f80c4..b7d04477da2 100644 --- a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java @@ -147,6 +147,6 @@ public class UnsafeAndroidAccess extends Activity { String thisUrl = getIntent().getStringExtra("url"); // This should be considered safe - the query lacks a proper sanitizer for partial URLs. - wv.loadUrl("https://www.mycorp.com/" + thisUrl); // $ SPURIOUS: hasUnsafeAndroidAccess + wv.loadUrl("https://www.mycorp.com/" + thisUrl); } } diff --git a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql index 6563286f5c7..697fc99dc71 100644 --- a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql @@ -1,8 +1,9 @@ import java import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.FlowSources -import TestUtilities.InlineExpectationsTest +import semmle.code.java.security.RequestForgeryConfig import semmle.code.java.security.UnsafeAndroidAccess +import TestUtilities.InlineExpectationsTest class Conf extends TaintTracking::Configuration { Conf() { this = "qltest:cwe:unsafe-android-access" } @@ -10,6 +11,10 @@ class Conf extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { sink instanceof UrlResourceSink } + + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer instanceof RequestForgerySanitizer + } } class UnsafeAndroidAccessTest extends InlineExpectationsTest { From 1014400a08252b10504bcd16fb49eeee33222842 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 17 Jun 2021 15:03:45 +0200 Subject: [PATCH 19/25] Fix test comments --- .../test/query-tests/security/CWE-749/UnsafeAndroidAccess.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java index b7d04477da2..95dbc326d3d 100644 --- a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccess.java @@ -146,7 +146,6 @@ public class UnsafeAndroidAccess extends Activity { }); String thisUrl = getIntent().getStringExtra("url"); - // This should be considered safe - the query lacks a proper sanitizer for partial URLs. - wv.loadUrl("https://www.mycorp.com/" + thisUrl); + wv.loadUrl("https://www.mycorp.com/" + thisUrl); // Safe } } From 26999c7ac4492e5f08cb9153b8dd18720522660d Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 20 Jul 2021 17:46:35 +0200 Subject: [PATCH 20/25] Decouple UnsafeAndroidAccess.qll to reuse the taint tracking configuration --- .../CWE/CWE-749/UnsafeAndroidAccess.ql | 19 +-------------- .../code/java/dataflow/ExternalFlow.qll | 1 - .../java/security/UnsafeAndroidAccess.qll | 24 ++++++------------- .../security/UnsafeAndroidAccessQuery.qll | 22 +++++++++++++++++ .../CWE-749/UnsafeAndroidAccessTest.ql | 21 ++++------------ 5 files changed, 34 insertions(+), 53 deletions(-) create mode 100644 java/ql/src/semmle/code/java/security/UnsafeAndroidAccessQuery.qll diff --git a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql index 34796fdc19b..76252cc04f5 100644 --- a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql +++ b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql @@ -12,26 +12,9 @@ */ import java -import semmle.code.java.dataflow.FlowSources -import semmle.code.java.security.RequestForgeryConfig -import semmle.code.java.security.UnsafeAndroidAccess +import semmle.code.java.security.UnsafeAndroidAccessQuery import DataFlow::PathGraph -/** - * Taint configuration tracking flow from untrusted inputs to a resource fetching call. - */ -class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration { - FetchUntrustedResourceConfiguration() { this = "FetchUntrustedResourceConfiguration" } - - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node sink) { sink instanceof UrlResourceSink } - - override predicate isSanitizer(DataFlow::Node sanitizer) { - sanitizer instanceof RequestForgerySanitizer - } -} - from DataFlow::PathNode source, DataFlow::PathNode sink, FetchUntrustedResourceConfiguration conf where conf.hasFlowPath(source, sink) select sink.getNode(), source, sink, "Unsafe resource fetching in Android webview due to $@.", diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index f39d11aab6d..e3d9e7e35cf 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -98,7 +98,6 @@ private module Frameworks { private import semmle.code.java.security.InformationLeak private import semmle.code.java.security.JexlInjectionSinkModels private import semmle.code.java.security.LdapInjection - private import semmle.code.java.security.UnsafeAndroidAccess private import semmle.code.java.security.XPath private import semmle.code.java.frameworks.android.SQLite private import semmle.code.java.frameworks.Jdbc diff --git a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll index 26c4cdb24a4..f13a970fe16 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll @@ -3,9 +3,9 @@ */ import java -import semmle.code.java.frameworks.android.WebView -import semmle.code.java.dataflow.DataFlow -import semmle.code.java.dataflow.ExternalFlow +private import semmle.code.java.frameworks.android.WebView +private import semmle.code.java.dataflow.DataFlow +private import semmle.code.java.dataflow.ExternalFlow /** * A sink that represents a method that fetches a web resource in Android. @@ -19,17 +19,6 @@ abstract class UrlResourceSink extends DataFlow::Node { abstract string getSinkType(); } -/** CSV sink models representing methods susceptible to Unsafe Resource Fetching attacks. */ -private class DefaultUrlResourceSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "android.webkit;WebView;true;loadUrl;;;Argument[0];unsafe-android-access", - "android.webkit;WebView;true;postUrl;;;Argument[0];unsafe-android-access" - ] - } -} - /** * Cross-origin access enabled resource fetch. * @@ -57,9 +46,10 @@ private class CrossOriginUrlResourceSink extends JavaScriptEnabledUrlResourceSin */ private class JavaScriptEnabledUrlResourceSink extends UrlResourceSink { JavaScriptEnabledUrlResourceSink() { - sinkNode(this, "unsafe-android-access") and - exists(VarAccess webviewVa, MethodAccess getSettingsMa, Variable v | - this.asExpr().(Argument).getCall().getQualifier() = webviewVa and + exists(MethodAccess loadUrl, VarAccess webviewVa, MethodAccess getSettingsMa, Variable v | + loadUrl.getArgument(0) = this.asExpr() and + loadUrl.getMethod() instanceof WebViewLoadUrlMethod and + loadUrl.getQualifier() = webviewVa and getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and webviewVa.getVariable().getAnAccess() = getSettingsMa.getQualifier() and v.getAnAssignedValue() = getSettingsMa and diff --git a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccessQuery.qll b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccessQuery.qll new file mode 100644 index 00000000000..133cbe60590 --- /dev/null +++ b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccessQuery.qll @@ -0,0 +1,22 @@ +/** Provides taint tracking configurations to be used in Unsafe Resource Fetching queries. */ + +import java +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.RequestForgery +import semmle.code.java.security.UnsafeAndroidAccess + +/** + * Taint configuration tracking flow from untrusted inputs to a resource fetching call. + */ +class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration { + FetchUntrustedResourceConfiguration() { this = "FetchUntrustedResourceConfiguration" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof UrlResourceSink } + + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer instanceof RequestForgerySanitizer + } +} diff --git a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql index 697fc99dc71..14f72c5e88c 100644 --- a/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql +++ b/java/ql/test/query-tests/security/CWE-749/UnsafeAndroidAccessTest.ql @@ -1,22 +1,7 @@ import java -import semmle.code.java.dataflow.DataFlow -import semmle.code.java.dataflow.FlowSources -import semmle.code.java.security.RequestForgeryConfig -import semmle.code.java.security.UnsafeAndroidAccess +import semmle.code.java.security.UnsafeAndroidAccessQuery import TestUtilities.InlineExpectationsTest -class Conf extends TaintTracking::Configuration { - Conf() { this = "qltest:cwe:unsafe-android-access" } - - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node sink) { sink instanceof UrlResourceSink } - - override predicate isSanitizer(DataFlow::Node sanitizer) { - sanitizer instanceof RequestForgerySanitizer - } -} - class UnsafeAndroidAccessTest extends InlineExpectationsTest { UnsafeAndroidAccessTest() { this = "HasUnsafeAndroidAccess" } @@ -24,7 +9,9 @@ class UnsafeAndroidAccessTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasUnsafeAndroidAccess" and - exists(DataFlow::Node src, DataFlow::Node sink, Conf conf | conf.hasFlow(src, sink) | + exists(DataFlow::Node src, DataFlow::Node sink, FetchUntrustedResourceConfiguration conf | + conf.hasFlow(src, sink) + | sink.getLocation() = location and element = sink.toString() and value = "" From 4622d8590b901550f9f9e7077149c8d63ffe270c Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 20 Jul 2021 17:50:58 +0200 Subject: [PATCH 21/25] Fix change note --- java/change-notes/2021-05-06-unsafe-android-access-query.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/change-notes/2021-05-06-unsafe-android-access-query.md b/java/change-notes/2021-05-06-unsafe-android-access-query.md index 428e6f0e282..8a1651acb60 100644 --- a/java/change-notes/2021-05-06-unsafe-android-access-query.md +++ b/java/change-notes/2021-05-06-unsafe-android-access-query.md @@ -1,2 +1,2 @@ lgtm,codescanning -* The query "Unsafe resource fetching in Android webview" (`java/android/unsafe-android-webview-fetch`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3706) \ No newline at end of file +* The query "Unsafe resource fetching in Android webview" (`java/android/unsafe-android-webview-fetch`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3706). \ No newline at end of file From bdf0f582a499369c8a6cab38f1d88f0150d754a2 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 29 Jul 2021 16:34:21 +0200 Subject: [PATCH 22/25] QLDoc improvements from code review Co-authored-by: Felicity Chapman Co-authored-by: Anders Schack-Mulligen --- .../2021-05-06-unsafe-android-access-query.md | 2 +- .../src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql | 8 ++++---- .../code/java/security/UnsafeAndroidAccess.qll | 12 ++++++------ .../code/java/security/UnsafeAndroidAccessQuery.qll | 2 +- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/java/change-notes/2021-05-06-unsafe-android-access-query.md b/java/change-notes/2021-05-06-unsafe-android-access-query.md index 8a1651acb60..4425d32816b 100644 --- a/java/change-notes/2021-05-06-unsafe-android-access-query.md +++ b/java/change-notes/2021-05-06-unsafe-android-access-query.md @@ -1,2 +1,2 @@ lgtm,codescanning -* The query "Unsafe resource fetching in Android webview" (`java/android/unsafe-android-webview-fetch`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3706). \ No newline at end of file +* The query "Unsafe resource fetching in Android WebView" (`java/android/unsafe-android-webview-fetch`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3706). diff --git a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql index 76252cc04f5..9a58678367c 100644 --- a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql +++ b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql @@ -1,7 +1,7 @@ /** - * @name Unsafe resource fetching in Android webview - * @description JavaScript rendered inside WebViews can access any protected - * application file and web resource from any origin + * @name Unsafe resource fetching in Android WebView + * @description JavaScript rendered inside WebViews can access protected + * application files and web resources from any origin exposing them to attack. * @kind path-problem * @problem.severity warning * @precision medium @@ -17,5 +17,5 @@ import DataFlow::PathGraph from DataFlow::PathNode source, DataFlow::PathNode sink, FetchUntrustedResourceConfiguration conf where conf.hasFlowPath(source, sink) -select sink.getNode(), source, sink, "Unsafe resource fetching in Android webview due to $@.", +select sink.getNode(), source, sink, "Unsafe resource fetching in Android WebView due to $@.", source.getNode(), sink.getNode().(UrlResourceSink).getSinkType() diff --git a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll index f13a970fe16..986120cedd3 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll @@ -14,15 +14,15 @@ private import semmle.code.java.dataflow.ExternalFlow */ abstract class UrlResourceSink extends DataFlow::Node { /** - * Returns a description of this vulnerability, + * Gets a description of this vulnerability. */ abstract string getSinkType(); } /** - * Cross-origin access enabled resource fetch. + * A cross-origin access enabled resource fetch. * - * It requires JavaScript to be enabled too to be considered a valid sink. + * Only considered a valid sink when JavaScript is also enabled. */ private class CrossOriginUrlResourceSink extends JavaScriptEnabledUrlResourceSink { CrossOriginUrlResourceSink() { @@ -61,7 +61,7 @@ private class JavaScriptEnabledUrlResourceSink extends UrlResourceSink { } /** - * Methods allowing any-local-file and cross-origin access in the WebSettings class + * A method allowing any-local-file and cross-origin access in the WebSettings class. */ private class CrossOriginAccessMethod extends Method { CrossOriginAccessMethod() { @@ -71,7 +71,7 @@ private class CrossOriginAccessMethod extends Method { } /** - * `setJavaScriptEnabled` method for the webview + * The `setJavaScriptEnabled` method for the webview. */ private class AllowJavaScriptMethod extends Method { AllowJavaScriptMethod() { @@ -81,7 +81,7 @@ private class AllowJavaScriptMethod extends Method { } /** - * Holds if a call to `v.setJavaScriptEnabled(true)` exists + * Holds if a call to `v.setJavaScriptEnabled(true)` exists. */ private predicate isJSEnabled(Variable v) { exists(MethodAccess jsa | diff --git a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccessQuery.qll b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccessQuery.qll index 133cbe60590..6bd9833047d 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccessQuery.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccessQuery.qll @@ -7,7 +7,7 @@ import semmle.code.java.security.RequestForgery import semmle.code.java.security.UnsafeAndroidAccess /** - * Taint configuration tracking flow from untrusted inputs to a resource fetching call. + * A taint configuration tracking flow from untrusted inputs to a resource fetching call. */ class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration { FetchUntrustedResourceConfiguration() { this = "FetchUntrustedResourceConfiguration" } From 6e3b6dcb98252908a4176fdaea17398dcd2fff65 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 29 Jul 2021 16:36:38 +0200 Subject: [PATCH 23/25] Imporve qhelp --- .../CWE/CWE-749/UnsafeAndroidAccess.qhelp | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp index 165b9310464..cdf817845ee 100644 --- a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp +++ b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp @@ -2,29 +2,34 @@ -

    Android WebViews that allow loading URLs controlled by external inputs and whose JavaScript interface is enabled are potentially vulnerable to cross-site scripting and sensitive resource disclosure attacks.

    -

    A WebView whose WebSettings object has setAllowFileAccessFromFileURLs(true) or setAllowUniversalAccessFromFileURLs(true) called must not load any untrusted web content.

    +

    Android WebViews that allow loading URLs controlled by external inputs, and whose JavaScript interface is enabled, are potentially vulnerable to cross-site scripting and sensitive resource disclosure attacks.

    +

    A WebView whose WebSettings object has called setAllowFileAccessFromFileURLs(true) or setAllowUniversalAccessFromFileURLs(true) must not load any untrusted web content.

    Enabling these settings allows malicious scripts loaded in a file:// context to launch cross-site scripting attacks, either accessing arbitrary local files including WebView cookies, session tokens, private app data or even credentials used on arbitrary web sites.

    This query detects the following two scenarios:

    1. Vulnerability introduced by WebViews with JavaScript enabled and remote inputs allowed.
      2. -
    2. A more severe vulnerability when allowing cross-origin resource access is also enabled. The setting was deprecated in API level 30 (Android 11), but most devices are still affected, especially given that some Android phones are updated slowly or no longer updated at all.
      3. +
    3. A more severe vulnerability when "allow cross-origin resource access" is also enabled. This setting was deprecated in API level 30 (Android 11), but most devices are still affected, especially since some Android phones are updated slowly or no longer updated at all.
    -

    Only allow trusted web content to be displayed in WebViews when JavaScript is enabled. Disallow cross-origin resource access in WebSetting to reduce the attack surface.

    +

    Only allow trusted web content to be displayed in WebViews when JavaScript is enabled. Disallow cross-origin resource access in WebSettings to reduce the attack surface.

     -

    The following example shows both 'BAD' and 'GOOD' configurations. In the 'BAD' configuration, setting is enabled and JavaScript is enabled while URLs are loaded from externally controlled inputs. In the 'GOOD' configuration, JavaScript is disabled or only trusted web content is allowed to be loaded.

    +

    The following example shows both 'BAD' and 'GOOD' configurations. In the 'BAD' configuration, JavaScript and the allow access setting are enabled and URLs are loaded from externally controlled inputs. In the 'GOOD' configuration, JavaScript is disabled or only trusted web content is allowed to be loaded.
  • - Fixing a File-based XSS Vulnerability - OWASP - Testing WebView Protocol Handlers (MSTG-PLATFORM-5 and MSTG-PLATFORM-6) + Google Help: Fixing a File-based XSS Vulnerability +
    • +
  • + OWASP: Testing JavaScript Execution in WebViews (MSTG-PLATFORM-5) +
    • +
  • + OWASP: Testing WebView Protocol Handlers (MSTG-PLATFORM-6)
    • \ No newline at end of file From 3fcc9fae79ddfbc299c2245e68e846be6737e0da Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 29 Jul 2021 16:48:47 +0200 Subject: [PATCH 24/25] Refactor sinks to reuse code --- .../java/security/UnsafeAndroidAccess.qll | 34 ++++++++++++------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll index 986120cedd3..d25f5c4a05c 100644 --- a/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll +++ b/java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll @@ -26,13 +26,11 @@ abstract class UrlResourceSink extends DataFlow::Node { */ private class CrossOriginUrlResourceSink extends JavaScriptEnabledUrlResourceSink { CrossOriginUrlResourceSink() { - exists(MethodAccess ma, MethodAccess getSettingsMa | + exists(Variable settings, MethodAccess ma | + webViewLoadUrl(this.asExpr(), settings) and ma.getMethod() instanceof CrossOriginAccessMethod and ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true and - ma.getQualifier().(VarAccess).getVariable().getAnAssignedValue() = getSettingsMa and - getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and - getSettingsMa.getQualifier().(VarAccess).getVariable().getAnAccess() = - this.asExpr().(Argument).getCall().getQualifier() + ma.getQualifier() = settings.getAnAccess() ) } @@ -46,20 +44,30 @@ private class CrossOriginUrlResourceSink extends JavaScriptEnabledUrlResourceSin */ private class JavaScriptEnabledUrlResourceSink extends UrlResourceSink { JavaScriptEnabledUrlResourceSink() { - exists(MethodAccess loadUrl, VarAccess webviewVa, MethodAccess getSettingsMa, Variable v | - loadUrl.getArgument(0) = this.asExpr() and - loadUrl.getMethod() instanceof WebViewLoadUrlMethod and - loadUrl.getQualifier() = webviewVa and - getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and - webviewVa.getVariable().getAnAccess() = getSettingsMa.getQualifier() and - v.getAnAssignedValue() = getSettingsMa and - isJSEnabled(v) + exists(Variable settings | + webViewLoadUrl(this.asExpr(), settings) and + isJSEnabled(settings) ) } override string getSinkType() { result = "user input vulnerable to XSS attacks" } } +/** + * Holds if a `WebViewLoadUrlMethod` method is called with the given `urlArg` on a + * WebView with settings stored in `settings`. + */ +private predicate webViewLoadUrl(Expr urlArg, Variable settings) { + exists(MethodAccess loadUrl, Variable webview, MethodAccess getSettings | + loadUrl.getArgument(0) = urlArg and + loadUrl.getMethod() instanceof WebViewLoadUrlMethod and + loadUrl.getQualifier() = webview.getAnAccess() and + getSettings.getMethod() instanceof WebViewGetSettingsMethod and + webview.getAnAccess() = getSettings.getQualifier() and + settings.getAnAssignedValue() = getSettings + ) +} + /** * A method allowing any-local-file and cross-origin access in the WebSettings class. */ From 4435853c8a38525e3e30866b8419286189758138 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 2 Aug 2021 09:56:40 +0200 Subject: [PATCH 25/25] Apply suggestions from code review Co-authored-by: Felicity Chapman --- .../ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp index cdf817845ee..88756909e4b 100644 --- a/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp +++ b/java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp @@ -2,12 +2,12 @@ -

    Android WebViews that allow loading URLs controlled by external inputs, and whose JavaScript interface is enabled, are potentially vulnerable to cross-site scripting and sensitive resource disclosure attacks.

    +

    Android WebViews that allow externally controlled URLs to be loaded, and whose JavaScript interface is enabled, are potentially vulnerable to cross-site scripting and sensitive resource disclosure attacks.

    A WebView whose WebSettings object has called setAllowFileAccessFromFileURLs(true) or setAllowUniversalAccessFromFileURLs(true) must not load any untrusted web content.

    -

    Enabling these settings allows malicious scripts loaded in a file:// context to launch cross-site scripting attacks, either accessing arbitrary local files including WebView cookies, session tokens, private app data or even credentials used on arbitrary web sites.

    +

    Enabling these settings allows malicious scripts loaded in a file:// context to launch cross-site scripting attacks, accessing arbitrary local files including WebView cookies, session tokens, private app data or even credentials used on arbitrary web sites.

    This query detects the following two scenarios:

      -
    1. Vulnerability introduced by WebViews with JavaScript enabled and remote inputs allowed.
      2. +
    2. A vulnerability introduced by WebViews when JavaScript is enabled and remote inputs are allowed.
    3. A more severe vulnerability when "allow cross-origin resource access" is also enabled. This setting was deprecated in API level 30 (Android 11), but most devices are still affected, especially since some Android phones are updated slowly or no longer updated at all.
     @@ -32,4 +32,4 @@ OWASP: Testing WebView Protocol Handlers (MSTG-PLATFORM-6)     - \ No newline at end of file +