From 3e09d86a4fecc9846f9d7fce1ad4e5bcf3277a30 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 1 Aug 2022 23:39:55 -0400 Subject: [PATCH 01/26] adding starter files --- .../ImplicitlyExportedAndroidComponent.qhelp | 38 +++++++++++++++++++ .../ImplicitlyExportedAndroidComponent.ql | 22 +++++++++++ .../2022-08-DD-android-implicit-export.md | 4 ++ ...citlyExportedAndroidComponentTest.expected | 0 .../ImplicitlyExportedAndroidComponentTest.ql | 23 +++++++++++ .../query-tests/security/CWE-926/Test.java | 3 ++ .../test/query-tests/security/CWE-926/options | 1 + 7 files changed, 91 insertions(+) create mode 100644 java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql create mode 100644 java/ql/src/change-notes/2022-08-DD-android-implicit-export.md create mode 100644 java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.expected create mode 100644 java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql create mode 100644 java/ql/test/query-tests/security/CWE-926/Test.java create mode 100644 java/ql/test/query-tests/security/CWE-926/options diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp new file mode 100644 index 00000000000..237c7dd0070 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp @@ -0,0 +1,38 @@ + + + + +

TODO: Replace the following +When a debugger is enabled it could allow for entry points in the application or reveal sensitive information.

+ + + + +

TODO: Replace the following +In Android applications either set the android:debuggable attribute to false +or do not include it in the manifest. The default value when not included is false.

+ + + + +

TODO: Replace the following +In the example below, the android:debuggable attribute is set to true.

+ + + +

The corrected version sets the android:debuggable attribute to false.

+ + + + + + +
  • + TODO: REPLACE LINKS. Android Developers: + The android:debuggable attribute. +
    • + +     +     diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql new file mode 100644 index 00000000000..be5c1742500 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql @@ -0,0 +1,22 @@ +/** + * @name Implicitly imported Android component + * @description TODO after more background reading + * @kind problem (TODO: confirm after more background reading) + * @problem.severity warning (TODO: confirm after more background reading) + * @security-severity 0.1 (TODO: run script) + * @id java/android/implicitly-imported-component + * @tags security + * external/cwe/cwe-926 + * @precision TODO after MRVA + */ + +import java +import semmle.code.xml.AndroidManifest + +// TODO: change query +from AndroidXmlAttribute androidXmlAttr +where + androidXmlAttr.getName() = "debuggable" and + androidXmlAttr.getValue() = "true" and + not androidXmlAttr.getLocation().getFile().getRelativePath().matches("%build%") +select androidXmlAttr, "The 'android:debuggable' attribute is enabled." diff --git a/java/ql/src/change-notes/2022-08-DD-android-implicit-export.md b/java/ql/src/change-notes/2022-08-DD-android-implicit-export.md new file mode 100644 index 00000000000..fa9252b8cfc --- /dev/null +++ b/java/ql/src/change-notes/2022-08-DD-android-implicit-export.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* Added a new query, `java/android/implicitly-imported-component`, to detect if an Android component can become implicitly exported. diff --git a/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.expected b/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql b/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql new file mode 100644 index 00000000000..fd723ac9548 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql @@ -0,0 +1,23 @@ +import java +import semmle.code.xml.AndroidManifest +import TestUtilities.InlineExpectationsTest + +// TODO: update for implicit export query +class DebuggableAttributeTrueTest extends InlineExpectationsTest { + DebuggableAttributeTrueTest() { this = "DebuggableAttributeEnabledTest" } + + override string getARelevantTag() { result = "hasDebuggableAttributeEnabled" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasDebuggableAttributeEnabled" and + exists(AndroidXmlAttribute androidXmlAttr | + androidXmlAttr.getName() = "debuggable" and + androidXmlAttr.getValue() = "true" and + not androidXmlAttr.getLocation().getFile().getRelativePath().matches("%build%") + | + androidXmlAttr.getLocation() = location and + element = androidXmlAttr.toString() and + value = "" + ) + } +} diff --git a/java/ql/test/query-tests/security/CWE-926/Test.java b/java/ql/test/query-tests/security/CWE-926/Test.java new file mode 100644 index 00000000000..4566fbca2ad --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-926/Test.java @@ -0,0 +1,3 @@ +public class Test { + +} diff --git a/java/ql/test/query-tests/security/CWE-926/options b/java/ql/test/query-tests/security/CWE-926/options new file mode 100644 index 00000000000..dacd3cb21df --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-926/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/google-android-9.0.0 From 8d5bbc458f137fc819416ae1625eb97bc9c81707 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Thu, 4 Aug 2022 09:58:10 -0400 Subject: [PATCH 02/26] first draft of query and tests --- .../ImplicitlyExportedAndroidComponent.ql | 24 ++++++++++------- .../security/CWE-926/AndroidManifest.xml | 26 +++++++++++++++++++ .../ImplicitlyExportedAndroidComponentTest.ql | 21 +++++++-------- 3 files changed, 51 insertions(+), 20 deletions(-) create mode 100644 java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql index be5c1742500..c1915d670e3 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql @@ -1,10 +1,10 @@ /** - * @name Implicitly imported Android component + * @name Implicitly exported Android component * @description TODO after more background reading - * @kind problem (TODO: confirm after more background reading) + * @kind problem * @problem.severity warning (TODO: confirm after more background reading) * @security-severity 0.1 (TODO: run script) - * @id java/android/implicitly-imported-component + * @id java/android/implicitly-exported-component * @tags security * external/cwe/cwe-926 * @precision TODO after MRVA @@ -13,10 +13,16 @@ import java import semmle.code.xml.AndroidManifest -// TODO: change query -from AndroidXmlAttribute androidXmlAttr +from AndroidComponentXmlElement compElem where - androidXmlAttr.getName() = "debuggable" and - androidXmlAttr.getValue() = "true" and - not androidXmlAttr.getLocation().getFile().getRelativePath().matches("%build%") -select androidXmlAttr, "The 'android:debuggable' attribute is enabled." + 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.%") 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." diff --git a/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml new file mode 100644 index 00000000000..7384880eec6 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml @@ -0,0 +1,26 @@ + + + + + + + + + + + + + + diff --git a/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql b/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql index fd723ac9548..1f63a048199 100644 --- a/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql +++ b/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql @@ -2,21 +2,20 @@ import java import semmle.code.xml.AndroidManifest import TestUtilities.InlineExpectationsTest -// TODO: update for implicit export query -class DebuggableAttributeTrueTest extends InlineExpectationsTest { - DebuggableAttributeTrueTest() { this = "DebuggableAttributeEnabledTest" } +class ImplicitlyExportedAndroidComponentTest extends InlineExpectationsTest { + ImplicitlyExportedAndroidComponentTest() { this = "ImplicitlyExportedAndroidComponentTest" } - override string getARelevantTag() { result = "hasDebuggableAttributeEnabled" } + override string getARelevantTag() { result = "hasImplicitExport" } override predicate hasActualResult(Location location, string element, string tag, string value) { - tag = "hasDebuggableAttributeEnabled" and - exists(AndroidXmlAttribute androidXmlAttr | - androidXmlAttr.getName() = "debuggable" and - androidXmlAttr.getValue() = "true" and - not androidXmlAttr.getLocation().getFile().getRelativePath().matches("%build%") + tag = "hasImplicitExport" and + exists(AndroidComponentXmlElement compElem, AndroidIntentFilterXmlElement intFiltElem | + not compElem.hasAttribute("exported") and + //compElem.getAnIntentFilterElement() instanceof AndroidIntentFilterXmlElement + not intFiltElem.getParent() = compElem | - androidXmlAttr.getLocation() = location and - element = androidXmlAttr.toString() and + compElem.getLocation() = location and + element = compElem.toString() and value = "" ) } From a6ecac6e0024ceaac78e41ba85f1b7f3e2d1708f Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Fri, 5 Aug 2022 01:16:59 -0400 Subject: [PATCH 03/26] third draft with category launcher and permission element excluded --- .../ImplicitlyExportedAndroidComponent.ql | 58 ++++++++++++++++--- .../security/CWE-926/AndroidManifest.xml | 11 +++- 2 files changed, 58 insertions(+), 11 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql index c1915d670e3..61882bdef14 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql @@ -13,16 +13,56 @@ import java import semmle.code.xml.AndroidManifest -from AndroidComponentXmlElement compElem +// 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, AndroidManifestXmlElement manifestElem where + // Does NOT have `exported` attribute not compElem.hasAttribute("exported") and - compElem.getAChild().hasName("intent-filter") 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 - not compElem - .getAnIntentFilterElement() - .getAnActionElement() - .getActionName() - .matches("android.intent.action.%") 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 + // 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 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 // 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? + // and NO element in manifest file since that will be applied to the component even if no `permission` attribute directly + // set on component per the docs: + not manifestElem.getAChild().hasName("permission") select compElem, "This component is implicitly exported." diff --git a/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml index 7384880eec6..f1b4cea98b9 100644 --- a/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml @@ -13,7 +13,7 @@ android:supportsRtl="true" android:theme="@style/Theme.HappyBirthday" tools:targetApi="31"> - @@ -21,6 +21,13 @@ - + + + + + + + From 60921a03559aeed29ce2a2c14bf0d1bdb999147d Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Fri, 5 Aug 2022 08:17:20 -0400 Subject: [PATCH 04/26] switched to checking for permission attr in application elem instead of in manifest elem --- .../ImplicitlyExportedAndroidComponent.ql | 16 ++++++++-------- .../security/CWE-926/AndroidManifest.xml | 1 + 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql index 61882bdef14..fef760e7328 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql @@ -46,23 +46,23 @@ import semmle.code.xml.AndroidManifest // "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, AndroidManifestXmlElement manifestElem +from AndroidComponentXmlElement compElem, AndroidApplicationXmlElement appElem 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 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 beloew is actually filtering out too much because there can be multiple intent-filters in one component, so 2nd intent-filter without the 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 // 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? - // and NO element in manifest file since that will be applied to the component even if no `permission` attribute directly - // set on component per the docs: - not manifestElem.getAChild().hasName("permission") + "android.intent.category.LAUNCHER" and + // and NO android:permission attribute in 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." diff --git a/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml index f1b4cea98b9..02b45fa4b61 100644 --- a/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml @@ -12,6 +12,7 @@ android:roundIcon="@mipmap/ic_launcher_round" android:supportsRtl="true" android:theme="@style/Theme.HappyBirthday" + android:permission="android.permission.SEND_SMS" tools:targetApi="31"> From eea1089ee0c5645aec5bdec9678aee2e8666a7ae Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 17 Aug 2022 14:19:50 -0400 Subject: [PATCH 05/26] resolved merge conflict in AndroidManifest --- .../lib/semmle/code/xml/AndroidManifest.qll | 60 +++++++++++++++++ .../ImplicitlyExportedAndroidComponent.qhelp | 26 ++++++-- .../ImplicitlyExportedAndroidComponent.ql | 64 ++----------------- 3 files changed, 86 insertions(+), 64 deletions(-) diff --git a/java/ql/lib/semmle/code/xml/AndroidManifest.qll b/java/ql/lib/semmle/code/xml/AndroidManifest.qll index 7d414cabe63..db361278775 100644 --- a/java/ql/lib/semmle/code/xml/AndroidManifest.qll +++ b/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 `` 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 `` child element of this `` element. */ AndroidActionXmlElement getAnActionElement() { result = this.getAChild() } + + /** + * Gets a `` child element of this `` element. + */ + AndroidCategoryXmlElement getACategoryElement() { result = this.getAChild("category") } + + /** + * Holds if this `` element has a `` 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 `` 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() + ) + } +} diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp index 237c7dd0070..8cac72b1267 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp @@ -4,21 +4,25 @@ -

    TODO: Replace the following -When a debugger is enabled it could allow for entry points in the application or reveal sensitive information.

    +

    The Android manifest file defines configuration settings for Android applications. +In this file, the android:debuggable attribute of the application element can be used to +define whether or not the application can be debugged. When set to true, this attribute will allow the +application to be debugged even when running on a device in user mode.

    + +

    When a debugger is enabled it could allow for entry points in the application or reveal sensitive information. +As a result, android:debuggable should only be enabled during development and should be disabled in +production builds.

     -

    TODO: Replace the following -In Android applications either set the android:debuggable attribute to false +

    In Android applications either set the android:debuggable attribute to false or do not include it in the manifest. The default value when not included is false.

     -

    TODO: Replace the following -In the example below, the android:debuggable attribute is set to true.

    +

    In the example below, the android:debuggable attribute is set to true.

    @@ -30,9 +34,17 @@ In the example below, the android:debuggable attribute is set to
  • - TODO: REPLACE LINKS. Android Developers: + Android Developers: + App Manifest Overview. +
    • +
  • + Android Developers: The android:debuggable attribute.
    • +
  • + Android Developers: + Enable debugging. +
    •  diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql index fef760e7328..af64b2b6461 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql +++ b/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 '' 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 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." From 10fa687e26008c5d4c39571a9afc92fe88becd12 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Fri, 5 Aug 2022 16:35:28 -0400 Subject: [PATCH 06/26] updated help file and unit tests --- .../src/Security/CWE/CWE-926/ExampleBad.xml | 11 ++++ .../src/Security/CWE/CWE-926/ExampleGood.xml | 12 +++++ .../ImplicitlyExportedAndroidComponent.qhelp | 33 +++++++----- .../security/CWE-926/AndroidManifest.xml | 21 +++++++- .../ImplicitlyExportedAndroidComponentTest.ql | 10 ++-- .../CWE-926/Testbuild/AndroidManifest.xml | 53 +++++++++++++++++++ 6 files changed, 118 insertions(+), 22 deletions(-) create mode 100644 java/ql/src/Security/CWE/CWE-926/ExampleBad.xml create mode 100644 java/ql/src/Security/CWE/CWE-926/ExampleGood.xml create mode 100644 java/ql/test/query-tests/security/CWE-926/Testbuild/AndroidManifest.xml diff --git a/java/ql/src/Security/CWE/CWE-926/ExampleBad.xml b/java/ql/src/Security/CWE/CWE-926/ExampleBad.xml new file mode 100644 index 00000000000..ec60406c460 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-926/ExampleBad.xml @@ -0,0 +1,11 @@ + + + + android:name=".Activity"> + + + + + + diff --git a/java/ql/src/Security/CWE/CWE-926/ExampleGood.xml b/java/ql/src/Security/CWE/CWE-926/ExampleGood.xml new file mode 100644 index 00000000000..f19184b8c13 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-926/ExampleGood.xml @@ -0,0 +1,12 @@ + + + + android:name=".Activity"> + android:exported="false" + + + + + + diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp index 8cac72b1267..8aa88304c09 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp @@ -5,30 +5,27 @@

    The Android manifest file defines configuration settings for Android applications. -In this file, the android:debuggable attribute of the application element can be used to -define whether or not the application can be debugged. When set to true, this attribute will allow the -application to be debugged even when running on a device in user mode.

    +In this file, components can be declared with intent filters which specify the types of intents the component can respond to. +If the android:exported attribute is omitted from the component when an intent filter is included, +then the component will be implicitly exported.

    -

    When a debugger is enabled it could allow for entry points in the application or reveal sensitive information. -As a result, android:debuggable should only be enabled during development and should be disabled in -production builds.

    +

    An implicitly exported component could allow for improper access to the component and its data.

     -

    In Android applications either set the android:debuggable attribute to false -or do not include it in the manifest. The default value when not included is false.

    +

    Explicitly set the android:exported attribute for every component or use permissions to limit access to the component.

     -

    In the example below, the android:debuggable attribute is set to true.

    +

    In the example below, the component android:exported attribute is omitted when an intent filter is used.

    - + -

    The corrected version sets the android:debuggable attribute to false.

    +

    A corrected version sets the android:exported attribute to false.

    - +     @@ -39,11 +36,19 @@ or do not include it in the manifest. The default value when not included is
  • Android Developers: - The android:debuggable attribute. + intent-filter-element.
  • Android Developers: - Enable debugging. + The android:exported attribute. +
    • +
  • + Android Developers: + The android:permission attribute. +
    • +
  • + Android Developers: + Safer component exporting.
    • diff --git a/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml index 02b45fa4b61..d1ecefda4a1 100644 --- a/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml @@ -12,7 +12,6 @@ android:roundIcon="@mipmap/ic_launcher_round" android:supportsRtl="true" android:theme="@style/Theme.HappyBirthday" - android:permission="android.permission.SEND_SMS" tools:targetApi="31"> @@ -29,6 +28,26 @@     + + + + + + + + + + + + + + +     diff --git a/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql b/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql index 1f63a048199..a8c9587ea49 100644 --- a/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql +++ b/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql @@ -9,13 +9,9 @@ class ImplicitlyExportedAndroidComponentTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasImplicitExport" and - exists(AndroidComponentXmlElement compElem, AndroidIntentFilterXmlElement intFiltElem | - not compElem.hasAttribute("exported") and - //compElem.getAnIntentFilterElement() instanceof AndroidIntentFilterXmlElement - not intFiltElem.getParent() = compElem - | - compElem.getLocation() = location and - element = compElem.toString() and + exists(AndroidComponentXmlElement compElement | compElement.isImplicitlyExported() | + compElement.getLocation() = location and + element = compElement.toString() and value = "" ) } diff --git a/java/ql/test/query-tests/security/CWE-926/Testbuild/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-926/Testbuild/AndroidManifest.xml new file mode 100644 index 00000000000..70896921295 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-926/Testbuild/AndroidManifest.xml @@ -0,0 +1,53 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 33c48ec685f1bfb161ae9bbdf58bd166865d53df Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 8 Aug 2022 13:31:59 -0400 Subject: [PATCH 07/26] updated change note --- .../ql/src/change-notes/2022-08-09-android-implicit-export.md | 4 ++++ .../ql/src/change-notes/2022-08-DD-android-implicit-export.md | 4 ---- 2 files changed, 4 insertions(+), 4 deletions(-) create mode 100644 java/ql/src/change-notes/2022-08-09-android-implicit-export.md delete mode 100644 java/ql/src/change-notes/2022-08-DD-android-implicit-export.md diff --git a/java/ql/src/change-notes/2022-08-09-android-implicit-export.md b/java/ql/src/change-notes/2022-08-09-android-implicit-export.md new file mode 100644 index 00000000000..31caced22f3 --- /dev/null +++ b/java/ql/src/change-notes/2022-08-09-android-implicit-export.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* Added a new query, `java/android/implicitly-exported-component`, to detect if Android components are implicitly exported in the Android manifest. diff --git a/java/ql/src/change-notes/2022-08-DD-android-implicit-export.md b/java/ql/src/change-notes/2022-08-DD-android-implicit-export.md deleted file mode 100644 index fa9252b8cfc..00000000000 --- a/java/ql/src/change-notes/2022-08-DD-android-implicit-export.md +++ /dev/null @@ -1,4 +0,0 @@ ---- -category: newQuery ---- -* Added a new query, `java/android/implicitly-imported-component`, to detect if an Android component can become implicitly exported. From a99d7ffaafb87e01f1a1f3d1b0c60b9895110d4c Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 8 Aug 2022 14:02:53 -0400 Subject: [PATCH 08/26] minor wording update in change note --- java/ql/src/change-notes/2022-08-09-android-implicit-export.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/change-notes/2022-08-09-android-implicit-export.md b/java/ql/src/change-notes/2022-08-09-android-implicit-export.md index 31caced22f3..beea9a8d3bf 100644 --- a/java/ql/src/change-notes/2022-08-09-android-implicit-export.md +++ b/java/ql/src/change-notes/2022-08-09-android-implicit-export.md @@ -1,4 +1,4 @@ --- category: newQuery --- -* Added a new query, `java/android/implicitly-exported-component`, to detect if Android components are implicitly exported in the Android manifest. +* Added a new query, `java/android/implicitly-exported-component`, to detect if components are implicitly exported in the Android manifest. From 55bd9f943fc9c49c035c99440daaa42e762663da Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 8 Aug 2022 14:53:54 -0400 Subject: [PATCH 09/26] minor wording updates in help file --- .../CWE-926/ImplicitlyExportedAndroidComponent.qhelp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp index 8aa88304c09..f9efb0d631b 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.qhelp @@ -5,9 +5,9 @@

    The Android manifest file defines configuration settings for Android applications. -In this file, components can be declared with intent filters which specify the types of intents the component can respond to. -If the android:exported attribute is omitted from the component when an intent filter is included, -then the component will be implicitly exported.

    +In this file, components can be declared with intent filters which specify what the components can do and what types +of intents the components can respond to. If the android:exported attribute is omitted from the component +when an intent filter is included, then the component will be implicitly exported.

    An implicitly exported component could allow for improper access to the component and its data.

    @@ -19,7 +19,7 @@ then the component will be implicitly exported.

    -

    In the example below, the component android:exported attribute is omitted when an intent filter is used.

    +

    In the example below, the android:exported attribute is omitted when an intent filter is used.

    @@ -36,7 +36,7 @@ then the component will be implicitly exported.

  • Android Developers: - intent-filter-element. + The <intent-filter> element.
  • Android Developers: From 084b9830bccfdba6e14bd397e6f23853cb5cb223 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 17 Aug 2022 14:27:01 -0400 Subject: [PATCH 10/26] resolved merge conflict in AndroidManifest --- java/ql/lib/semmle/code/xml/AndroidManifest.qll | 15 ++++++++------- .../CWE-926/ImplicitlyExportedAndroidComponent.ql | 2 +- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/java/ql/lib/semmle/code/xml/AndroidManifest.qll b/java/ql/lib/semmle/code/xml/AndroidManifest.qll index db361278775..1203698441d 100644 --- a/java/ql/lib/semmle/code/xml/AndroidManifest.qll +++ b/java/ql/lib/semmle/code/xml/AndroidManifest.qll @@ -68,8 +68,10 @@ class AndroidApplicationXmlElement extends XmlElement { ) } - /** Holds if this component element has an attribute with the name `permission`. */ - predicate hasPermissionAttribute() { exists(this.getAttribute("permission")) } + /** + * Holds if this application element has explicitly set a value for its `android:permission` attribute. + */ + predicate requiresPermissions() { this.getAnAttribute().(AndroidPermissionXmlAttribute).isFull() } } /** @@ -234,14 +236,13 @@ class AndroidComponentXmlElement extends XmlElement { */ predicate hasExportedAttribute() { this.hasAttribute("exported") } - /** Holds if this component element has an attribute with the name `permission`. */ - predicate hasPermissionAttribute() { exists(this.getAttribute("permission")) } - + // /** 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.hasAttribute("permission") and // not seeing how isFull() is any better than this..., this seems to more directly check what I want... + not this.getParent().(AndroidApplicationXmlElement).hasAttribute("permission") and not this.getAnIntentFilterElement().hasLauncherCategoryElement() and not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() } diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql index af64b2b6461..f68bc9a2556 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql @@ -7,7 +7,7 @@ * @id java/android/implicitly-exported-component * @tags security * external/cwe/cwe-926 - * @precision high + * @precision medium */ import java From 825df218a333caf30db3d8cd0e48cfe27abd6312 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Tue, 9 Aug 2022 14:57:13 -0400 Subject: [PATCH 11/26] adding library change note --- .../change-notes/2022-08-09-android-implicit-export.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 java/ql/lib/change-notes/2022-08-09-android-implicit-export.md diff --git a/java/ql/lib/change-notes/2022-08-09-android-implicit-export.md b/java/ql/lib/change-notes/2022-08-09-android-implicit-export.md new file mode 100644 index 00000000000..8e8684376d2 --- /dev/null +++ b/java/ql/lib/change-notes/2022-08-09-android-implicit-export.md @@ -0,0 +1,10 @@ +--- +category: feature +--- +* Added a new predicate, `hasAnIntentFilterElement`, in the `AndroidComponentXmlElement` class to detect if a component contains an intent filter element. +* Added a new predicate, `hasExportedAttribute`, in the `AndroidComponentXmlElement` class to detect if a component has an `android:exported` attribute. +* Added a new predicate, `isImplicitlyExported`, in the `AndroidComponentXmlElement` class to detect if a component is implicitly exported. +* Added a new predicate, `getACategoryElement`, in the `AndroidIntentFilterXmlElement` class to detect if an intent filter contains a category element. +* Added a new predicate, `hasLauncherCategoryElement`, in the `AndroidIntentFilterXmlElement` class to detect if an intent filter contains a launcher category element. +* Added a new class, `AndroidCategoryXmlElement`, to represent a category element in an Android manifest file. +* Added a new predicate, `getCategoryName`, in the `AndroidCategoryXmlElement` class to get the name of the category element. From 58d3d89b2ef480069bae5f992ac409a5f1220bc6 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 17 Aug 2022 14:29:15 -0400 Subject: [PATCH 12/26] resolved merge conflict in AndroidManifest --- .../ImplicitlyExportedAndroidComponent.qll | 15 ++++++++++++++ .../lib/semmle/code/xml/AndroidManifest.qll | 20 +++++++------------ .../ImplicitlyExportedAndroidComponent.ql | 8 ++++---- 3 files changed, 26 insertions(+), 17 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll diff --git a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll new file mode 100644 index 00000000000..171efdb2825 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll @@ -0,0 +1,15 @@ +/** Provides a class to reason about Android implicitly exported components. */ + +private import semmle.code.xml.AndroidManifest + +class ImplicitlyExportedAndroidComponent extends AndroidComponentXmlElement { + //ImplicitlyExportedAndroidComponent() { } + predicate isImplicitlyExported() { + not this.hasExportedAttribute() and + this.hasAnIntentFilterElement() and + not this.requiresPermissions() and + not this.getParent().(AndroidApplicationXmlElement).hasAttribute("permission") and + not this.getAnIntentFilterElement().hasLauncherCategoryElement() and + not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() + } +} diff --git a/java/ql/lib/semmle/code/xml/AndroidManifest.qll b/java/ql/lib/semmle/code/xml/AndroidManifest.qll index 1203698441d..08042eedb1d 100644 --- a/java/ql/lib/semmle/code/xml/AndroidManifest.qll +++ b/java/ql/lib/semmle/code/xml/AndroidManifest.qll @@ -113,7 +113,7 @@ class AndroidProviderXmlElement extends AndroidComponentXmlElement { * `android:permission` attribute or its `android:readPermission` and `android:writePermission` * attributes. */ - predicate requiresPermissions() { + override predicate requiresPermissions() { this.getAnAttribute().(AndroidPermissionXmlAttribute).isFull() or this.getAnAttribute().(AndroidPermissionXmlAttribute).isWrite() and @@ -236,16 +236,10 @@ class AndroidComponentXmlElement extends XmlElement { */ 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.hasAttribute("permission") and // not seeing how isFull() is any better than this..., this seems to more directly check what I want... - not this.getParent().(AndroidApplicationXmlElement).hasAttribute("permission") and - not this.getAnIntentFilterElement().hasLauncherCategoryElement() and - not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() - } + /** + * Holds if this component element has explicitly set a value for its `android:permission` attribute. + */ + predicate requiresPermissions() { this.getAnAttribute().(AndroidPermissionXmlAttribute).isFull() } } /** @@ -268,10 +262,10 @@ class AndroidIntentFilterXmlElement extends XmlElement { /** * Holds if this `` element has a `` child element - * named "android.intent.category.LAUNCHER". + * named `android.intent.category.LAUNCHER`. */ predicate hasLauncherCategoryElement() { - this.getACategoryElement().getAttributeValue("name") = "android.intent.category.LAUNCHER" + this.getACategoryElement().getCategoryName() = "android.intent.category.LAUNCHER" } } diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql index f68bc9a2556..eea7c3f5806 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql @@ -11,8 +11,8 @@ */ import java -import semmle.code.xml.AndroidManifest +import semmle.code.java.security.ImplicitlyExportedAndroidComponent -from AndroidComponentXmlElement compElement -where compElement.isImplicitlyExported() -select compElement, "This component is implicitly exported." +from ImplicitlyExportedAndroidComponent impExpAndroidComp +where impExpAndroidComp.isImplicitlyExported() +select impExpAndroidComp, "This component is implicitly exported." From b88d545c4948d0bc8c1664bd31bb05452738a9f1 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 10 Aug 2022 17:07:58 -0400 Subject: [PATCH 13/26] added unit tests --- .../security/CWE-926/AndroidManifest.xml | 94 +++++++++++++------ .../ImplicitlyExportedAndroidComponentTest.ql | 10 +- .../AndroidManifest.xml | 90 ++++++++++++++++++ .../CWE-926/Testbuild/AndroidManifest.xml | 94 +++++++++++++------ 4 files changed, 226 insertions(+), 62 deletions(-) create mode 100644 java/ql/test/query-tests/security/CWE-926/TestApplicationPermission/AndroidManifest.xml diff --git a/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml index d1ecefda4a1..9b4258baaf3 100644 --- a/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml @@ -12,9 +12,71 @@ android:roundIcon="@mipmap/ic_launcher_round" android:supportsRtl="true" android:theme="@style/Theme.HappyBirthday" - tools:targetApi="31"> - + tools:targetApi="31"> + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -22,32 +84,6 @@ - - - - - - - - - - - - - - - - - - - - diff --git a/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql b/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql index a8c9587ea49..155628108cc 100644 --- a/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql +++ b/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql @@ -1,5 +1,5 @@ import java -import semmle.code.xml.AndroidManifest +import semmle.code.java.security.ImplicitlyExportedAndroidComponent import TestUtilities.InlineExpectationsTest class ImplicitlyExportedAndroidComponentTest extends InlineExpectationsTest { @@ -9,9 +9,11 @@ class ImplicitlyExportedAndroidComponentTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasImplicitExport" and - exists(AndroidComponentXmlElement compElement | compElement.isImplicitlyExported() | - compElement.getLocation() = location and - element = compElement.toString() and + exists(ImplicitlyExportedAndroidComponent impExpAndroidComp | + impExpAndroidComp.isImplicitlyExported() + | + impExpAndroidComp.getLocation() = location and + element = impExpAndroidComp.toString() and value = "" ) } diff --git a/java/ql/test/query-tests/security/CWE-926/TestApplicationPermission/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-926/TestApplicationPermission/AndroidManifest.xml new file mode 100644 index 00000000000..ba7d970d3c0 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-926/TestApplicationPermission/AndroidManifest.xml @@ -0,0 +1,90 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/java/ql/test/query-tests/security/CWE-926/Testbuild/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-926/Testbuild/AndroidManifest.xml index 70896921295..2777e704cfc 100644 --- a/java/ql/test/query-tests/security/CWE-926/Testbuild/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-926/Testbuild/AndroidManifest.xml @@ -12,9 +12,71 @@ android:roundIcon="@mipmap/ic_launcher_round" android:supportsRtl="true" android:theme="@style/Theme.HappyBirthday" - tools:targetApi="31"> - + tools:targetApi="31"> + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @@ -22,32 +84,6 @@ - - - - - - - - - - - - - - - - - - - - From 115f76ac5a196b1b3feaa6ffa1fe7a260a92ff29 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 10 Aug 2022 20:57:50 -0400 Subject: [PATCH 14/26] fixed typo in unit tests; added a couple more tests --- .../security/CWE-926/AndroidManifest.xml | 22 +++++++++++++++++-- .../AndroidManifest.xml | 22 +++++++++++++++++-- .../CWE-926/Testbuild/AndroidManifest.xml | 22 +++++++++++++++++-- 3 files changed, 60 insertions(+), 6 deletions(-) diff --git a/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml index 9b4258baaf3..7a5b89ce48c 100644 --- a/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml @@ -43,7 +43,7 @@ - + - + + + + + + + + + + + + + + + diff --git a/java/ql/test/query-tests/security/CWE-926/TestApplicationPermission/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-926/TestApplicationPermission/AndroidManifest.xml index ba7d970d3c0..de229fb9be9 100644 --- a/java/ql/test/query-tests/security/CWE-926/TestApplicationPermission/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-926/TestApplicationPermission/AndroidManifest.xml @@ -44,7 +44,7 @@ - + - + + + + + + + + + + + + + + + diff --git a/java/ql/test/query-tests/security/CWE-926/Testbuild/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-926/Testbuild/AndroidManifest.xml index 2777e704cfc..336734a565f 100644 --- a/java/ql/test/query-tests/security/CWE-926/Testbuild/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-926/Testbuild/AndroidManifest.xml @@ -43,7 +43,7 @@ - + - + + + + + + + + + + + + + + + From 9968d5d816bb2bed8dc2d0dfab129c07a8fe04d6 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 10 Aug 2022 23:06:13 -0400 Subject: [PATCH 15/26] updated predicates --- .../ImplicitlyExportedAndroidComponent.qll | 15 +++++++++++++-- java/ql/lib/semmle/code/xml/AndroidManifest.qll | 2 +- .../CWE-926/ImplicitlyExportedAndroidComponent.ql | 3 +++ 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll index 171efdb2825..9445457f19f 100644 --- a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll +++ b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll @@ -1,9 +1,20 @@ -/** Provides a class to reason about Android implicitly exported components. */ +/** Provides a class to identify implicitly exported Android components. */ private import semmle.code.xml.AndroidManifest +/** Represents an implicitly exported Android component */ class ImplicitlyExportedAndroidComponent extends AndroidComponentXmlElement { - //ImplicitlyExportedAndroidComponent() { } + // ImplicitlyExportedAndroidComponent() { + // not this.hasExportedAttribute() and + // this.hasAnIntentFilterElement() and + // not this.requiresPermissions() and + // not this.getParent().(AndroidApplicationXmlElement).hasAttribute("permission") and + // not this.getAnIntentFilterElement().hasLauncherCategoryElement() and + // not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() + // } + /** + * Holds if this Android component is implicitly exported. + */ predicate isImplicitlyExported() { not this.hasExportedAttribute() and this.hasAnIntentFilterElement() and diff --git a/java/ql/lib/semmle/code/xml/AndroidManifest.qll b/java/ql/lib/semmle/code/xml/AndroidManifest.qll index 08042eedb1d..1af5eee722d 100644 --- a/java/ql/lib/semmle/code/xml/AndroidManifest.qll +++ b/java/ql/lib/semmle/code/xml/AndroidManifest.qll @@ -178,7 +178,7 @@ class AndroidComponentXmlElement extends XmlElement { /** * Holds if this component element has an `` child element. */ - predicate hasAnIntentFilterElement() { this.getAChild().hasName("intent-filter") } + predicate hasAnIntentFilterElement() { exists(this.getAnIntentFilterElement()) } /** * Gets the value of the `android:name` attribute of this component element. diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql index eea7c3f5806..09d8393f490 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql @@ -16,3 +16,6 @@ import semmle.code.java.security.ImplicitlyExportedAndroidComponent from ImplicitlyExportedAndroidComponent impExpAndroidComp where impExpAndroidComp.isImplicitlyExported() select impExpAndroidComp, "This component is implicitly exported." +// from ImplicitlyExportedAndroidComponent impExpAndroidComp +// where exists(impExpAndroidComp) +// select impExpAndroidComp, "This component is implicitly exported." From 0934c1d1840766b6992637ad233dbaff63dd9b11 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 22 Aug 2022 11:14:57 -0400 Subject: [PATCH 16/26] resolved merge conflict in AndroidManifest lib --- .../ImplicitlyExportedAndroidComponent.qll | 29 ++++++++++--------- .../lib/semmle/code/xml/AndroidManifest.qll | 24 ++++++++------- .../ImplicitlyExportedAndroidComponent.ql | 8 ++--- .../ImplicitlyExportedAndroidComponentTest.ql | 2 -- 4 files changed, 34 insertions(+), 29 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll index 9445457f19f..2b122619a3b 100644 --- a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll +++ b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll @@ -4,23 +4,26 @@ private import semmle.code.xml.AndroidManifest /** Represents an implicitly exported Android component */ class ImplicitlyExportedAndroidComponent extends AndroidComponentXmlElement { - // ImplicitlyExportedAndroidComponent() { + ImplicitlyExportedAndroidComponent() { + not this.hasExportedAttribute() and + this.hasAnIntentFilterElement() and + not this.getAnIntentFilterElement().getACategoryElement().getCategoryName() = + "android.intent.category.LAUNCHER" and + not this.requiresPermissions() and + not this.getParent().(AndroidApplicationXmlElement).requiresPermissions() and + //not this.getAnIntentFilterElement().hasLauncherCategoryElement() and + not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() + //this.getFile() instanceof SourceAndroidManifestXmlFile + } + // predicate isImplicitlyExported() { // not this.hasExportedAttribute() and // this.hasAnIntentFilterElement() and // not this.requiresPermissions() and // not this.getParent().(AndroidApplicationXmlElement).hasAttribute("permission") and // not this.getAnIntentFilterElement().hasLauncherCategoryElement() and - // not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() + // not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() //and + // not this.getAnIntentFilterElement().getAnActionElement().getActionName().matches("%MEDIA%") and // try MEDIA exclusion -- MRVA returns 251 results, so only removed 13 + // not this.getAnIntentFilterElement().getAnActionElement().getActionName() = + // "android.intent.action.MAIN" // try MAIN exclusion -- MRVA returns 193 results, so removed 251-193 = 58 results // } - /** - * Holds if this Android component is implicitly exported. - */ - predicate isImplicitlyExported() { - not this.hasExportedAttribute() and - this.hasAnIntentFilterElement() and - not this.requiresPermissions() and - not this.getParent().(AndroidApplicationXmlElement).hasAttribute("permission") and - not this.getAnIntentFilterElement().hasLauncherCategoryElement() and - not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() - } } diff --git a/java/ql/lib/semmle/code/xml/AndroidManifest.qll b/java/ql/lib/semmle/code/xml/AndroidManifest.qll index 1af5eee722d..fd2339d50da 100644 --- a/java/ql/lib/semmle/code/xml/AndroidManifest.qll +++ b/java/ql/lib/semmle/code/xml/AndroidManifest.qll @@ -25,6 +25,9 @@ class AndroidManifestXmlFile extends XmlFile { predicate isInBuildDirectory() { this.getFile().getRelativePath().matches("%build%") } } +// class SourceAndroidManifestXmlFile extends AndroidManifestXmlFile { +// SourceAndroidManifestXmlFile() { not this.getFile().getRelativePath().matches("%build%") } +// } /** * A `` element in an Android manifest file. */ @@ -139,6 +142,7 @@ class AndroidPermissionXmlAttribute extends XmlAttribute { AndroidPermissionXmlAttribute() { this.getNamespace().getPrefix() = "android" and this.getName() = ["permission", "readPermission", "writePermission"] + //this.getName() = ["permission"] } /** Holds if this is an `android:permission` attribute. */ @@ -234,7 +238,8 @@ class AndroidComponentXmlElement extends XmlElement { /** * Holds if this component element has an `android:exported` attribute. */ - predicate hasExportedAttribute() { this.hasAttribute("exported") } + //predicate hasExportedAttribute() { this.hasAttribute("exported") } + predicate hasExportedAttribute() { exists(this.getExportedAttributeValue()) } /** * Holds if this component element has explicitly set a value for its `android:permission` attribute. @@ -258,15 +263,14 @@ class AndroidIntentFilterXmlElement extends XmlElement { /** * Gets a `` child element of this `` element. */ - AndroidCategoryXmlElement getACategoryElement() { result = this.getAChild("category") } - - /** - * Holds if this `` element has a `` child element - * named `android.intent.category.LAUNCHER`. - */ - predicate hasLauncherCategoryElement() { - this.getACategoryElement().getCategoryName() = "android.intent.category.LAUNCHER" - } + AndroidCategoryXmlElement getACategoryElement() { result = this.getAChild() } + // /** + // * Holds if this `` element has a `` child element + // * named `android.intent.category.LAUNCHER`. + // */ + // predicate hasLauncherCategoryElement() { + // this.getACategoryElement().getCategoryName() = "android.intent.category.LAUNCHER" + // } } /** diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql index 09d8393f490..ad08139241d 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql @@ -13,9 +13,9 @@ import java import semmle.code.java.security.ImplicitlyExportedAndroidComponent -from ImplicitlyExportedAndroidComponent impExpAndroidComp -where impExpAndroidComp.isImplicitlyExported() -select impExpAndroidComp, "This component is implicitly exported." // from ImplicitlyExportedAndroidComponent impExpAndroidComp -// where exists(impExpAndroidComp) +// where impExpAndroidComp.isImplicitlyExported() // select impExpAndroidComp, "This component is implicitly exported." +from ImplicitlyExportedAndroidComponent impExpAndroidComp +//where exists(impExpAndroidComp) +select impExpAndroidComp, "This component is implicitly exported." diff --git a/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql b/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql index 155628108cc..fbda52d36ab 100644 --- a/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql +++ b/java/ql/test/query-tests/security/CWE-926/ImplicitlyExportedAndroidComponentTest.ql @@ -10,8 +10,6 @@ class ImplicitlyExportedAndroidComponentTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasImplicitExport" and exists(ImplicitlyExportedAndroidComponent impExpAndroidComp | - impExpAndroidComp.isImplicitlyExported() - | impExpAndroidComp.getLocation() = location and element = impExpAndroidComp.toString() and value = "" From eacce0307350aa67296bba1b71b8d99ffa49501b Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 22 Aug 2022 11:20:22 -0400 Subject: [PATCH 17/26] resolved merge conflict in AndroidManifest lib --- .../ImplicitlyExportedAndroidComponent.qll | 17 +++-------------- java/ql/lib/semmle/code/xml/AndroidManifest.qll | 11 ----------- .../ImplicitlyExportedAndroidComponent.ql | 4 ---- 3 files changed, 3 insertions(+), 29 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll index 2b122619a3b..8de2efb2be1 100644 --- a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll +++ b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll @@ -11,19 +11,8 @@ class ImplicitlyExportedAndroidComponent extends AndroidComponentXmlElement { "android.intent.category.LAUNCHER" and not this.requiresPermissions() and not this.getParent().(AndroidApplicationXmlElement).requiresPermissions() and - //not this.getAnIntentFilterElement().hasLauncherCategoryElement() and - not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() - //this.getFile() instanceof SourceAndroidManifestXmlFile + not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() //and + //not this.getAnIntentFilterElement().getAnActionElement().getActionName() = + // "android.intent.action.MAIN" } - // predicate isImplicitlyExported() { - // not this.hasExportedAttribute() and - // this.hasAnIntentFilterElement() and - // not this.requiresPermissions() and - // not this.getParent().(AndroidApplicationXmlElement).hasAttribute("permission") and - // not this.getAnIntentFilterElement().hasLauncherCategoryElement() and - // not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() //and - // not this.getAnIntentFilterElement().getAnActionElement().getActionName().matches("%MEDIA%") and // try MEDIA exclusion -- MRVA returns 251 results, so only removed 13 - // not this.getAnIntentFilterElement().getAnActionElement().getActionName() = - // "android.intent.action.MAIN" // try MAIN exclusion -- MRVA returns 193 results, so removed 251-193 = 58 results - // } } diff --git a/java/ql/lib/semmle/code/xml/AndroidManifest.qll b/java/ql/lib/semmle/code/xml/AndroidManifest.qll index fd2339d50da..0e1977d5f17 100644 --- a/java/ql/lib/semmle/code/xml/AndroidManifest.qll +++ b/java/ql/lib/semmle/code/xml/AndroidManifest.qll @@ -25,9 +25,6 @@ class AndroidManifestXmlFile extends XmlFile { predicate isInBuildDirectory() { this.getFile().getRelativePath().matches("%build%") } } -// class SourceAndroidManifestXmlFile extends AndroidManifestXmlFile { -// SourceAndroidManifestXmlFile() { not this.getFile().getRelativePath().matches("%build%") } -// } /** * A `` element in an Android manifest file. */ @@ -238,7 +235,6 @@ class AndroidComponentXmlElement extends XmlElement { /** * Holds if this component element has an `android:exported` attribute. */ - //predicate hasExportedAttribute() { this.hasAttribute("exported") } predicate hasExportedAttribute() { exists(this.getExportedAttributeValue()) } /** @@ -264,13 +260,6 @@ class AndroidIntentFilterXmlElement extends XmlElement { * Gets a `` child element of this `` element. */ AndroidCategoryXmlElement getACategoryElement() { result = this.getAChild() } - // /** - // * Holds if this `` element has a `` child element - // * named `android.intent.category.LAUNCHER`. - // */ - // predicate hasLauncherCategoryElement() { - // this.getACategoryElement().getCategoryName() = "android.intent.category.LAUNCHER" - // } } /** diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql index ad08139241d..00d5833b60a 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql @@ -13,9 +13,5 @@ import java import semmle.code.java.security.ImplicitlyExportedAndroidComponent -// from ImplicitlyExportedAndroidComponent impExpAndroidComp -// where impExpAndroidComp.isImplicitlyExported() -// select impExpAndroidComp, "This component is implicitly exported." from ImplicitlyExportedAndroidComponent impExpAndroidComp -//where exists(impExpAndroidComp) select impExpAndroidComp, "This component is implicitly exported." From fba9ffd49a7a45842dbc3322d923470605cb0bca Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Thu, 11 Aug 2022 21:43:26 -0400 Subject: [PATCH 18/26] update lib change note --- .../change-notes/2022-08-09-android-implicit-export.md | 10 ---------- ...-08-09-android-manifest-new-class-and-predicates.md | 8 ++++++++ 2 files changed, 8 insertions(+), 10 deletions(-) delete mode 100644 java/ql/lib/change-notes/2022-08-09-android-implicit-export.md create mode 100644 java/ql/lib/change-notes/2022-08-09-android-manifest-new-class-and-predicates.md diff --git a/java/ql/lib/change-notes/2022-08-09-android-implicit-export.md b/java/ql/lib/change-notes/2022-08-09-android-implicit-export.md deleted file mode 100644 index 8e8684376d2..00000000000 --- a/java/ql/lib/change-notes/2022-08-09-android-implicit-export.md +++ /dev/null @@ -1,10 +0,0 @@ ---- -category: feature ---- -* Added a new predicate, `hasAnIntentFilterElement`, in the `AndroidComponentXmlElement` class to detect if a component contains an intent filter element. -* Added a new predicate, `hasExportedAttribute`, in the `AndroidComponentXmlElement` class to detect if a component has an `android:exported` attribute. -* Added a new predicate, `isImplicitlyExported`, in the `AndroidComponentXmlElement` class to detect if a component is implicitly exported. -* Added a new predicate, `getACategoryElement`, in the `AndroidIntentFilterXmlElement` class to detect if an intent filter contains a category element. -* Added a new predicate, `hasLauncherCategoryElement`, in the `AndroidIntentFilterXmlElement` class to detect if an intent filter contains a launcher category element. -* Added a new class, `AndroidCategoryXmlElement`, to represent a category element in an Android manifest file. -* Added a new predicate, `getCategoryName`, in the `AndroidCategoryXmlElement` class to get the name of the category element. diff --git a/java/ql/lib/change-notes/2022-08-09-android-manifest-new-class-and-predicates.md b/java/ql/lib/change-notes/2022-08-09-android-manifest-new-class-and-predicates.md new file mode 100644 index 00000000000..27740fca13e --- /dev/null +++ b/java/ql/lib/change-notes/2022-08-09-android-manifest-new-class-and-predicates.md @@ -0,0 +1,8 @@ +--- +category: feature +--- +* Added a new predicate, `requiresPermissions`, in the `AndroidComponentXmlElement` and `AndroidApplicationXmlElement` classes to detect if the element has explicitly set a value for its `android:permission` attribute. Also added `override` annotation to the pre-existing `requiresPermissions` predicate in the `AndroidProviderXmlElement` class. +* Added a new predicate, `hasAnIntentFilterElement`, in the `AndroidComponentXmlElement` class to detect if a component contains an intent filter element. +* Added a new predicate, `hasExportedAttribute`, in the `AndroidComponentXmlElement` class to detect if a component has an `android:exported` attribute. +* Added a new class, `AndroidCategoryXmlElement`, to represent a category element in an Android manifest file. Also added a new predicate, `getCategoryName`, in this class to get the name of the category element. +* Added a new predicate, `getACategoryElement`, in the `AndroidIntentFilterXmlElement` class to get a category element of an intent filter. From c5526ffef8feaf82d8a6ae0d305af94e8c57a677 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Thu, 11 Aug 2022 21:48:48 -0400 Subject: [PATCH 19/26] update class QLDoc to start with 'An' --- .../code/java/security/ImplicitlyExportedAndroidComponent.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll index 8de2efb2be1..9d2d278c9be 100644 --- a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll +++ b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll @@ -2,7 +2,7 @@ private import semmle.code.xml.AndroidManifest -/** Represents an implicitly exported Android component */ +/** An implicitly exported Android component */ class ImplicitlyExportedAndroidComponent extends AndroidComponentXmlElement { ImplicitlyExportedAndroidComponent() { not this.hasExportedAttribute() and From eee12264c3e664b37104622f51ada4c3eacdd4ce Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Fri, 12 Aug 2022 09:22:39 -0400 Subject: [PATCH 20/26] excluded action main from query results, added unit test --- .../java/security/ImplicitlyExportedAndroidComponent.qll | 6 +++--- .../test/query-tests/security/CWE-926/AndroidManifest.xml | 7 +++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll index 9d2d278c9be..9b9f484678e 100644 --- a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll +++ b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll @@ -9,10 +9,10 @@ class ImplicitlyExportedAndroidComponent extends AndroidComponentXmlElement { this.hasAnIntentFilterElement() and not this.getAnIntentFilterElement().getACategoryElement().getCategoryName() = "android.intent.category.LAUNCHER" and + not this.getAnIntentFilterElement().getAnActionElement().getActionName() = + "android.intent.action.MAIN" and not this.requiresPermissions() and not this.getParent().(AndroidApplicationXmlElement).requiresPermissions() and - not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() //and - //not this.getAnIntentFilterElement().getAnActionElement().getActionName() = - // "android.intent.action.MAIN" + not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() } } diff --git a/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml index 7a5b89ce48c..210c97b26a2 100644 --- a/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml +++ b/java/ql/test/query-tests/security/CWE-926/AndroidManifest.xml @@ -102,6 +102,13 @@ + + + + + + From e003e2c809457d8c3f361128b31fa70240cbd6e7 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Fri, 12 Aug 2022 10:04:53 -0400 Subject: [PATCH 21/26] lib change note updates --- .../2022-08-09-android-manifest-new-class-and-predicates.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/change-notes/2022-08-09-android-manifest-new-class-and-predicates.md b/java/ql/lib/change-notes/2022-08-09-android-manifest-new-class-and-predicates.md index 27740fca13e..95a1d8997cb 100644 --- a/java/ql/lib/change-notes/2022-08-09-android-manifest-new-class-and-predicates.md +++ b/java/ql/lib/change-notes/2022-08-09-android-manifest-new-class-and-predicates.md @@ -1,8 +1,8 @@ --- category: feature --- -* Added a new predicate, `requiresPermissions`, in the `AndroidComponentXmlElement` and `AndroidApplicationXmlElement` classes to detect if the element has explicitly set a value for its `android:permission` attribute. Also added `override` annotation to the pre-existing `requiresPermissions` predicate in the `AndroidProviderXmlElement` class. +* Added a new predicate, `requiresPermissions`, in the `AndroidComponentXmlElement` and `AndroidApplicationXmlElement` classes to detect if the element has explicitly set a value for its `android:permission` attribute. * Added a new predicate, `hasAnIntentFilterElement`, in the `AndroidComponentXmlElement` class to detect if a component contains an intent filter element. * Added a new predicate, `hasExportedAttribute`, in the `AndroidComponentXmlElement` class to detect if a component has an `android:exported` attribute. -* Added a new class, `AndroidCategoryXmlElement`, to represent a category element in an Android manifest file. Also added a new predicate, `getCategoryName`, in this class to get the name of the category element. +* Added a new class, `AndroidCategoryXmlElement`, to represent a category element in an Android manifest file. * Added a new predicate, `getACategoryElement`, in the `AndroidIntentFilterXmlElement` class to get a category element of an intent filter. From efac4b197df395eb2b91820e33a8018d57ed4eb5 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Fri, 12 Aug 2022 12:16:49 -0400 Subject: [PATCH 22/26] removed another comment --- java/ql/lib/semmle/code/xml/AndroidManifest.qll | 1 - 1 file changed, 1 deletion(-) diff --git a/java/ql/lib/semmle/code/xml/AndroidManifest.qll b/java/ql/lib/semmle/code/xml/AndroidManifest.qll index 0e1977d5f17..1560e598b90 100644 --- a/java/ql/lib/semmle/code/xml/AndroidManifest.qll +++ b/java/ql/lib/semmle/code/xml/AndroidManifest.qll @@ -139,7 +139,6 @@ class AndroidPermissionXmlAttribute extends XmlAttribute { AndroidPermissionXmlAttribute() { this.getNamespace().getPrefix() = "android" and this.getName() = ["permission", "readPermission", "writePermission"] - //this.getName() = ["permission"] } /** Holds if this is an `android:permission` attribute. */ From ac07544d70f63170c958683c9a670aa9f89bc3c5 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Fri, 12 Aug 2022 12:25:43 -0400 Subject: [PATCH 23/26] group negated expressions together --- .../code/java/security/ImplicitlyExportedAndroidComponent.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll index 9b9f484678e..c38181d1a49 100644 --- a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll +++ b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll @@ -5,8 +5,8 @@ private import semmle.code.xml.AndroidManifest /** An implicitly exported Android component */ class ImplicitlyExportedAndroidComponent extends AndroidComponentXmlElement { ImplicitlyExportedAndroidComponent() { - not this.hasExportedAttribute() and this.hasAnIntentFilterElement() and + not this.hasExportedAttribute() and not this.getAnIntentFilterElement().getACategoryElement().getCategoryName() = "android.intent.category.LAUNCHER" and not this.getAnIntentFilterElement().getAnActionElement().getActionName() = From f34e23bdbacf90abdd8236ba6482f2be2d9c0367 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Wed, 17 Aug 2022 14:07:10 -0400 Subject: [PATCH 24/26] adjusted comments and precision level --- .../security/ImplicitlyExportedAndroidComponent.qll | 11 ++++++++++- .../CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll index c38181d1a49..4aa21c4a260 100644 --- a/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll +++ b/java/ql/lib/semmle/code/java/security/ImplicitlyExportedAndroidComponent.qll @@ -2,15 +2,24 @@ private import semmle.code.xml.AndroidManifest -/** An implicitly exported Android component */ +/** + * An Android component without an `exported` attribute explicitly set + * that also has an `intent-filter` element. + */ class ImplicitlyExportedAndroidComponent extends AndroidComponentXmlElement { ImplicitlyExportedAndroidComponent() { this.hasAnIntentFilterElement() and not this.hasExportedAttribute() and + // Components with category LAUNCHER or with action MAIN need to be exported since + // they are entry-points to the application. As a result, we do not consider these + // components to be implicitly exported since the developer intends them to be exported anyways. not this.getAnIntentFilterElement().getACategoryElement().getCategoryName() = "android.intent.category.LAUNCHER" and not this.getAnIntentFilterElement().getAnActionElement().getActionName() = "android.intent.action.MAIN" and + // The `permission` attribute can be used to limit components' exposure to other applications. + // As a result, we do not consider components with an explicitly set `permission` attribute to be + // implicitly exported since the developer has already limited access to such components. not this.requiresPermissions() and not this.getParent().(AndroidApplicationXmlElement).requiresPermissions() and not this.getFile().(AndroidManifestXmlFile).isInBuildDirectory() diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql index 00d5833b60a..eb12d13b060 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql @@ -7,7 +7,7 @@ * @id java/android/implicitly-exported-component * @tags security * external/cwe/cwe-926 - * @precision medium + * @precision high */ import java From 733078183ed3695edb5ea5060b49cbe25fe7626d Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 22 Aug 2022 11:08:42 -0400 Subject: [PATCH 25/26] update query description --- .../Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql index eb12d13b060..9796a3f59e2 100644 --- a/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql +++ b/java/ql/src/Security/CWE/CWE-926/ImplicitlyExportedAndroidComponent.ql @@ -1,6 +1,6 @@ /** * @name Implicitly exported Android component - * @description An Android component with an '' and no 'android:exported' attribute is implicitly exported. This can allow for improper access to the component and its data. + * @description Android components with an '' and no 'android:exported' attribute are implicitly exported, which can allow for improper access to the components themselves and to their data. * @kind problem * @problem.severity warning * @security-severity 8.2 From 0136c7542b5521081c492cda9445d6d7d0de1281 Mon Sep 17 00:00:00 2001 From: Jami Cogswell Date: Mon, 22 Aug 2022 12:39:43 -0400 Subject: [PATCH 26/26] update XML to Xml due to recent deprecation --- java/ql/lib/semmle/code/xml/AndroidManifest.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/semmle/code/xml/AndroidManifest.qll b/java/ql/lib/semmle/code/xml/AndroidManifest.qll index 1560e598b90..b5c79efa853 100644 --- a/java/ql/lib/semmle/code/xml/AndroidManifest.qll +++ b/java/ql/lib/semmle/code/xml/AndroidManifest.qll @@ -286,7 +286,7 @@ class AndroidActionXmlElement extends XmlElement { /** * A `` element in an Android manifest file. */ -class AndroidCategoryXmlElement extends XMLElement { +class AndroidCategoryXmlElement extends XmlElement { AndroidCategoryXmlElement() { this.getFile() instanceof AndroidManifestXmlFile and this.getName() = "category" } @@ -295,7 +295,7 @@ class AndroidCategoryXmlElement extends XMLElement { * Gets the name of this category. */ string getCategoryName() { - exists(XMLAttribute attr | + exists(XmlAttribute attr | attr = this.getAnAttribute() and attr.getNamespace().getPrefix() = "android" and attr.getName() = "name"