Repository navigation
Bug 2081834 - Add a shared profile authentication check for sensitive data on Android Automotive OS - #426
Bug 2081834 - Add a shared profile authentication check for sensitive data on Android Automotive OS#426JohnOberhauser wants to merge 1 commit into
Conversation
|
Warning The target branch has been automatically set to Please Edit this PR if this wasn't your intention. |
8898fd9 to
0df9b53
Compare
![mozilla-code-review[bot]](https://avatars.githubusercontent.com/in/3631945?s=60&v=4){kind=small}
There was a problem hiding this comment.
24 issues have been found in this revision.
24 issues are located outside of the files touched by the patch:
Cargo.lockError: Crate depends on a vulnerable version of zip.
Advisory:
zip: ZipArchive::extract aborts the process on a symlink entry with a large declared size
Package: zip
ID: RUSTSEC-2026-0429
CVSS: CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H
Report date: 2026-10-22
ZipArchive::extract() sizes the symlink-target buffer from the uncompressed
size declared in the central directory record, before reading a single byte of
entry data:
let mut target = Vec::with_capacity(file.size() as usize);Both inputs are attacker-controlled. ZipFile::size() returns
uncompressed_size verbatim from the central directory, widened to a full u64
by a ZIP64 extended-information extra field, and is never cross-checked against
the compressed size, the remaining archive length, or any platform-appropriate
bound for a symlink target. ZipFile::is_symlink() is decided entirely by the
S_IFLNK bit of the external-attributes field.
Vec::with_capacity() is infallible: when the reservation is refused it calls
handle_alloc_error() and aborts the process. This is not a Rust panic. It is
not caught by catch_unwind(), is unaffected by panic = "unwind", and is not
contained by running the call on a worker thread.
A 112-byte archive is sufficient. One Stored entry flagged S_IFLNK with a
ZIP64 field declaring uncompressed_size = 2^62 and a zero-byte payload makes
extract() fail with:
memory allocation of 4611686018427387904 bytes failed
Aborted (core dumped) # exit 134
The declared value only needs to exceed the host's address space, not its RAM:
Vec::with_capacity() maps address space without touching pages, so resident
memory stays flat and the abort happens on the refused reservation itself. Under
a ulimit -v cap or a memory-constrained container the threshold drops far
enough that a plain 32-bit uncompressed_size with no ZIP64 field suffices.
Applications that extract archives from untrusted input are affected on Unix and
Windows targets with default features and no special configuration: upload
handling, mail-attachment processing, package and artefact ingestion, backup
restore and container-image layer extraction. No confidentiality or integrity
property is affected: there is no memory unsafety, no out-of-bounds access, no
disclosure and no data corruption.
The fix landed in zip-rs/zip2#984 and
shipped in 9.0.0.
For further details see: GHSA-q7v9-x93c-xqv3
Patched versions: [
">=9.0.0"
]
Advisory metadata: {
"aliases": [
"CVE-2026-115909",
"GHSA-q7v9-x93c-xqv3"
],
"related": [],
"collection": "crates",
"categories": [
"denial-of-service"
],
"keywords": [
"dos",
"symlink",
"unbounded-allocation",
"extract"
],
"informational": null,
"references": [],
"source": null,
"withdrawn": null,
"license": "CC0-1.0"
}
Package info: {
"name": "zip",
"version": "2.4.2",
"source": null,
"checksum": null,
"dependencies": [
{
"name": "arbitrary",
"version": "1.4.1",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "crc32fast",
"version": "1.4.2",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "crossbeam-utils",
"version": "0.8.21",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "displaydoc",
"version": "0.2.4",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "flate2",
"version": "1.1.4",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "indexmap",
"version": "2.14.1",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "memchr",
"version": "2.7.4",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "thiserror",
"version": "2.0.12",
"source": "registry+https://github.com/rust-lang/crates.io-index"
}
],
"replace": null
} [cargo-audit: source-test-mozlint-cargo-audit]
mfbt/RefPtr.h:49Error: Member access into incomplete type 'nsIPrincipal' [clang-tidy: clang-diagnostic-error]mfbt/RefPtr.h:48Error: Member access into incomplete type 'nsINode' [clang-tidy: clang-diagnostic-error]mfbt/RefPtr.h:48Error: Member access into incomplete type 'nsIPrincipal' [clang-tidy: clang-diagnostic-error]mfbt/RefPtr.h:49Error: Member access into incomplete type 'nsINode' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:361Error: Member access into incomplete type 'nsINode' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:421Error: No matching function for call to 'ToSupports' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:358Error: Member access into incomplete type 'nsIPrincipal' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:339Error: No matching function for call to 'ToSupports' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:358Error: Incomplete type 'nsINode' named in nested name specifier [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:358Error: Member access into incomplete type 'nsINode' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:361Error: Member access into incomplete type 'nsIPrincipal' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:358Error: Incomplete type 'nsIPrincipal' named in nested name specifier [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:327Error: Static assertion failed due to requirement 'std::integral_constant<bool, false>::value': nsCOMPtr only works for types with IIDs. Either use RefPtr; add an IID to your type with NS_INLINE_DECL_STATIC_IID/ ;or make the nsCOMPtr point to a base class with an IID. [clang-tidy: clang-diagnostic-error]memory/replace/dmd/DMD.h:85Error: Use of undeclared identifier 'MOZ_ASSERT' [clang-tidy: clang-diagnostic-error]xpcom/string/nsReadableUtils.h:422Error: Non-const lvalue reference to type 'nsAString' (aka 'nsTSubstring<char16_t>') cannot bind to a temporary of type 'Span<element_type, dynamic_extent>' (aka 'Span<char16_t, dynamic_extent>') [clang-tidy: clang-diagnostic-error]xpcom/string/nsReadableUtils.h:422Error: No member named 'EnsureUtf16ValiditySpan' in namespace 'mozilla'; did you mean 'EnsureUTF16Validity'? [clang-tidy: clang-diagnostic-error]xpcom/string/nsReadableUtils.h:411Error: No member named 'Utf16ValidUpTo' in namespace 'mozilla' [clang-tidy: clang-diagnostic-error]xpcom/string/nsTSubstring.h:22Error: "Using XPCOM strings is limited to code linked into libxul." [clang-tidy: clang-diagnostic-error]xpcom/threads/TimerThreadMonitor.h:72Error: Deleted member function should be public [clang-tidy: modernize-use-equals-delete,-warnings-as-errors]
Checker reliability is high, meaning that the false positive ratio is low.xpcom/threads/TimerThreadMonitor.h:71Error: Deleted member function should be public [clang-tidy: modernize-use-equals-delete,-warnings-as-errors]
Checker reliability is high, meaning that the false positive ratio is low.xpcom/threads/TimerThreadMonitor.h:68Error: Deleted member function should be public [clang-tidy: modernize-use-equals-delete,-warnings-as-errors]
Checker reliability is high, meaning that the false positive ratio is low.xpcom/threads/TimerThreadMonitor.h:69Error: Deleted member function should be public [clang-tidy: modernize-use-equals-delete,-warnings-as-errors]
Checker reliability is high, meaning that the false positive ratio is low.xpcom/threads/TimerThreadMonitor.h:70Error: Deleted member function should be public [clang-tidy: modernize-use-equals-delete,-warnings-as-errors]
Checker reliability is high, meaning that the false positive ratio is low.
This review is now deprecated.
There was a problem hiding this comment.
24 issues have been found in this revision.
24 issues are located outside of the files touched by the patch:
Cargo.lockError: Crate depends on a vulnerable version of zip.
Advisory:
zip: ZipArchive::extract aborts the process on a symlink entry with a large declared size
Package: zip
ID: RUSTSEC-2026-0429
CVSS: CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H
Report date: 2026-10-22
ZipArchive::extract() sizes the symlink-target buffer from the uncompressed
size declared in the central directory record, before reading a single byte of
entry data:
let mut target = Vec::with_capacity(file.size() as usize);Both inputs are attacker-controlled. ZipFile::size() returns
uncompressed_size verbatim from the central directory, widened to a full u64
by a ZIP64 extended-information extra field, and is never cross-checked against
the compressed size, the remaining archive length, or any platform-appropriate
bound for a symlink target. ZipFile::is_symlink() is decided entirely by the
S_IFLNK bit of the external-attributes field.
Vec::with_capacity() is infallible: when the reservation is refused it calls
handle_alloc_error() and aborts the process. This is not a Rust panic. It is
not caught by catch_unwind(), is unaffected by panic = "unwind", and is not
contained by running the call on a worker thread.
A 112-byte archive is sufficient. One Stored entry flagged S_IFLNK with a
ZIP64 field declaring uncompressed_size = 2^62 and a zero-byte payload makes
extract() fail with:
memory allocation of 4611686018427387904 bytes failed
Aborted (core dumped) # exit 134
The declared value only needs to exceed the host's address space, not its RAM:
Vec::with_capacity() maps address space without touching pages, so resident
memory stays flat and the abort happens on the refused reservation itself. Under
a ulimit -v cap or a memory-constrained container the threshold drops far
enough that a plain 32-bit uncompressed_size with no ZIP64 field suffices.
Applications that extract archives from untrusted input are affected on Unix and
Windows targets with default features and no special configuration: upload
handling, mail-attachment processing, package and artefact ingestion, backup
restore and container-image layer extraction. No confidentiality or integrity
property is affected: there is no memory unsafety, no out-of-bounds access, no
disclosure and no data corruption.
The fix landed in zip-rs/zip2#984 and
shipped in 9.0.0.
For further details see: GHSA-q7v9-x93c-xqv3
Patched versions: [
">=9.0.0"
]
Advisory metadata: {
"aliases": [
"CVE-2026-115909",
"GHSA-q7v9-x93c-xqv3"
],
"related": [],
"collection": "crates",
"categories": [
"denial-of-service"
],
"keywords": [
"dos",
"symlink",
"unbounded-allocation",
"extract"
],
"informational": null,
"references": [],
"source": null,
"withdrawn": null,
"license": "CC0-1.0"
}
Package info: {
"name": "zip",
"version": "2.4.2",
"source": null,
"checksum": null,
"dependencies": [
{
"name": "arbitrary",
"version": "1.4.1",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "crc32fast",
"version": "1.4.2",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "crossbeam-utils",
"version": "0.8.21",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "displaydoc",
"version": "0.2.4",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "flate2",
"version": "1.1.4",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "indexmap",
"version": "2.14.1",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "memchr",
"version": "2.7.4",
"source": "registry+https://github.com/rust-lang/crates.io-index"
},
{
"name": "thiserror",
"version": "2.0.12",
"source": "registry+https://github.com/rust-lang/crates.io-index"
}
],
"replace": null
} [cargo-audit: source-test-mozlint-cargo-audit]
mfbt/RefPtr.h:49Error: Member access into incomplete type 'nsIPrincipal' [clang-tidy: clang-diagnostic-error]mfbt/RefPtr.h:48Error: Member access into incomplete type 'nsINode' [clang-tidy: clang-diagnostic-error]mfbt/RefPtr.h:48Error: Member access into incomplete type 'nsIPrincipal' [clang-tidy: clang-diagnostic-error]mfbt/RefPtr.h:49Error: Member access into incomplete type 'nsINode' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:361Error: Member access into incomplete type 'nsINode' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:421Error: No matching function for call to 'ToSupports' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:358Error: Member access into incomplete type 'nsIPrincipal' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:339Error: No matching function for call to 'ToSupports' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:358Error: Incomplete type 'nsINode' named in nested name specifier [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:358Error: Member access into incomplete type 'nsINode' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:361Error: Member access into incomplete type 'nsIPrincipal' [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:358Error: Incomplete type 'nsIPrincipal' named in nested name specifier [clang-tidy: clang-diagnostic-error]xpcom/base/nsCOMPtr.h:327Error: Static assertion failed due to requirement 'std::integral_constant<bool, false>::value': nsCOMPtr only works for types with IIDs. Either use RefPtr; add an IID to your type with NS_INLINE_DECL_STATIC_IID/ ;or make the nsCOMPtr point to a base class with an IID. [clang-tidy: clang-diagnostic-error]memory/replace/dmd/DMD.h:85Error: Use of undeclared identifier 'MOZ_ASSERT' [clang-tidy: clang-diagnostic-error]xpcom/string/nsReadableUtils.h:422Error: Non-const lvalue reference to type 'nsAString' (aka 'nsTSubstring<char16_t>') cannot bind to a temporary of type 'Span<element_type, dynamic_extent>' (aka 'Span<char16_t, dynamic_extent>') [clang-tidy: clang-diagnostic-error]xpcom/string/nsReadableUtils.h:422Error: No member named 'EnsureUtf16ValiditySpan' in namespace 'mozilla'; did you mean 'EnsureUTF16Validity'? [clang-tidy: clang-diagnostic-error]xpcom/string/nsReadableUtils.h:411Error: No member named 'Utf16ValidUpTo' in namespace 'mozilla' [clang-tidy: clang-diagnostic-error]xpcom/string/nsTSubstring.h:22Error: "Using XPCOM strings is limited to code linked into libxul." [clang-tidy: clang-diagnostic-error]xpcom/threads/TimerThreadMonitor.h:72Error: Deleted member function should be public [clang-tidy: modernize-use-equals-delete,-warnings-as-errors]
Checker reliability is high, meaning that the false positive ratio is low.xpcom/threads/TimerThreadMonitor.h:71Error: Deleted member function should be public [clang-tidy: modernize-use-equals-delete,-warnings-as-errors]
Checker reliability is high, meaning that the false positive ratio is low.xpcom/threads/TimerThreadMonitor.h:68Error: Deleted member function should be public [clang-tidy: modernize-use-equals-delete,-warnings-as-errors]
Checker reliability is high, meaning that the false positive ratio is low.xpcom/threads/TimerThreadMonitor.h:69Error: Deleted member function should be public [clang-tidy: modernize-use-equals-delete,-warnings-as-errors]
Checker reliability is high, meaning that the false positive ratio is low.xpcom/threads/TimerThreadMonitor.h:70Error: Deleted member function should be public [clang-tidy: modernize-use-equals-delete,-warnings-as-errors]
Checker reliability is high, meaning that the false positive ratio is low.
0df9b53 to
ab5b802
Compare
… data on Android Automotive OS
ab5b802 to
9d05b03
Compare
This review is now deprecated.
|
Previous issues have been fixed. This pull request is 🆗 |
|
No new issues detected. This pull request is 🆗 |
| */ | ||
| class DefaultAutomotiveSensitiveDataAuthenticator( | ||
| private val isRequired: (Context) -> Boolean = ::isAutomotiveSensitiveDataAuthenticationRequired, | ||
| private val isDeviceLockSetUp: (Context) -> Boolean = { it.getSystemService<KeyguardManager>()?.isKeyguardSecure == true }, |
There was a problem hiding this comment.
nit: I think we have an existing helper for this? https://searchfox.org/firefox-main/rev/141096d7cd9e6f6118d94d79ea0c1777839efe9e/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/settings/logins/ui/BiometricAuthenticationUtils.kt#127
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| package org.mozilla.fenix.settings.biometric |
There was a problem hiding this comment.
🤔 Wdyt about moving this into it's own package? Seems like it might be cleaner to have Android Automotive specific functionality kept together? Doesn't have to be done as part of this task.
|
|
||
| @VisibleForTesting | ||
| internal fun isAutomotiveSensitiveDataAuthenticationRequired(context: Context): Boolean = | ||
| context.isAndroidAutomotiveAvailable() && FxNimbus.features.automotiveSensitiveData.value().enabled |
There was a problem hiding this comment.
nit: I wonder if it makes more sense to have the feature flag check first?
| onFailure: () -> Unit, | ||
| ) -> Unit = ::showDeviceLockPrompt, | ||
| ) : AutomotiveSensitiveDataAuthenticator { | ||
| override fun authenticate( |
There was a problem hiding this comment.
nit: can We add a space here, the code block is pretty dense
| override fun authenticate( | |
| override fun authenticate( |
| * open and the user can try again. | ||
| */ | ||
| @VisibleForTesting | ||
| internal class DeviceLockAuthenticationCallback( |
| private val isRequiredCheck: (Context) -> Boolean = ::isAutomotiveSensitiveDataAuthenticationRequired, | ||
| private val isDeviceLockSetUp: (Context) -> Boolean = ::isKeyguardSecure, |
There was a problem hiding this comment.
nit: I think these checks are being repeated in a few places now, e.g. BiometricUtils & BiometricAuthenticationUtils can we, and does it make sense to reuse them?
| ) : BiometricPrompt.AuthenticationCallback() { | ||
| override fun onAuthenticationSucceeded(result: BiometricPrompt.AuthenticationResult) = onSuccess() | ||
|
|
||
| override fun onAuthenticationError(errorCode: Int, errString: CharSequence) = onFailure() |
There was a problem hiding this comment.
nit: Can we log this error code and string?
| import org.robolectric.Shadows.shadowOf | ||
|
|
||
| @RunWith(RobolectricTestRunner::class) | ||
| class AutomotiveSensitiveDataAuthenticatorTest { |
There was a problem hiding this comment.
Non-blocking AI feedback that I believe is valid to improve the tests:
- Pin the "re-check every call" requirement. Bug 2081834 says that if the user removes their device lock later, the setup prompt shows again. Both "authenticating again" tests use a constant fake, so they would still pass if the authenticator cached the first result. Make the fake mutable and flip it between calls:
private var isDeviceLockSetUp = true @Test fun `GIVEN the device lock was removed WHEN authenticating again THEN the setup prompt is shown`() { val authenticator = createAuthenticator() // reads the var, not a constant authenticator.authenticate() onAuthenticationSuccess() isDeviceLockSetUp = false authenticator.authenticate() assertCounts(authenticationPrompt = 1, authenticated = 1, setupPrompt = 1)
- Test the default isDeviceLockSetUp. It is the only default with real logic and nothing exercises it. The sibling setup prompt test already drives this exact check through ShadowKeyguardManager.setIsKeyguardSecure, so the recipe is in the tree.
- Assert pass-through on the setup path. The authentication prompt fake discards activity and title. Capture them and assert, at least in the setup-then-authenticate test, that the second prompt gets the same activity and title the caller passed in. That is where a later refactor would most plausibly swap in a different title.
Adds
AutomotiveSensitiveDataAuthenticator, a shared check that sensitive data flows call before continuing on Android Automotive OS when theautomotive-sensitive-dataNimbus feature is enabled. If no device lock is set up, it shows the setup prompt from bug 2081830; otherwise it asks the user to authenticate. The action only continues on success.Try: https://treeherder.mozilla.org/jobs?repo=try&revision=1979e56683c11cd76f9ae6e568037711c000f1d5
Lando: link
Bugzilla: bug 2081834
🚫 This pull request has 2 blockers.