Skip to content
This repository has been archived by the owner on Oct 29, 2024. It is now read-only.

Don’t require strong auth on regular time interval #19

Conversation

ypid
Copy link

@ypid ypid commented Nov 24, 2019

This disables the need to provide strong authentication except when booting the device. The idea behind this is that you are no longer forced to enter your strong authentication credentials in random locations where it might be easy to snoop your strong authentication credentials allowing an adversary to boot and decrypt your device against your will.

Changing DEFAULT_STRONG_AUTH_TIMEOUT_MS is not enough. Rather, it has the opposite effect. In my tests, it caused the strong auth to be required every hour rather than a 42 d interval.

Ref: frameworks/base/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java

In my tests, dpm.getRequiredStrongAuthTimeout(null, userId)) returned 3600000.

Note that this patch circumvents the values that DeviceAdmin may provide. In other words, it ignores whatever a MDM would request for the strong auth timeout!

To be clear, this patch is a practical approach, in an ideal world, we would properly use the DeviceAdmin feature.

Confirmed working on:

  • Android 9
  • Android 10

Submitted to:

Related to:

android-build-team Robot and others added 30 commits April 10, 2019 23:48
Change-Id: Icb7ab9c3367c3ee1e6e5754a3b16eff1e935502f
Change-Id: Ifec20fa1438852336acb0926affa02289e286b2e
This reverts commit c6778e5.

Reason for revert: This CL causes Bluetooth fail to start

Bluetooth Java services cannot turn on.

Change-Id: I4a9649d61de7781d3a00074da780c3a5a1dfbd56
Fixes: 130260055
Test: compile, BluetoothInstrumentationTests
(cherry picked from commit 6103479)
Change-Id: I356979d5696c5d8e2fb49d19ed0310b187dc0a03
Change-Id: I940ca071ecbf123c7a223fb3036aa7572722ede3
Change-Id: I5aad6560fea708f3f1a638a6fbc9b0f4455a90ca
In the process of revising libnativehelper interface, the
initialization of the fArray field was dropped. This change fixes
this and adds regression tests.

In writing the tests, it became apparent that the AutoBufferPointer
destructor would crash if the underlying buffer was heap array backed
and the buffer position was non-zero. ReleasePrimitiveArrayCritical
was called with the element corresponding to the current position
rather than the first element.

Bug: b/130390512
Test: atest FrameworksCoreTests:android.graphics.BitmapTest

(cherry picked from commit e522c99)

Merged-In: I319512aaede7ba8af5d013c9281417695abf9099
Change-Id: I319512aaede7ba8af5d013c9281417695abf9099
(cherry picked from commit 4174b76)
This reverts commit 08c5083.

Reason for revert: Creating revert to cherrypick into qt-release and fix daily build

Change-Id: I9a760c222e1b0dac9c9ec131cc14e7a3a689e939
(cherry picked from commit 1faaeaf560eed54163c67293e6accf9c4e61755e)
…to qt-release

Change-Id: Ie5eaf6dc7e3e2433c8c77636a151efc33f7f7cb5
Change-Id: If88ba8f64ceb984a6997fce7fba5f168145eba75
Change-Id: I0e3fb60c127663c5bec35e5fa7625da4c89eb52d
Change-Id: I221f9f27d44f68d7e858aba8e86310d569785885
Change-Id: I07bfda538a993a526f5421496cdae9d887a8ef50
Change-Id: I8739c5430cef3cd396d0241b6996bd77a39eeb2b
Change-Id: I9a7381da407316924e7a4ef6d9c275d1fce61a4c
Change-Id: Ia16a3e535c1d3ba20ed1b77f40a76c5266351b2e
Change-Id: I8a209ad2a587dfd86ba5277c6a6e7d69e564f94c
This reverts commit 75a7e2f.

Reason for revert: Turns out this wipes out data we need :(

Change-Id: Id1d79dec17639440eae55e41fb4e91f0d9e6162c
Fixes: 131290765
(cherry picked from commit 3e49669)
Change-Id: I1f31864f532780c2081e6f9405396395226d7cc9
DirectActions are abstract actions defined by an Activtiy. The
actual definition of these actions will be available through
the support lib.

This API provides a secure channel for system or assistant to
interact with a running app using these Actions.

Test: atest CtsVoiceInteractionTestCases
Test: added android.voiceinteraction.cts.DirectActionsTest

Bug: 129705716

Change-Id: I0ce568e0d8f41e0fe46306052016a74c7b394efa
(cherry picked from commit d40c345)
Change-Id: Ib8380f98e69880d5ec85fa35b6feeebfe5d2ffa8
Change-Id: I4b5c8fd25546015cc9962211104f4ea817e362df
Change-Id: Ie56b2bb4e63a14a83c7c3426a737f1a0dc4d4664
Change-Id: I65cdd8a6d8f2c3744def76ee18c7489ad2ffefe9
Change-Id: I8e0d784b1c344bc8864ddd1822f8eee3f4f54e51
Change-Id: Ia68cc7141fca212324daeb8a6b736a43096bd4b2
It seems that for some people the estimate was being set to null
as part of the process to update the estimate as they were trying
to access it. This CL makes it so that we don't allow callbacks
to start if we are currently updating which will ensure that this
doesn't happen again.

Test: repro'd issue without change, issue gone with change
Bug: 131607084
Change-Id: I938d05a3977bb4bba2c5d28e800d94120b8425d2
(cherry picked from commit af36fb3)
Change-Id: I5c46d334c22a5b681782cc7aeaf43d92ef5df004
1. startingData was accessed on a different thread without a lock held.
This caused synchronization issues where startingData could be set to
null while the starting window was getting created.

2. In onFirstWindowDrawn, startingData was not being set to null if the
startingWindow didn't exist. That means if the startingWindow hadn't
been created yet, it would get created after the first window was drawn.

Fixes: 129654434
Test: Steps from bug no longer reproducible
Change-Id: Ic5086798082b9f312cbd5069a937eac95cff7a9c
(cherry picked from commit a8f07a7)
Change-Id: I4cf77896b5924f9c0a0dc8be6496db80c4a6935e
thestinger and others added 20 commits November 4, 2019 15:57
Only support fetching the serial number via the new Build.getSerial()
requiring the READ_PHONE_STATE permission.
These are treated as a runtime permission even for legacy apps. They
need to be granted by default for all apps to maintain compatibility.
This covers sensors not included in the existing runtime permission for
body sensors.
This disables the need to provide strong authentication except when booting the
device. The idea behind this is that you are no longer forced to enter your
strong authentication credentials in random locations where it might be easy to
snoop your strong authentication credentials allowing an adversary to boot and
decrypt your device against your will.

Changing DEFAULT_STRONG_AUTH_TIMEOUT_MS is not enough. Rather, it has the
opposite effect. In my tests, it caused the strong auth to be
required every hour rather than a 42 d interval.

Ref: frameworks/base/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java

In my tests, dpm.getRequiredStrongAuthTimeout(null, userId)) returned 3600000.

Note that this patch circumvents the values that DeviceAdmin may provide. In other words, it ignores whatever a MDM would request for the strong auth timeout!

To be clear, this patch is a practical approach, in an ideal world, we would properly use the DeviceAdmin feature.

Confirmed working on:

* Android 9
* Android 10

Submitted to:

* hashbang/os#32
* RattlesnakeOS/community_patches#9
* https://github.com/ypid/ypid-android-patches
@renlord
Copy link
Contributor

renlord commented Nov 25, 2019

What if the user never reboots their phone and ultimately forgets their strong auth credentials? The regular interval is quite useful at reminding users of their strong auth credentials.

IMO, as a deterrence against snooping, the randomzied keypad/keyboard approach is more promising.

@ypid
Copy link
Author

ypid commented Nov 25, 2019

I probably should mentioned the context out of which I opened this PR: https://www.reddit.com/r/GrapheneOS/comments/dyjgry/couple_questions_regarding_usage/

As pointed out in hashbang/os#32 (comment), this is of course a downside of this patch. Users must be made aware of this risk (in the case that this PR gets merged, which I am not sure it should). But, even though the 42 d timeout in this patch is not effective, every serious GrapheneOS user will at least reboot every month ;-)

IMO, as a deterrence against snooping, the randomzied keypad/keyboard approach is more promising.

Not sure on that. In my daily usage, I figured I would like my phone to not ask me for strong auth in any such situation at all. The adversary I imaging could just watch the screen and see exactly what you enter.

@thestinger
Copy link
Contributor

I don't want this as an unconditional change since it substantially decreases security compared to the weak authentication mechanism timing out. You need to rethink this.

@thestinger thestinger closed this Jan 27, 2020
@ypid
Copy link
Author

ypid commented Jan 27, 2020

You need to rethink this.

I agree with you. For reference, I opened linux-colonel/AdminControl#11 where I think such feature could find a home.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.