Make SharedPrefs apply package's security context on commit

Make SharedPrefs apply package's security context on commit

If SharedPrefs created a new shared preference file, the app could not
read it due to SELinux security. Applying /data/data/<package>'s
security context to /data/data/<package>/shared_prefs fixes this.

BUG=chromium:671373
, catapult:#3366

Review-Url: https://codereview.chromium.org/2743003003
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/4550ebce30c0593fb85be7890b84b942e8b7af57

[bsheedy, 2017-03-10 22:28:26]

bsheedy@chromium.org changed reviewers:
+ jbudorick@chromium.org

[bsheedy, 2017-03-10 22:28:27]

PTAL

[jbudorick, 2017-03-10 22:34:21]

https://codereview.chromium.org/2743003003/diff/1/devil/devil/android/sdk/sha...
File devil/devil/android/sdk/shared_prefs.py (right):

https://codereview.chromium.org/2743003003/diff/1/devil/devil/android/sdk/sha...
devil/devil/android/sdk/shared_prefs.py:277: for line in
self._device.RunShellCommand(['ls', '-Z', '/data/data/'],
two things here:
 - Can you split the security context determination logic into its own helper
function? Being able to write

     security_context = self._GetSecurityContext(self.package)
     self._device.RunShellCommand(['chcon', '-R', security_context, ...])

   would be cleaner & would let you test this more easily.

 - I'd be surprised if this logic worked pre-M, and it certainly isn't necessary
before that point (and may not be necessary pre-N).

https://codereview.chromium.org/2743003003/diff/1/devil/devil/android/sdk/sha...
devil/devil/android/sdk/shared_prefs.py:283: '/data/data/' + self.package +
'/shared_prefs'],
nit: '/data/data/%s/shared_prefs' % self.package

[bsheedy, 2017-03-10 23:08:58]

https://codereview.chromium.org/2743003003/diff/1/devil/devil/android/sdk/sha...
File devil/devil/android/sdk/shared_prefs.py (right):

https://codereview.chromium.org/2743003003/diff/1/devil/devil/android/sdk/sha...
devil/devil/android/sdk/shared_prefs.py:277: for line in
self._device.RunShellCommand(['ls', '-Z', '/data/data/'],
On 2017/03/10 22:34:21, jbudorick wrote:
> two things here:
>  - Can you split the security context determination logic into its own helper
> function? Being able to write
> 
>      security_context = self._GetSecurityContext(self.package)
>      self._device.RunShellCommand(['chcon', '-R', security_context, ...])
> 
>    would be cleaner & would let you test this more easily.
> 
>  - I'd be surprised if this logic worked pre-M, and it certainly isn't
necessary
> before that point (and may not be necessary pre-N).

Done. It is necessary on M, at least on my 5X, so I've set that as the minimum
version to run the code on.

https://codereview.chromium.org/2743003003/diff/1/devil/devil/android/sdk/sha...
devil/devil/android/sdk/shared_prefs.py:283: '/data/data/' + self.package +
'/shared_prefs'],
On 2017/03/10 22:34:21, jbudorick wrote:
> nit: '/data/data/%s/shared_prefs' % self.package

Done.

[jbudorick, 2017-03-10 23:10:39]

https://codereview.chromium.org/2743003003/diff/20001/devil/devil/android/sdk...
File devil/devil/android/sdk/shared_prefs.py (right):

https://codereview.chromium.org/2743003003/diff/20001/devil/devil/android/sdk...
devil/devil/android/sdk/shared_prefs.py:280: self._device.RunShellCommand(
Should we attempt to run this if _GetSecurityContext returns None?

[bsheedy, 2017-03-10 23:17:51]

https://codereview.chromium.org/2743003003/diff/20001/devil/devil/android/sdk...
File devil/devil/android/sdk/shared_prefs.py (right):

https://codereview.chromium.org/2743003003/diff/20001/devil/devil/android/sdk...
devil/devil/android/sdk/shared_prefs.py:280: self._device.RunShellCommand(
On 2017/03/10 23:10:39, jbudorick wrote:
> Should we attempt to run this if _GetSecurityContext returns None?

Running with None will cause a failure, which is probably what we want since a
preference file with incorrect security context can cause issues that are
annoying/difficult to debug. Should manually raise an exception for clarity,
though. WDYT?

[jbudorick, 2017-03-10 23:20:15]

https://codereview.chromium.org/2743003003/diff/20001/devil/devil/android/sdk...
File devil/devil/android/sdk/shared_prefs.py (right):

https://codereview.chromium.org/2743003003/diff/20001/devil/devil/android/sdk...
devil/devil/android/sdk/shared_prefs.py:280: self._device.RunShellCommand(
On 2017/03/10 23:17:51, bsheedy wrote:
> On 2017/03/10 23:10:39, jbudorick wrote:
> > Should we attempt to run this if _GetSecurityContext returns None?
> 
> Running with None will cause a failure, which is probably what we want since a
> preference file with incorrect security context can cause issues that are
> annoying/difficult to debug. Should manually raise an exception for clarity,
> though. WDYT?

Agreed on both counts.

[bsheedy, 2017-03-11 00:25:33]

https://codereview.chromium.org/2743003003/diff/20001/devil/devil/android/sdk...
File devil/devil/android/sdk/shared_prefs.py (right):

https://codereview.chromium.org/2743003003/diff/20001/devil/devil/android/sdk...
devil/devil/android/sdk/shared_prefs.py:280: self._device.RunShellCommand(
On 2017/03/10 23:20:15, jbudorick wrote:
> On 2017/03/10 23:17:51, bsheedy wrote:
> > On 2017/03/10 23:10:39, jbudorick wrote:
> > > Should we attempt to run this if _GetSecurityContext returns None?
> > 
> > Running with None will cause a failure, which is probably what we want since
a
> > preference file with incorrect security context can cause issues that are
> > annoying/difficult to debug. Should manually raise an exception for clarity,
> > though. WDYT?
> 
> Agreed on both counts.

Done.

[jbudorick, 2017-03-11 00:29:27]

lgtm w/ q

https://codereview.chromium.org/2743003003/diff/40001/devil/devil/android/sdk...
File devil/devil/android/sdk/shared_prefs_test.py (right):

https://codereview.chromium.org/2743003003/diff/40001/devil/devil/android/sdk...
devil/devil/android/sdk/shared_prefs_test.py:101:
type(self.device).build_version_sdk = mock.PropertyMock(
Does this mock get torn down?

[bsheedy, 2017-03-11 00:37:26]

https://codereview.chromium.org/2743003003/diff/40001/devil/devil/android/sdk...
File devil/devil/android/sdk/shared_prefs_test.py (right):

https://codereview.chromium.org/2743003003/diff/40001/devil/devil/android/sdk...
devil/devil/android/sdk/shared_prefs_test.py:101:
type(self.device).build_version_sdk = mock.PropertyMock(
On 2017/03/11 00:29:27, jbudorick wrote:
> Does this mock get torn down?

It looks like it does, as self.device.build_version_sdk returns <MagicMock
name='mock.build_version_sdk' id='<<some number>>'> in all tests except the ones
with that mock, in which case it returns 22.

[bsheedy, 2017-03-11 00:42:56]

The CQ bit was checked by bsheedy@chromium.org

[commit-bot: I haz the power, 2017-03-11 00:43:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 01:25:58]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489192976709240,
"parent_rev": "5cdfbfc974591fbaca53645b670c8191cb89f8aa", "commit_rev":
"4550ebce30c0593fb85be7890b84b942e8b7af57"}

[commit-bot: I haz the power, 2017-03-11 01:26:01]

Description was changed from

==========
Make SharedPrefs apply package's security context on commit

If SharedPrefs created a new shared preference file, the app could not
read it due to SELinux security. Applying /data/data/<package>'s
security context to /data/data/<package>/shared_prefs fixes this.

BUG=chromium:671373, catapult:#3366
==========

to

==========
Make SharedPrefs apply package's security context on commit

If SharedPrefs created a new shared preference file, the app could not
read it due to SELinux security. Applying /data/data/<package>'s
security context to /data/data/<package>/shared_prefs fixes this.

BUG=chromium:671373, catapult:#3366

Review-Url: https://codereview.chromium.org/2743003003
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-03-11 01:26:02]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...