Add security detector for oversharing content providers

Look for content providers which use a grant-uri-permission element
and share everything ( / ).

Also renames the ExportedServiceDetector to SecurityDetector since a
single manifest scanner is used for both (and potentially more in the
future.)

Change-Id: I1e4ac0409c524e2655f8a389fa18162a94388b29

5 files changed, 100 insertions, 10 deletions

diff --git a/lint/libs/lint_api/src/com/android/tools/lint/detector/api/LintConstants.java b/lint/libs/lint_api/src/com/android/tools/lint/detector/api/LintConstants.java
index f0fb685..81de380 100644
--- a/lint/libs/lint_api/src/com/android/tools/lint/detector/api/LintConstants.java
+++ b/lint/libs/lint_api/src/com/android/tools/lint/detector/api/LintConstants.java
@@ -33,6 +33,7 @@ public class LintConstants {
     public static final String TAG_INTENT_FILTER = " intent-filter";   //$NON-NLS-1$
     public static final String TAG_USES_SDK = "uses-sdk";              //$NON-NLS-1$
     public static final String TAG_ACTIVITY = "activity";              //$NON-NLS-1$
+    public static final String TAG_GRANT_PERMISSION = "grant-uri-permission"; //$NON-NLS-1$
 
     // Tags: Resources
     public static final String TAG_RESOURCES = "resources";            //$NON-NLS-1$
@@ -75,6 +76,9 @@ public class LintConstants {
     public static final String ATTR_ICON = "icon";                     //$NON-NLS-1$
     public final static String ATTR_PACKAGE = "package";               //$NON-NLS-1$
     public static final String ATTR_THEME = "theme";                   //$NON-NLS-1$
+    public static final String ATTR_PATH = "path";                     //$NON-NLS-1$
+    public static final String ATTR_PATH_PREFIX = "pathPrefix";        //$NON-NLS-1$
+    public static final String ATTR_PATH_PATTERN = "pathPattern";      //$NON-NLS-1$
 
     // Attributes: Resources
     public static final String ATTR_NAME = "name";                     //$NON-NLS-1$
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/BuiltinIssueRegistry.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/BuiltinIssueRegistry.java
index 2c4af67..8522ea4 100644
--- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/BuiltinIssueRegistry.java
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/BuiltinIssueRegistry.java
@@ -83,7 +83,8 @@ public class BuiltinIssueRegistry extends IssueRegistry {
         issues.add(UnusedResourceDetector.ISSUE_IDS);
         issues.add(ArraySizeDetector.INCONSISTENT);
         issues.add(ManifestOrderDetector.ISSUE);
-        issues.add(ExportedServiceDetector.ISSUE);
+        issues.add(SecurityDetector.EXPORTED_SERVICE);
+        issues.add(SecurityDetector.OPEN_PROVIDER);
         issues.add(IconDetector.GIF_USAGE);
         issues.add(IconDetector.ICON_DENSITIES);
         issues.add(IconDetector.ICON_MISSING_FOLDER);
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/ExportedServiceDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/SecurityDetector.java
index 437256c..ef76e51 100644
--- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/ExportedServiceDetector.java
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/SecurityDetector.java
@@ -19,8 +19,12 @@ package com.android.tools.lint.checks;
 import static com.android.tools.lint.detector.api.LintConstants.ANDROID_MANIFEST_XML;
 import static com.android.tools.lint.detector.api.LintConstants.ANDROID_URI;
 import static com.android.tools.lint.detector.api.LintConstants.ATTR_EXPORTED;
+import static com.android.tools.lint.detector.api.LintConstants.ATTR_PATH;
+import static com.android.tools.lint.detector.api.LintConstants.ATTR_PATH_PATTERN;
+import static com.android.tools.lint.detector.api.LintConstants.ATTR_PATH_PREFIX;
 import static com.android.tools.lint.detector.api.LintConstants.ATTR_PERMISSION;
 import static com.android.tools.lint.detector.api.LintConstants.TAG_APPLICATION;
+import static com.android.tools.lint.detector.api.LintConstants.TAG_GRANT_PERMISSION;
 import static com.android.tools.lint.detector.api.LintConstants.TAG_INTENT_FILTER;
 import static com.android.tools.lint.detector.api.LintConstants.TAG_SERVICE;
 
@@ -33,6 +37,7 @@ import com.android.tools.lint.detector.api.Scope;
 import com.android.tools.lint.detector.api.Severity;
 import com.android.tools.lint.detector.api.Speed;
 
+import org.w3c.dom.Attr;
 import org.w3c.dom.Element;
 import org.w3c.dom.Node;
 
@@ -44,10 +49,10 @@ import java.util.EnumSet;
 /**
  * Checks that exported services request a permission.
  */
-public class ExportedServiceDetector extends Detector.XmlDetectorAdapter {
+public class SecurityDetector extends Detector.XmlDetectorAdapter {
 
-    /** The main issue discovered by this detector */
-    public static final Issue ISSUE = Issue.create(
+    /** Exported services */
+    public static final Issue EXPORTED_SERVICE = Issue.create(
             "ExportedService", //$NON-NLS-1$
             "Checks for exported services that do not require permissions",
             "Exported services (services which either set exported=true or contain " +
@@ -57,11 +62,24 @@ public class ExportedServiceDetector extends Detector.XmlDetectorAdapter {
             Category.SECURITY,
             5,
             Severity.WARNING,
-            ExportedServiceDetector.class,
+            SecurityDetector.class,
+            EnumSet.of(Scope.MANIFEST));
+
+    /** Content provides which grant all URIs access */
+    public static final Issue OPEN_PROVIDER = Issue.create(
+            "GrantAllUris", //$NON-NLS-1$
+            "Checks for <grant-uri-permission> elements where everything is shared",
+            "The <grant-uri-permission> element allows specific paths to be shared. " +
+            "This detector checks for a path URL of just '/' (everything), which is " +
+            "probably not what you want; you should limit access to a subset.",
+            Category.SECURITY,
+            7,
+            Severity.WARNING,
+            SecurityDetector.class,
             EnumSet.of(Scope.MANIFEST));
 
     /** Constructs a new accessibility check */
-    public ExportedServiceDetector() {
+    public SecurityDetector() {
     }
 
     @Override
@@ -79,13 +97,21 @@ public class ExportedServiceDetector extends Detector.XmlDetectorAdapter {
     public Collection<String> getApplicableElements() {
         return Arrays.asList(new String[] {
             TAG_SERVICE,
+            TAG_GRANT_PERMISSION
         });
     }
 
     @Override
     public void visitElement(Context context, Element element) {
-        assert element.getTagName().equals(TAG_SERVICE);
+        String tag = element.getTagName();
+        if (tag.equals(TAG_SERVICE)) {
+            checkService(context, element);
+        } else if (tag.equals(TAG_GRANT_PERMISSION)) {
+            checkGrantPermission(context, element);
+        }
+    }
 
+    private void checkService(Context context, Element element) {
         String exportValue = element.getAttributeNS(ANDROID_URI, ATTR_EXPORTED);
         boolean exported;
         if (exportValue != null && exportValue.length() > 0) {
@@ -112,7 +138,7 @@ public class ExportedServiceDetector extends Detector.XmlDetectorAdapter {
                     permission = application.getAttributeNS(ANDROID_URI, ATTR_PERMISSION);
                     if (permission == null || permission.length() == 0) {
                         // No declared permission for this exported service: complain
-                        context.client.report(context, ISSUE,
+                        context.client.report(context, EXPORTED_SERVICE,
                             context.getLocation(element),
                             "Exported service does not require permission", null);
                     }
@@ -120,4 +146,22 @@ public class ExportedServiceDetector extends Detector.XmlDetectorAdapter {
             }
         }
     }
+
+    private void checkGrantPermission(Context context, Element element) {
+        Attr path = element.getAttributeNodeNS(ANDROID_URI, ATTR_PATH);
+        Attr prefix = element.getAttributeNodeNS(ANDROID_URI, ATTR_PATH_PREFIX);
+        Attr pattern = element.getAttributeNodeNS(ANDROID_URI, ATTR_PATH_PATTERN);
+
+        String msg = "Content provider shares everything; this is potentially dangerous.";
+        if (path != null && path.getValue().equals("/")) { //$NON-NLS-1$
+            context.client.report(context, OPEN_PROVIDER, context.getLocation(path), msg, null);
+        }
+        if (prefix != null && prefix.getValue().equals("/")) { //$NON-NLS-1$
+            context.client.report(context, OPEN_PROVIDER, context.getLocation(prefix), msg, null);
+        }
+        if (pattern != null && (pattern.getValue().equals("/")  //$NON-NLS-1$
+               /* || pattern.getValue().equals(".*")*/)) {
+            context.client.report(context, OPEN_PROVIDER, context.getLocation(pattern), msg, null);
+        }
+    }
 }
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/ExportedServiceDetectorTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/SecurityDetectorTest.java
index 839c199..d50fe1b 100644
--- a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/ExportedServiceDetectorTest.java
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/SecurityDetectorTest.java
@@ -19,10 +19,10 @@ package com.android.tools.lint.checks;
 import com.android.tools.lint.detector.api.Detector;
 
 @SuppressWarnings("javadoc")
-public class ExportedServiceDetectorTest extends AbstractCheckTest {
+public class SecurityDetectorTest extends AbstractCheckTest {
     @Override
     protected Detector getDetector() {
-        return new ExportedServiceDetector();
+        return new SecurityDetector();
     }
 
     public void testBroken() throws Exception {
@@ -58,4 +58,16 @@ public class ExportedServiceDetectorTest extends AbstractCheckTest {
                     "exportservice3.xml=>AndroidManifest.xml",
                     "res/values/strings.xml"));
     }
+
+    public void testUri() throws Exception {
+        assertEquals(
+            "AndroidManifest.xml:24: Warning: Content provider shares everything; this is potentially dangerous.\n" +
+            "AndroidManifest.xml:25: Warning: Content provider shares everything; this is potentially dangerous.",
+
+            lintProject(
+                    "grantpermission.xml=>AndroidManifest.xml",
+                    "res/values/strings.xml"));
+    }
+
+
 }
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/grantpermission.xml b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/grantpermission.xml
new file mode 100644
index 0000000..b75f712
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/grantpermission.xml
@@ -0,0 +1,29 @@
+<?xml version="1.0" encoding="utf-8"?>
+<manifest xmlns:android="http://schemas.android.com/apk/res/android"
+    package="foo.bar2"
+    android:versionCode="1"
+    android:versionName="1.0" >
+
+    <uses-sdk android:minSdkVersion="14" />
+
+    <application
+        android:icon="@drawable/ic_launcher"
+        android:label="@string/app_name" >
+        <activity
+            android:label="@string/app_name"
+            android:name=".Foo2Activity" >
+            <intent-filter >
+                <action android:name="android.intent.action.MAIN" />
+
+                <category android:name="android.intent.category.LAUNCHER" />
+            </intent-filter>
+        </activity>
+        <!-- good: -->
+        <grant-uri-permission android:pathPrefix="/all_downloads/"/>
+        <!-- bad: -->
+        <grant-uri-permission android:path="/"/>
+        <grant-uri-permission android:pathPrefix="/"/>
+        <grant-uri-permission android:pathPattern=".*"/>
+    </application>
+
+</manifest>