Fall back to using the legacy android:allowBackup flag for apps that
don't specify the new ALLOW_ADB_BACKUP property. This is done to not
break GTS tests that rely on adb backup since corresponding apps can't
use ALLOW_ADB_BACKUP yet (ag/13205188 is blocked with some failure that
I'm currently investigating).
Bug: 175250895
Test: atest CtsBackupHostTestCases
Change-Id: Id40b2ae6b677afca18011077f57926518b28960f
http://ag/12885739 introduced a enforceCallingPermission(BACKUP) check
but callers of this API do not hold that permission. This CL fixes
this by changing the check to enforceCallingOrSelfPermission(BACKUP),
and clearing the binder identity in DevicePolicyManagerService, which
makes the system server process the owner of that call.
Bug: 158482162
Bug: 172466964
Test: atest com.android.cts.devicepolicy.{Device,Profile}OwnerTest#testBackupServiceEnabling
Change-Id: I11d863229c4d62a058aaf37446a694b9c73ae5b8
1. Adb backup is enabled by default for "android" package that corresponds to SystemBackupAgent (no change from current behavior)
2. System and privileged apps can use android.backup.ALLOW_ADB_BACKUP manifest property to enable / disable adb backup. Disabled by default.
3. Other apps can only use adb backup when running in debuggable mode.
Bug: 171032338
Test: 1. atest BackupEligibilityRulesTest
2.1. Run adb backup / restore for SystemBackupAgent and verify
success (running in system_server)
2.2. Run adb backup / restore for NexusLauncher with
"allowAdbBackup=true" and verify success (privileged app)
2.3. Run adb backup / restore for a non-privileged debuggable app
and verify success.
Change-Id: Ifefe6d888377d3ac9482928b27c86b2e562ad8fa
Base CL ag/12885739 introduced unconditional enforcement of the BACKUP
permission for callers of BackupManagerService.isBackupServiceActive()
in the service, but dropped the enforcement on the app process side
(BackupManager).
This CL makes the behavior change conditional on a compat ChangeId.
Bug: 158482162
Test: Manually checked that an app similar to the code sample from
http://b/158482162#comment1 can reproduce the behavior.
This is true both before the base CL and after this CL, when
the app targets an old SDK version (26).
Test: Checked that both (a) before this CL, (b) after this CL where
the change is manually enabled for the app via the below commands,
the app runs into a SecurityException instead:
$ adb shell am compat enable 158482162 com.example.tester
$ adb shell dumpsys platform_compat | grep 158482162
ChangeId(158482162; name=IS_BACKUP_SERVICE_ACTIVE_ENFORCE_PERMISSION_IN_SERVICE; enableSinceTargetSdk=31; packageOverrides={com.example.tester=true})
Change-Id: I58e5d2a0b438296137fd76720636c8fdce740ded
BackupManager runs in the client process, whereas BackupManagerService
runs in the system server process. Therefore, apps permissions need should
be enforced on the service side.
Bug: 158482162
Test: Manually checked that a sample app encounters SecurityException
after but not before this CL, when running code similar to the
sample in http://b/158482162#comment1 to figure out whether
isBackupServiceActive() for its own uid.
Change-Id: I59693819542a80a065a9c88373393b0ba0dbef65
Strictly speaking other owners for SettingsProvider/test/...
should exist but there aren't any right now, so it's not
worth keeping a per-file rule since I don't think that
those can be combined with include.
Bug: 159055442
Test: Treehugger
Change-Id: I38c9c227e16c91f72ee0fc761670f82160e46ae2
We've been writing many new framework-specific Error Prone checkers
to help detect obscure platform bugs, and this change starts enabling
those checkers for more packages across the platform.
Bug: 155703208
Test: manual
Exempt-From-Owner-Approval: trivial blueprint changes
Change-Id: I1db3412b0be40f6f78c68331ae01756887192071
No production code relies on the mutability. Code in fakeandroid's
AlarmManager added in http://ag/3826144 relies on the mutability,
and backup/internal/PerformInitializeTask sets a (now immutable)
PendingIntent, but fakeandroid uses a different AlarmManager instance
that the real system so these should be unrelated (I think - reviewer,
please double-check my thinking).
Fixes: 170163281
Test: Treehugger
Change-Id: I3dd45cc270c2fd8ab9d4b3402edc6cc5d765fd5a
* changes:
Upgrade AndroidFrameworkBinderIdentity to fatal.
Tighten up Binder.clearCallingIdentity() usage.
Tighten up Binder.clearCallingIdentity() usage.
Tighten up Binder.clearCallingIdentity() usage.
This is a third CL in a chain that adjusts existing malformed code
to follow AndroidFrameworkBinderIdentity best-practices.
Specifically, if a thread clears an identity they need to restore it
to avoid obscure security vulnerabilities. In addition, the relevant
"try" block must start immediately after the identity is cleared to
ensure that its restored if/when any exceptions are thrown.
Bug: 155703208
Test: make
Exempt-From-Owner-Approval: trivial refactoring
Change-Id: I74cb958b68d55a647547aae21baff6ddc364859b
This change is motivated by b/161248425 where the restore times out before all
call logs can be restored by CallLogBackupAgent. The CL increases
restore time limit for agents running in the system process. See the
linked bug below for rationale as to why the change is limited to system
agents only.
Changes in the CL:
* Split timeouts for restore session and agent restore into 2 separate
values in BackupAgentTimeoutParameters, so that the latter can be
changed independently.
* Pass application UID to #getRestoreSessionTimeoutMillis() so that we
adjust the return value based on whether the agent is part of the system.
Bug: 170076589
Test: 1. atest BackupAgentTimeoutParametersTest
2. Populate 7000 call log entries; run backup; clear call history;
run restore and verify it fails without the change and succeeds
with the change.
Change-Id: Id32e34973be380f7206cb9000ca95e6b76bbf8c0
The recently added AndroidFrameworkBinderIdentity Error Prone checker
examines code to ensure that any cleared identities are restored to
avoid obscure security vulnerabilities.
This change is a purely mechanical refactoring that adds the "final"
keyword to the cleared identity to ensure that it's not accidentally
modified before eventually being cleared. Here's the exact command
used to generate this CL:
$ find . -name "*.java" -exec sed -Ei \
's/ (long \w+ = .+?clearCallingIdentity)/ final \1/' \
{} \;
Bug: 155703208
Test: make
Exempt-From-Owner-Approval: trivial refactoring
Change-Id: I832c9d70c3dfcd8d669cf71939d97837becc973a
Context: KeyValueBackupTask's ctor takes a DataChangedJournal instance.
KeyValueBackupTaskTest uses a mock instance to check that remove() is
called. http://ag/12218310 made DataChangedJournal final, breaking the
test. This CL fixes the failure cause by making the class nonfinal.
Note that the test still fails because at least one unrelated failure
cause remains - see http://b/162022005#comment11
In the longer term, we may want to consider splitting DataChangedJournal
into an interface/abstract class + a concrete implementation; there's no
good reason why KeyValueBackupTask and KeyValueBackupReporter need to
depend on specifically the implementation class, when all they're calling
is remove() and toString(). However, the change to make the class final
wasn't important, and reverting it is the easiest / fasted way to unbreak
the test.
Bug: 162022005
Test: atest KeyValueBackupTaskTest
Change-Id: I22b2a5d65f03c4ccdcc718ed3866d37ba9441385
System server should not directly called Settings method such as Settings.Secure.getString()
instead of Settings.Secure.getStringForUser(). The "ForUser" methods allow us to properly
enforce the user's restrictions and it prevents issues like b/163571398.
This CL is part of the effort that changes existing usage of non "ForUser" methods
to using "ForUser" methods. It is supposed to act as a no-op.
BUG: 166312046
Test: builds
Change-Id: I632a243ec916a95437bcd32e5c480ab947eebb4e
Add an API to BackupManager to start restore in migration mode.
Bug: 160407842
Test: atest PerformUnifiedRestoreTaskTest BackupManagerServiceTest
UserBackupManagerServiceTest
Change-Id: I75fdb1c99edc3d11d1264d69e1f20c682d588479
This is just a plain refactoring: the removed methods in the changed
classes were called by default by the new methods in the superclass
(SystemService).
Test: m
Test: atest NotificationManagerServiceTest BackupManagerServiceRoboTest
Fixes: 161943081
Exempt-From-Owner-Approval: refactoring without side-effects
Change-Id: Ifd8df592eb4494cc0922b7e0b2ff20187b8a8b3e
See https://source.android.com/setup/contribute/respectful-code for reference
PerformAdb{Backup,Restore}Task handles backing up to and restoring from
a file. The backup data is encrypted with a randomly generated
encryption key. The encryption key is encrypted with a key that is
derived from a user supplied passphrase.
The key that was used to encrypt and decrypt the backup data used to be
called the "master key", I've renamed it to "encryption key" which
better describes its purpose.
Bug: 161896447
Test: `atest PerformAdbRestoreTaskTest`, `atest TarBackupReaderTest`
and manually tested using adb with the following commands:
`adb backup -all -apk -shared -f backup.ab` and
`adb restore backup.ab`.
Change-Id: I8e6b22a06b2a583440839994f0626d370bdb2e19
With the goal of reducing log spam, print only a summary 'Found stale
backup journal' messages instead of logging within the inner loop.
Previously, over 12k messages could be printed at a time from this
function.
Before this CL:
- a backup was scheduled for each packageName from each stale
journal
- one (or two, if MORE_DEBUG) message was logged for each
packageName in each journal file.
After this CL:
- packageNames are de-duplicated before scheduling backups or logging
(it's not clear to me whether duplicate packageNames previously
occurred, in practice).
- one message is logged for the number (if > 0) of stale journals.
- one message is logged for the number (including their names, if
MORE_DEBUG) of packages.
Bug: 161940947
Test: fewer 'Found state backup journal' messages printed
Merged-In: Ia1343e4cea31feb1eba9da561d20736eb5df0a14
Change-Id: Ia1343e4cea31feb1eba9da561d20736eb5df0a14
With the goal of reducing log spam, print only a summary 'Found stale
backup journal' messages instead of logging within the inner loop.
Previously, over 12k messages could be printed at a time from this
function.
Before this CL:
- a backup was scheduled for each packageName from each stale
journal
- one (or two, if MORE_DEBUG) message was logged for each
packageName in each journal file.
After this CL:
- packageNames are de-duplicated before scheduling backups or logging
(it's not clear to me whether duplicate packageNames previously
occurred, in practice).
- one message is logged for the number (if > 0) of stale journals.
- one message is logged for the number (including their names, if
MORE_DEBUG) of packages.
Bug: 161940947
Test: fewer 'Found state backup journal' messages printed
Merged-In: Ia1343e4cea31feb1eba9da561d20736eb5df0a14
Change-Id: Ia1343e4cea31feb1eba9da561d20736eb5df0a14
(cherry picked from commit 112e3c2d049d304b0aa57750fcaa6415ea2b6fef)
Since commit c31a839fd3ecc91807d735884d09fcbaf62e9244, this
logic looped "while (dataInputStream.available() > 0)", which
indicates whether more bytes can be read _without blocking_.
It's possible for this to be false even when there are more
bytes available in the file, which means that this loop was
prone to premature termination; somewhat surprisingly to me, a
DataInputStream(BufferedInputStream(FileInputStream))) wrapper
does return available() > 0 initially (empirically tested on
the latest development version), but it's not clear whether
this will reliably remain the case later on in the file and
how often in practice this failed. Either way, this code
was fragile and subject to potential silent breakage in future
where some packageNames would not have been reliably read.
This CL changes the logic back to reading unconditionally but
catching EOFException, like the corresponding logic did prior
to commit c31a839fd3ecc91807d735884d09fcbaf62e9244 (July 2017).
To demonstrate the original bug through a regression test, I
would have needed to extract a static helper method
@VisibleForTesting forEach(Consumer, InputStream) to demonstrate
the behavior with an InputStream whose available() returns 0.
This didn't seem worth it, so I've only added a comment explaining
the logic to minimize the chance of future regression.
This CL also fixes incorrect use of a try-with-resources statement
that would have not closed the FileInputStream if the
BufferedInputStream ctor had thrown (would not have occurred in
practice).
Bug: 162160897
Test: atest \
FrameworksServicesTests:com.android.server.backup.DataChangedJournalTest
Change-Id: I7641cf9ea269ef050ceb716c4e7fe34166bc8295
Passing a null File into DataChangedJournal would always have
thrown NPE if various methods (toString(), equals(), hashCode(),
addPackage()) were called later, but by that time the origin
(stacktrace) of the offending null value would have been lost.
This CL changes the class to detect this error earlier, namely
in the ctor.
In addition, this CL changes newJournal(File journalDirectory)
to not accept null; previously, if null was passed, then the
journal would have been created in the wrong directory, which
would then have led to incorrect behavior of listJournals:
- listJournals(null) would have thrown NPE
- listJournals(nonNullJournalDirectory) would not have found
the journal.
I convinced myself through code inspection that this error case
is not currently possible, but this was not straightforward:
UserBackupManagerService.mJournalDir is passed to DCJ.newJournal()
from writeToJournalLocked() which is called from dataChangedImpl()
which is called from mPackageTrackingReceiver, which is luckily
registered only after mJournalDir was initialized fairly late in the
non-test ctor); should this become buggy in future, then after this
CL it should be more likely to be discovered.
Bug: 162022005
Test: Manually checked no non-test callers pass in non-null.
Test: atest FrameworksServicesTests:com.android.server.backup.DataChangedJournalTest
Change-Id: I1756add05f3d65e455b2e61448f98ceceb5b39f1
Problems:
1. The class implemented equals() but not hashCode()
2. equals() attempted to canonicalize the underlying path and silently
returned false if the I/O operation failed.
Fixes:
1. Implemented hashCode().
2. Stopped equals() attempting canonicalization and built in directly
on top of File.equals() instead. I also considered moving the
canonicalization to the DataChangedJournal ctor, but that
- had the undesirable side effect of stopping interaction with the
journal for reasons other than the file itself being corrupt, and
- posed the problem of what to do in case of an IOException during
construction.
The downside of course is that caller which (inappropriately) relied
on equals() doing canonicalization can no longer rely on that. I
didn't find any such callers.
Note:
When DateChangedJournal.listJournals() fails to perform a directory
listing (e.g. if the journal directory doesn't exist), it currently
still returns an (empty) ArrayList<DateChangedJournal>. This CL does
not address this, but in a follow-up CL I might change this to return
an unmodifiable List or Set<DateChangedJournal>, and throw an
IOException if the directory contents can't be listed.
Bug: 162022005
Test: atest FrameworksServicesTests:com.android.server.backup.DataChangedJournalTest
Change-Id: I94f35f9a3082f398758fe5b03fb8d260e4fb3e1b
With the goal of reducing log spam, print only a summary 'Found stale
backup journal' messages instead of logging within the inner loop.
Previously, over 12k messages could be printed at a time from this
function.
Before this CL:
- a backup was scheduled for each packageName from each stale
journal
- one (or two, if MORE_DEBUG) message was logged for each
packageName in each journal file.
After this CL:
- packageNames are de-duplicated before scheduling backups or logging
(it's not clear to me whether duplicate packageNames previously
occurred, in practice).
- one message is logged for the number (if > 0) of stale journals.
- one message is logged for the number (including their names, if
MORE_DEBUG) of packages.
Bug: 161940947
Test: fewer 'Found state backup journal' messages printed
Change-Id: Ia1343e4cea31feb1eba9da561d20736eb5df0a14
After refactoring AppBackupUtils into BackupEligibilityRules (see the
other linked CL), update the usages throughout the code.
Bug: 161241479
Test: atest UserBackupManagerServiceTest
atest TarBackupReaderTest
atest BackupHandlerTest
atest PerformUnifiedRestoreTaskTest
Change-Id: I2a90c4f5b951fa3e3c564a1065ad10a88cc16273
AppBackupUtils contains static utility methods for checking backup
eligibility of packages. Refactor it into an instance class called
BackupEligibilityRules to better reflect the responsibilities of the
class, encapsulate eligibility-related state as well as logic and
simplify testing. See the bug description for full rationale.
Bug: 161241479
Test: atest AppBackupUtilsTest
Change-Id: I8ae47ac2a15e3c70da8feab752dd8354951224ea
Add an override of BackupManager#requestBackup where type of the
operation (a regular backup or a migration) can be specified.
Bug: 160407842
Test: atest UserBackupManagerServiceTest
Change-Id: Ia54fa26b040c3ec3612672585561794ff831afef
This CL will be part of a series to implement backup / restore of all
app data during a device-to-device migration. Detailed overview of the changes at go/br-fsd2d-design-doc-app-data.
Bug: 160407842
Test: atest AppBackupUtilsTest
Change-Id: I35ce627f1d372437d8ba8495029c6df15db31552
