From ab035b633d3bb74047364973f326c33a42c8891c Mon Sep 17 00:00:00 2001 From: Tor Norbye Date: Fri, 21 Sep 2012 06:55:11 -0700 Subject: Add allowBackup lint security check Change-Id: I3b79bef6981d880fe6a545429754e03bd384645c --- common/src/com/android/SdkConstants.java | 1 + .../ide/eclipse/adt/internal/lint/LintFix.java | 2 ++ .../eclipse/adt/internal/lint/SetAttributeFix.java | 10 ++++-- .../tools/lint/checks/BuiltinIssueRegistry.java | 3 +- .../tools/lint/checks/ManifestOrderDetector.java | 42 ++++++++++++++++++++++ .../lint/checks/ManifestOrderDetectorTest.java | 33 +++++++++++++++++ .../android/tools/lint/checks/data/allowbackup.xml | 24 +++++++++++++ .../tools/lint/checks/data/allowbackup_ignore.xml | 25 +++++++++++++ 8 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/allowbackup.xml create mode 100644 lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/allowbackup_ignore.xml diff --git a/common/src/com/android/SdkConstants.java b/common/src/com/android/SdkConstants.java index 1aa3853..b20d15f 100644 --- a/common/src/com/android/SdkConstants.java +++ b/common/src/com/android/SdkConstants.java @@ -683,6 +683,7 @@ public final class SdkConstants { 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$ + public static final String ATTR_ALLOW_BACKUP = "allowBackup"; //$NON_NLS-1$ public static final String ATTR_DEBUGGABLE = "debuggable"; //$NON-NLS-1$ public static final String ATTR_READ_PERMISSION = "readPermission"; //$NON_NLS-1$ public static final String ATTR_WRITE_PERMISSION = "writePermission"; //$NON_NLS-1$ diff --git a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/lint/LintFix.java b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/lint/LintFix.java index c02c303..7683f8d 100644 --- a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/lint/LintFix.java +++ b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/lint/LintFix.java @@ -23,6 +23,7 @@ import com.android.tools.lint.checks.AccessibilityDetector; import com.android.tools.lint.checks.DetectMissingPrefix; import com.android.tools.lint.checks.HardcodedValuesDetector; import com.android.tools.lint.checks.InefficientWeightDetector; +import com.android.tools.lint.checks.ManifestOrderDetector; import com.android.tools.lint.checks.ObsoleteLayoutParamsDetector; import com.android.tools.lint.checks.PxUsageDetector; import com.android.tools.lint.checks.ScrollViewChildDetector; @@ -148,6 +149,7 @@ abstract class LintFix implements ICompletionProposal { LinearLayoutWeightFix.class); sFixes.put(AccessibilityDetector.ISSUE.getId(), SetAttributeFix.class); sFixes.put(InefficientWeightDetector.BASELINE_WEIGHTS.getId(), SetAttributeFix.class); + sFixes.put(ManifestOrderDetector.ALLOW_BACKUP.getId(), SetAttributeFix.class); sFixes.put(HardcodedValuesDetector.ISSUE.getId(), ExtractStringFix.class); sFixes.put(UselessViewDetector.USELESS_LEAF.getId(), RemoveUselessViewFix.class); sFixes.put(UselessViewDetector.USELESS_PARENT.getId(), RemoveUselessViewFix.class); diff --git a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/lint/SetAttributeFix.java b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/lint/SetAttributeFix.java index 4ed5e05..1d743b8 100644 --- a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/lint/SetAttributeFix.java +++ b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/lint/SetAttributeFix.java @@ -15,6 +15,7 @@ */ package com.android.ide.eclipse.adt.internal.lint; +import static com.android.SdkConstants.ATTR_ALLOW_BACKUP; import static com.android.SdkConstants.ATTR_BASELINE_ALIGNED; import static com.android.SdkConstants.ATTR_CONTENT_DESCRIPTION; import static com.android.SdkConstants.ATTR_INPUT_TYPE; @@ -24,6 +25,7 @@ import static com.android.SdkConstants.VALUE_FALSE; import com.android.tools.lint.checks.AccessibilityDetector; import com.android.tools.lint.checks.InefficientWeightDetector; +import com.android.tools.lint.checks.ManifestOrderDetector; import com.android.tools.lint.checks.SecurityDetector; import com.android.tools.lint.checks.TextFieldDetector; import com.android.tools.lint.checks.TranslationDetector; @@ -48,6 +50,8 @@ final class SetAttributeFix extends SetPropertyFix { return ATTR_INPUT_TYPE; } else if (mId.equals(TranslationDetector.MISSING.getId())) { return ATTR_TRANSLATABLE; + } else if (mId.equals(ManifestOrderDetector.ALLOW_BACKUP.getId())) { + return ATTR_ALLOW_BACKUP; } else { assert false : mId; return ""; @@ -75,6 +79,8 @@ final class SetAttributeFix extends SetPropertyFix { return "Add permission attribute"; } else if (mId.equals(TranslationDetector.MISSING.getId())) { return "Mark this as a non-translatable resource"; + } else if (mId.equals(ManifestOrderDetector.ALLOW_BACKUP.getId())) { + return "Set the allowBackup attribute to true or false"; } else { assert false : mId; return ""; @@ -95,7 +101,8 @@ final class SetAttributeFix extends SetPropertyFix { @Override protected boolean invokeCodeCompletion() { return mId.equals(SecurityDetector.EXPORTED_SERVICE.getId()) - || mId.equals(TextFieldDetector.ISSUE.getId()); + || mId.equals(TextFieldDetector.ISSUE.getId()) + || mId.equals(ManifestOrderDetector.ALLOW_BACKUP.getId()); } @Override @@ -117,5 +124,4 @@ final class SetAttributeFix extends SetPropertyFix { return super.getProposal(); } - } 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 c5e4e64..476ebf9 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 @@ -55,7 +55,7 @@ public class BuiltinIssueRegistry extends IssueRegistry { private static final List sIssues; static { - final int initialCapacity = 110; + final int initialCapacity = 111; List issues = new ArrayList(initialCapacity); issues.add(AccessibilityDetector.ISSUE); @@ -118,6 +118,7 @@ public class BuiltinIssueRegistry extends IssueRegistry { issues.add(ManifestOrderDetector.WRONG_PARENT); issues.add(ManifestOrderDetector.DUPLICATE_ACTIVITY); issues.add(ManifestOrderDetector.TARGET_NEWER); + issues.add(ManifestOrderDetector.ALLOW_BACKUP); issues.add(SecurityDetector.EXPORTED_PROVIDER); issues.add(SecurityDetector.EXPORTED_SERVICE); issues.add(SecurityDetector.EXPORTED_ACTIVITY); diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/ManifestOrderDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/ManifestOrderDetector.java index 237d8a0..c95b12d 100644 --- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/ManifestOrderDetector.java +++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/ManifestOrderDetector.java @@ -31,6 +31,7 @@ import static com.android.SdkConstants.TAG_USES_LIBRARY; import static com.android.SdkConstants.TAG_USES_PERMISSION; import static com.android.SdkConstants.TAG_USES_SDK; +import com.android.SdkConstants; import com.android.annotations.NonNull; import com.android.tools.lint.detector.api.Category; import com.android.tools.lint.detector.api.Context; @@ -168,6 +169,40 @@ public class ManifestOrderDetector extends Detector implements Detector.XmlScann ManifestOrderDetector.class, EnumSet.of(Scope.MANIFEST)); + /** Not explicitly defining allowBackup */ + public static final Issue ALLOW_BACKUP = Issue.create( + "AllowBackup", //$NON-NLS-1$ + "Ensure that allowBackup is explicitly set in the application's manifest", + + "The allowBackup attribute determines if an application's data can be backed up " + + "and restored. It is documented at " + + "http://developer.android.com/reference/android/R.attr.html#allowBackup\n" + + "\n" + + "By default, this flag is set to `true`. When this flag is set to `true`, " + + "application data can be backed up and restored by the user using `adb backup` " + + "and `adb restore`.\n" + + "\n" + + "This may have security consequences for an application. `adb backup` allows " + + "users who have enabled USB debugging to copy application data off of the " + + "device. Once backed up, all application data can be read by the user. " + + "`adb restore` allows creation of application data from a source specified " + + "by the user. Following a restore, applications should not assume that the " + + "data, file permissions, and directory permissions were created by the " + + "application itself.\n" + + "\n" + + "Setting `allowBackup=\"false\"` opts an application out of both backup and " + + "restore.\n" + + "\n" + + "To fix this warning, decide whether your application should support backup, " + + "and explicitly set `android:allowBackup=(true|false)\"`", + + Category.SECURITY, + 3, + Severity.WARNING, + ManifestOrderDetector.class, + EnumSet.of(Scope.MANIFEST)).setMoreInfo( + "http://developer.android.com/reference/android/R.attr.html#allowBackup"); + /** Constructs a new {@link ManifestOrderDetector} check */ public ManifestOrderDetector() { } @@ -347,6 +382,13 @@ public class ManifestOrderDetector extends Detector implements Detector.XmlScann if (tag.equals(TAG_APPLICATION)) { mSeenApplication = true; + if (!element.hasAttributeNS(ANDROID_URI, SdkConstants.ATTR_ALLOW_BACKUP) + && context.isEnabled(ALLOW_BACKUP)) { + context.report(ALLOW_BACKUP, element, context.getLocation(element), + String.format("Should explicitly set android:allowBackup to true or " + + "false (it's true by default, and that can have some security " + + "implications for the application's data)", tag), null); + } } else if (mSeenApplication) { if (context.isEnabled(ORDER)) { context.report(ORDER, element, context.getLocation(element), diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/ManifestOrderDetectorTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/ManifestOrderDetectorTest.java index 7513138..8d49bfa 100644 --- a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/ManifestOrderDetectorTest.java +++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/ManifestOrderDetectorTest.java @@ -199,4 +199,37 @@ public class ManifestOrderDetectorTest extends AbstractCheckTest { "duplicate-manifest-ignore.xml=>AndroidManifest.xml", "res/values/strings.xml")); } + + public void testAllowBackup() throws Exception { + mEnabled = Collections.singleton(ManifestOrderDetector.ALLOW_BACKUP); + assertEquals( + "AndroidManifest.xml:9: Warning: Should explicitly set android:allowBackup to " + + "true or false (it's true by default, and that can have some security " + + "implications for the application's data) [AllowBackup]\n" + + " AndroidManifest.xml", + "res/values/strings.xml")); + } + + public void testAllowIgnore() throws Exception { + mEnabled = Collections.singleton(ManifestOrderDetector.ALLOW_BACKUP); + assertEquals( + "No warnings.", + lintProject( + "allowbackup_ignore.xml=>AndroidManifest.xml", + "res/values/strings.xml")); + } + } diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/allowbackup.xml b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/allowbackup.xml new file mode 100644 index 0000000..2a95252 --- /dev/null +++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/allowbackup.xml @@ -0,0 +1,24 @@ + + + + + + + + + + + + + + + + diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/allowbackup_ignore.xml b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/allowbackup_ignore.xml new file mode 100644 index 0000000..72f8bb4 --- /dev/null +++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/allowbackup_ignore.xml @@ -0,0 +1,25 @@ + + + + + + + + + + + + + + + -- cgit v1.1