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
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
Since commit c31a839fd3, 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 c31a839fd3 (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
Bug: 159648065
Test: N/A
Change the setting key to be 'backup_skip_user_facing_packages'. See the
bug for context.
Change-Id: I315141a509976821c7db311544a5c0f4e6fd1917
It was bothering me that the space (incorrectly) came before the ':'
rather than after :-)
Test: Treehugger
Change-Id: Iacec9f8da8988bb1edf994cd077af27f9ba8c777
Since Android 10, backupPm() includes sendDataToTransport(), which was
not previously the case. This means that error handling logic that
deletes the backup state file (causing initialize_device() on the next
attempt, which deletes any existing backup) will now also be triggered
upon errors during sendDataToTransport(), which wasn't previously
(Android <= 9) the case.
This has the potential of making an existing temporary outage much
worse:
1. A few devices might run into temporary issues, e.g. a B&R server
returning HTTP 503 Service Unavailable (treated as a
TransientHttpStatusException instanceof NetworkException, which
is mapped to TRANSPORT_ERROR during handleTransportStatus(),
which results in a TaskException with stateCompromised==false
but which backupPm() wraps in another TaskException that forces
stateCompromised=true).
2. On their next backup attempt, those devices throw away any
existing backup and start from scratch (initialize_device()),
increasing the load on the server.
3. This leads to a positive-feedback loop where more devices than
before run into HTTP 503 Service Unavailable.
4. As a result, masses of devices delete their backups and then
hammer the B&R server with attempts to upload new backups.
5. Backups are unavailable to any users who would otherwise rely
on them during this outage.
To improve on this dangerous situation, this CL changes the code to
force stateCompromised=true only for TaskExceptions thrown
specifically during extractPmAgentData(), and (as before) for all
AgentExceptions.
Note that the code is still quite brittle. It still seems like we
are probably forcing stateCompromised=true in too many situations,
but it's hard to say so this CL is being conservative about the
changes. Changing back to the old behavior could be done through
a local change around KeyValueBackupTask.java:676; a future CL may
do this to have a safety hatch in case we want to cherry-pick this
CL into an upcoming Android release late in the release cycle.
[1] https://android.googlesource.com/platform/frameworks/base/+/refs/heads/pie-dev/services/backup/java/com/android/server/backup/internal/PerformBackupTask.java#1035
[2] https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java#1040
[3] https://source.corp.google.com/piper///depot/google3/java/com/google/android/gmscore/integ/modules/backup/transport/src/com/google/android/gms/backup/transport/GmsBackupTransport.java;l=770;rcl=281845876
Bug: 144030477
Test: Checked that the following passes after this CL, but
testRunTask_whenTransportReturnsErrorForPm_updatesFilesAndCleansUp()
fails if I revert the state of KeyValueBackupTask.java to before this CL:
ROBOTEST_FILTER=KeyValueBackupTaskTest make \
RunBackupFrameworksServicesRoboTests
Change-Id: I6c622c55fbd804ec0a12e0bea7ade1308f7a3877
Previously, we attempt to pass ownership of the fd owned by a
ParcelFileDescriptor to a FileInputStream, which explodes when we try to
close it.
Bug: http://b/156867945
Test: treehugger
Change-Id: I9d5124658beb50f0a08499ed09e652037cb9ae66
There were a few issues with the ancestral file storage methods which
I've fixed in this refactoring;
1) The log message for "get" said it was doing a write
2) The logic to get the file created a new file even if this was a read
3) The reading of the file asked for write permissions which it didn't
need
4) By asking for the correct permissions we can use the RandomAccessFile
constructor behaviour to create a file if needed.
5) By asking for the correct permissions we can catch the FileNotFound
exception thrown when the file doesn't exist and avoid creating an empty
unused file
Fixes: 157922462
Test: atest UserBackupManagerServiceTest ProfileSerialNumberHostSideTest
Change-Id: Ia5b070d2dbe9a778ec39473ec484f889ca8a8214
Add the user ID so it's easier to see which user log messages refer to.
There's a bit of churn around conditionals in this to ensure that the
code complies with the style guidelines.
Fixes: 148376687
Test: make RunBackupFrameworksServicesRoboTests
Change-Id: I3ca92d21492fae4b89cb73fb39db1a490c796f5d
Add the user ID so it's easier to see which user log messages refer to.
There's a bit of churn around conditionals in this to ensure that the
code complies with the style guidelines.
Fixes: 148376687
Test: make RunBackupFrameworksServicesRoboTests
Change-Id: I3ca92d21492fae4b89cb73fb39db1a490c796f5d
When backup service is brought up, we read the enabled status
from a file and store it in a class variable in
UserBackupManagerService. However, we also write the value we just read
back into the file. This is an unncessary operation and it makes the
code more fragile: if we read the wrong value due to some I/O error,
we'll then write it back making the error permanent.
More details here: https://docs.google.com/document/d/1GtR2HYMiSJuy0wuxBg8n_zkEJGl3C5apcvpvKheNa_A/edit
Bug: 149084461
Test: 1. "bmgr enable true"; verify "bmgr enabled" returns true; reboot; verify "bmgr enabled" returns true
2. "bmgr enable false"; verify "bmgr enabled" returns false; reboot; verify "bmgr enabled" returns false
3. atest RunBackupFrameworksServicesRoboTests
4. atest FrameworksServicesTests:UserBackupManagerServiceTest
Change-Id: If213a6b1de0de51c9337138be347c304b44689d1
This reverts commit 27c64a3bed.
Reason for revert: The GMSCore code to handle the flag will be part of DP2 and so we can remove the flag ahead of the cut.
GMSCore CL Status; https://cl-status.corp.google.com/#/summary/gmscore_prod/291378558
GMSCore Calendar; http://go/gms-schedule
Bug: 147481066
Test: m -j RunBackupFrameworksServicesRoboTests
Change-Id: I4159e064e739c6f366063c7fadd7cca40a7f07d9
Adding a log statement at debug level so when the key blocking system
is tested end-to-end there's an easy indicator of a key being blocked.
Bug: 149548272
Test: No code which affects functionality to test
Change-Id: I9de8a7d47ffb75520d1adcba620ec13345c1c4ae
Bug: 145126096
Test: atest KeyValueRestoreExclusionHostSideTest
atest PerformUnifiedRestoreHostSideTest
atest UserBackupPreferencesTest
Currently PerformUnifiedRestoreTask gets the list of blocked restore
keys at construction. However, at that point the list might not be fully
constructed yet. We should get the keys through the getter avaialble in
UserBMS when we need them.
Change-Id: I62ad34138ba7a893e66d6af05d2e242c9c964a44
