resolved merge conflict in AndroidManifest

Author: Jami Cogswell

java/ql/lib/semmle/code/xml/AndroidManifest.qll

@@ -67,6 +67,9 @@ class AndroidApplicationXmlElement extends XmlElement {
       attr.getValue() = "true"
     )
   }
+
+  /** Holds if this component element has an attribute with the name `permission`. */
+  predicate hasPermissionAttribute() { exists(this.getAttribute("permission")) }
 }
 
 /**
@@ -170,6 +173,11 @@ class AndroidComponentXmlElement extends XmlElement {
    */
   AndroidIntentFilterXmlElement getAnIntentFilterElement() { result = this.getAChild() }
 
+  /**
+   * Holds if this component element has an `<intent-filter>` child element.
+   */
+  predicate hasAnIntentFilterElement() { this.getAChild().hasName("intent-filter") }
+
   /**
    * Gets the value of the `android:name` attribute of this component element.
    */
@@ -220,6 +228,23 @@ class AndroidComponentXmlElement extends XmlElement {
    * Holds if the `android:exported` attribute of this component element is explicitly set to `false`.
    */
   predicate isNotExported() { this.getExportedAttributeValue() = "false" }
+
+  /**
+   * Holds if this component element has an `android:exported` attribute.
+   */
+  predicate hasExportedAttribute() { this.hasAttribute("exported") }
+
+  /** Holds if this component element has an attribute with the name `permission`. */
+  predicate hasPermissionAttribute() { exists(this.getAttribute("permission")) }
+
+  predicate isImplicitlyExported() {
+    not this.hasExportedAttribute() and
+    this.hasAnIntentFilterElement() and // Note: did not use getAnIntentFilterElement since don't need a return value
+    not this.hasPermissionAttribute() and
+    not this.getParent().(AndroidApplicationXmlElement).hasPermissionAttribute() and
+    not this.getAnIntentFilterElement().hasLauncherCategoryElement() and
+    not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory()
+  }
 }
 
 /**
@@ -234,6 +259,19 @@ class AndroidIntentFilterXmlElement extends XmlElement {
    * Gets an `<action>` child element of this `<intent-filter>` element.
    */
   AndroidActionXmlElement getAnActionElement() { result = this.getAChild() }
+
+  /**
+   * Gets a `<category>` child element of this `<intent-filter>` element.
+   */
+  AndroidCategoryXmlElement getACategoryElement() { result = this.getAChild("category") }
+
+  /**
+   * Holds if this `<intent-filter>` element has a `<category>` child element
+   * named "android.intent.category.LAUNCHER".
+   */
+  predicate hasLauncherCategoryElement() {
+    this.getACategoryElement().getAttributeValue("name") = "android.intent.category.LAUNCHER"
+  }
 }
 
 /**
@@ -257,3 +295,25 @@ class AndroidActionXmlElement extends XmlElement {
     )
   }
 }
+
+/**
+ * A `<category>` element in an Android manifest file.
+ */
+class AndroidCategoryXmlElement extends XMLElement {
+  AndroidCategoryXmlElement() {
+    this.getFile() instanceof AndroidManifestXmlFile and this.getName() = "category"
+  }
+
+  /**
+   * Gets the name of this category.
+   */
+  string getCategoryName() {
+    exists(XMLAttribute attr |
+      attr = this.getAnAttribute() and
+      attr.getNamespace().getPrefix() = "android" and
+      attr.getName() = "name"
+    |
+      result = attr.getValue()
+    )
+  }
+}

java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp

@@ -4,21 +4,25 @@
 <qhelp>
 
 <overview>
-<p>TODO: Replace the following
-When a debugger is enabled it could allow for entry points in the application or reveal sensitive information.</p>
+<p>The Android manifest file defines configuration settings for Android applications.
+In this file, the <code>android:debuggable</code> attribute of the <code>application</code> element can be used to
+define whether or not the application can be debugged. When set to <code>true</code>, this attribute will allow the
+application to be debugged even when running on a device in user mode.</p>
+
+<p>When a debugger is enabled it could allow for entry points in the application or reveal sensitive information.
+As a result, <code>android:debuggable</code> should only be enabled during development and should be disabled in
+production builds.</p>
 
 </overview>
 <recommendation>
 
-<p>TODO: Replace the following
-In Android applications either set the <code>android:debuggable</code> attribute to <code>false</code>
+<p>In Android applications either set the <code>android:debuggable</code> attribute to <code>false</code>
 or do not include it in the manifest. The default value when not included is <code>false</code>.</p>
 
 </recommendation>
 <example>
 
-<p>TODO: Replace the following
-In the example below, the <code>android:debuggable</code> attribute is set to <code>true</code>.</p>
+<p>In the example below, the <code>android:debuggable</code> attribute is set to <code>true</code>.</p>
 
 <!--<sample src="DebuggableTrue.xml" />-->
 
@@ -30,9 +34,17 @@ In the example below, the <code>android:debuggable</code> attribute is set to <c
 <references>
 
 <li>
-  TODO: REPLACE LINKS. Android Developers:
+  Android Developers:
+  <a href="https://developer.android.com/guide/topics/manifest/manifest-intro">App Manifest Overview</a>.
+</li>
+<li>
+  Android Developers:
   <a href="https://developer.android.com/guide/topics/manifest/application-element#debug">The android:debuggable attribute</a>.
 </li>
+<li>
+  Android Developers:
+  <a href="https://developer.android.com/studio/debug#enable-debug">Enable debugging</a>.
+</li>
 
 </references>
 </qhelp>

java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql

@@ -1,68 +1,18 @@
 /**
  * @name Implicitly exported Android component
- * @description TODO after more background reading
+ * @description An Android component with an '<intent-filter>' and no 'android:exported' attribute is implicitly exported. This can allow for improper access to the component and its data.
  * @kind problem
- * @problem.severity warning (TODO: confirm after more background reading)
- * @security-severity 0.1 (TODO: run script)
+ * @problem.severity warning
+ * @security-severity 8.2
  * @id java/android/implicitly-exported-component
  * @tags security
  *       external/cwe/cwe-926
- * @precision TODO after MRVA
+ * @precision high
  */
 
 import java
 import semmle.code.xml.AndroidManifest
 
-// FIRST DRAFT
-// from AndroidComponentXmlElement compElem
-// where
-//   not compElem.hasAttribute("exported") and
-//   compElem.getAChild().hasName("intent-filter") and
-//   not compElem.hasAttribute("permission") and
-//   not compElem
-//       .getAnIntentFilterElement()
-//       .getAnActionElement()
-//       .getActionName()
-//       .matches("android.intent.action.MAIN") and // filter out anything that is android intent (e.g. don't just filter out MAIN) because I think those are fine (but need to look at docs to confirm)
-//   //not compElem.getAnIntentFilterElement().getAnActionElement().getActionName() = "android.intent.category.LAUNCHER" and // I should add this as well, but above will techincally filter out since they always seem to occur together
-//   not compElem.getFile().getRelativePath().matches("%build%") // switch to not isInBuildDirectory() once new predicate is merged into main
-// select compElem, "This component is implicitly exported."
-// SECOND DRAFT
-// from AndroidComponentXmlElement compElem
-// where
-//   // Does NOT have `exported` attribute
-//   not compElem.hasAttribute("exported") and
-//   // and DOES have an intent-filter (DOUBLE-CHECK THIS CODE AND CHECK AGAINST OTHER VERSIONS THAT SEEMED TO WORK THE SAME)
-//   compElem.getAChild().hasName("intent-filter") and // compElem.getAChild("intent-filter"); need hasComponent with exists(...) here?
-//   // and does NOT have `permission` attribute
-//   not compElem.hasAttribute("permission") and
-//   // and is NOT in build directory (NOTE: switch to not isInBuildDirectory() once new predicate is merged into main)
-//   not compElem.getFile().getRelativePath().matches("%build%") and
-//   // and does NOT have a LAUNCHER category, see docs: https://developer.android.com/about/versions/12/behavior-changes-12#exported
-//   // Constant Value: "android.intent.category.LAUNCHER" from https://developer.android.com/reference/android/content/Intent#CATEGORY_LAUNCHER
-//   // I think beloew is actually too coarse because there can be multiple intent-filters in one component, so 2nd intent-filter without the launcher
-//   // could maybe be an issue, e.g. https://github.com/microsoft/DynamicsWOM/blob/62c2dad4cbbd4496a55aa3f644336044105bb1c1/app/src/main/AndroidManifest.xml#L56-L66
-//   not compElem.getAnIntentFilterElement().getAChild("category").getAttributeValue("name") =
-//     "android.intent.category.LAUNCHER" // double-check this code (especially use of getAChild and pattern match with LAUNCHER (e.g. should I do .%LAUNCHER instead?--No, constant value per docs), etc.), and definitely need to add stuff to library for this; should use exists(...) here?
-// select compElem, "This component is implicitly exported."
-// THIRD DRAFT
-from AndroidComponentXmlElement compElem, AndroidApplicationXmlElement appElem
-where
-  // Does NOT have `exported` attribute
-  not compElem.hasAttribute("exported") and
-  // and DOES have an intent-filter
-  compElem.getAChild().hasName("intent-filter") and // compElem.getAChild("intent-filter")
-  // and does NOT have `permission` attribute
-  not compElem.hasAttribute("permission") and
-  // and is NOT in build directory (NOTE: switch to not isInBuildDirectory() once new predicate is merged into main)
-  not compElem.getFile().getRelativePath().matches("%build%") and
-  // and does NOT have a LAUNCHER category, see docs: https://developer.android.com/about/versions/12/behavior-changes-12#exported
-  // Constant Value: "android.intent.category.LAUNCHER" from https://developer.android.com/reference/android/content/Intent#CATEGORY_LAUNCHER
-  // I think below is actually filtering out too much because there can be multiple intent-filters in one component, so 2nd intent-filter without the launcher
-  // could maybe be an issue, e.g. https://github.com/microsoft/DynamicsWOM/blob/62c2dad4cbbd4496a55aa3f644336044105bb1c1/app/src/main/AndroidManifest.xml#L56-L66
-  not compElem.getAnIntentFilterElement().getAChild("category").getAttributeValue("name") =
-    "android.intent.category.LAUNCHER" and
-  // and NO android:permission attribute in <application> element since that will be applied to the component even
-  // if no `permission` attribute directly set on component per the docs:
-  not appElem.hasAttribute("permission")
-select compElem, "This component is implicitly exported."
+from AndroidComponentXmlElement compElement
+where compElement.isImplicitlyExported()
+select compElement, "This component is implicitly exported."