Merge pull request #9129 from atorralba/atorralba/get-underlying-expr

Java: Add Expr::getUnderlyingExpr predicate

Author: atorralba

java/ql/lib/change-notes/2022-05-12-get-underlying-expr.md

@@ -0,0 +1,4 @@
+---
+category: feature
+---
+* The QL predicate `Expr::getUnderlyingExpr` has been added. It can be used to look through casts and not-null expressions and obtain the underlying expression to which they apply.

java/ql/lib/semmle/code/java/Expr.qll

@@ -100,6 +100,18 @@ class Expr extends ExprParent, @expr {
 
   /** Holds if this expression is parenthesized. */
   predicate isParenthesized() { isParenthesized(this, _) }
+
+  /**
+   * Gets the underlying expression looking through casts and not-nulls, if any.
+   * Otherwise just gets this expression.
+   */
+  Expr getUnderlyingExpr() {
+    if this instanceof CastingExpr or this instanceof NotNullExpr
+    then
+      result = this.(CastingExpr).getExpr().getUnderlyingExpr() or
+      result = this.(NotNullExpr).getExpr().getUnderlyingExpr()
+    else result = this
+  }
 }
 
 /**

java/ql/lib/semmle/code/java/security/CleartextStorageSharedPrefsQuery.qll

@@ -51,7 +51,7 @@ private predicate sharedPreferencesInput(DataFlow::Node editor, Expr input) {
   exists(MethodAccess m |
     m.getMethod() instanceof PutSharedPreferenceMethod and
     input = m.getArgument(1) and
-    editor.asExpr() = m.getQualifier()
+    editor.asExpr() = m.getQualifier().getUnderlyingExpr()
   )
 }
 
@@ -61,7 +61,7 @@ private predicate sharedPreferencesInput(DataFlow::Node editor, Expr input) {
  */
 private predicate sharedPreferencesStore(DataFlow::Node editor, MethodAccess m) {
   m.getMethod() instanceof StoreSharedPreferenceMethod and
-  editor.asExpr() = m.getQualifier()
+  editor.asExpr() = m.getQualifier().getUnderlyingExpr()
 }
 
 /** Flow from `SharedPreferences.Editor` to either a setter or a store method. */

java/ql/lib/semmle/code/java/security/UnsafeAndroidAccess.qll

@@ -75,15 +75,18 @@ private predicate webViewLoadUrl(Argument urlArg, WebViewRef webview) {
     loadUrl.getArgument(0) = urlArg and
     loadUrl.getMethod() instanceof WebViewLoadUrlMethod
   |
+    webview.getAnAccess() = DataFlow::exprNode(loadUrl.getQualifier().getUnderlyingExpr())
+    or
     webview.getAnAccess() = DataFlow::getInstanceArgument(loadUrl)
     or
     // `webview` is received as a parameter of an event method in a custom `WebViewClient`,
     // so we need to find `WebViews` that use that specific `WebViewClient`.
     exists(WebViewClientEventMethod eventMethod, MethodAccess setWebClient |
       setWebClient.getMethod() instanceof WebViewSetWebViewClientMethod and
       setWebClient.getArgument(0).getType() = eventMethod.getDeclaringType() and
-      loadUrl.getQualifier() = eventMethod.getWebViewParameter().getAnAccess()
+      loadUrl.getQualifier().getUnderlyingExpr() = eventMethod.getWebViewParameter().getAnAccess()
     |
+      webview.getAnAccess() = DataFlow::exprNode(setWebClient.getQualifier().getUnderlyingExpr()) or
       webview.getAnAccess() = DataFlow::getInstanceArgument(setWebClient)
     )
   )

java/ql/test/query-tests/security/CWE-312/CleartextStorageSharedPrefsTestKt.kt

@@ -0,0 +1,11 @@
+import android.app.Activity
+import android.content.Context
+import android.content.SharedPreferences
+
+class CleartextStorageSharedPrefsTestKt : Activity() {
+    fun testSetSharedPrefs1(context: Context, name: String, password: String) {
+		val sharedPrefs = context.getSharedPreferences("user_prefs", Context.MODE_PRIVATE);
+		sharedPrefs.edit().putString("name", name).apply(); // Safe
+		sharedPrefs.edit().putString("password", password).apply(); // $ hasCleartextStorageSharedPrefs
+	}
+}

java/ql/test/query-tests/security/CWE-312/options

@@ -1 +1,2 @@
 // semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/google-android-9.0.0
+// codeql-extractor-kotlin-options: ${testdir}/../../../stubs/google-android-9.0.0

java/ql/test/query-tests/security/CWE-749/AndroidManifest.xml

@@ -44,6 +44,7 @@
 
         <activity android:name=".UnsafeActivity3" android:exported="true" />
         <activity android:name=".UnsafeActivity4" android:exported="true" />
+        <activity android:name=".UnsafeActivityKt" android:exported="true" />
 
         <receiver android:name=".UnsafeAndroidBroadcastReceiver" android:exported="true" />        
     </application>

java/ql/test/query-tests/security/CWE-749/UnsafeActivityKt.kt

@@ -0,0 +1,20 @@
+package com.example.app
+
+import android.app.Activity
+import android.os.Bundle
+import android.webkit.WebSettings
+import android.webkit.WebView
+import android.webkit.WebViewClient
+
+class UnsafeActivityKt : Activity() {
+    override fun onCreate(savedInstanceState : Bundle) {
+
+		val wv = findViewById<WebView>(-1)
+		// Implicit not-nulls happening here
+		wv.settings.setJavaScriptEnabled(true)
+		wv.settings.setAllowFileAccessFromFileURLs(true)
+
+		val thisUrl : String = intent.extras.getString("url")
+		wv.loadUrl(thisUrl) // $ hasUnsafeAndroidAccess
+    }
+}

java/ql/test/query-tests/security/CWE-749/options

@@ -1 +1,2 @@
-//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/android
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/google-android-9.0.0
+//codeql-extractor-kotlin-options: ${testdir}/../../../stubs/google-android-9.0.0