mirror of
https://github.com/sudosylabs/vnidrop.git
synced 2026-08-12 05:29:57 +02:00
fix(android): unblock protected-core startup on Android
Use flock for profile locks, hash secret record filenames under NAME_MAX, and dismiss the starting overlay after the first init attempt. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -70,7 +70,11 @@ impl AndroidSecureSecretStore {
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn record_path(&self, handle: &SecretHandle) -> PathBuf {
|
fn record_path(&self, handle: &SecretHandle) -> PathBuf {
|
||||||
let encoded = HEXLOWER.encode(handle.as_str().as_bytes());
|
// Hash the handle for the on-disk name. Scoped handles are longer than
|
||||||
|
// Linux/Android NAME_MAX when hex-encoded, and the handle is already
|
||||||
|
// authenticated inside the record body.
|
||||||
|
let digest = blake3::hash(handle.as_str().as_bytes());
|
||||||
|
let encoded = HEXLOWER.encode(digest.as_bytes());
|
||||||
self.records_dir
|
self.records_dir
|
||||||
.join(format!("{encoded}.{RECORD_EXTENSION}"))
|
.join(format!("{encoded}.{RECORD_EXTENSION}"))
|
||||||
}
|
}
|
||||||
@@ -192,16 +196,12 @@ impl SecureSecretStore for AndroidSecureSecretStore {
|
|||||||
if path.extension().and_then(|value| value.to_str()) != Some(RECORD_EXTENSION) {
|
if path.extension().and_then(|value| value.to_str()) != Some(RECORD_EXTENSION) {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
let stem = path
|
let mut bytes = Vec::new();
|
||||||
.file_stem()
|
File::open(&path)
|
||||||
.and_then(|value| value.to_str())
|
.map_err(map_io_error)?
|
||||||
.ok_or(SecureSecretStoreError::Corrupted)?;
|
.read_to_end(&mut bytes)
|
||||||
let decoded = HEXLOWER
|
.map_err(map_io_error)?;
|
||||||
.decode(stem.as_bytes())
|
handles.push(decode_handle_from_record(&bytes)?);
|
||||||
.map_err(|_| SecureSecretStoreError::Corrupted)?;
|
|
||||||
let handle =
|
|
||||||
String::from_utf8(decoded).map_err(|_| SecureSecretStoreError::Corrupted)?;
|
|
||||||
handles.push(SecretHandle(handle));
|
|
||||||
}
|
}
|
||||||
handles.sort_by(|left, right| left.as_str().cmp(right.as_str()));
|
handles.sort_by(|left, right| left.as_str().cmp(right.as_str()));
|
||||||
Ok(handles)
|
Ok(handles)
|
||||||
@@ -242,11 +242,32 @@ fn decode_record(
|
|||||||
bytes: &[u8],
|
bytes: &[u8],
|
||||||
expected_handle: &SecretHandle,
|
expected_handle: &SecretHandle,
|
||||||
) -> Result<AndroidSealedValue, SecureSecretStoreError> {
|
) -> Result<AndroidSealedValue, SecureSecretStoreError> {
|
||||||
|
let (handle, state, nonce, ciphertext) = parse_record(bytes)?;
|
||||||
|
if handle.as_str() != expected_handle.as_str() || state != RECORD_SEALED {
|
||||||
|
return Err(SecureSecretStoreError::Corrupted);
|
||||||
|
}
|
||||||
|
if nonce.is_empty() || ciphertext.is_empty() {
|
||||||
|
return Err(SecureSecretStoreError::Corrupted);
|
||||||
|
}
|
||||||
|
Ok(AndroidSealedValue { nonce, ciphertext })
|
||||||
|
}
|
||||||
|
|
||||||
|
fn decode_handle_from_record(bytes: &[u8]) -> Result<SecretHandle, SecureSecretStoreError> {
|
||||||
|
let (handle, _state, _nonce, _ciphertext) = parse_record(bytes)?;
|
||||||
|
Ok(handle)
|
||||||
|
}
|
||||||
|
|
||||||
|
fn parse_record(
|
||||||
|
bytes: &[u8],
|
||||||
|
) -> Result<(SecretHandle, u8, Vec<u8>, Vec<u8>), SecureSecretStoreError> {
|
||||||
const HEADER_LEN: usize = 8 + 1 + 2 + 2 + 4;
|
const HEADER_LEN: usize = 8 + 1 + 2 + 2 + 4;
|
||||||
if bytes.len() < HEADER_LEN || &bytes[..8] != RECORD_MAGIC {
|
if bytes.len() < HEADER_LEN || &bytes[..8] != RECORD_MAGIC {
|
||||||
return Err(SecureSecretStoreError::Corrupted);
|
return Err(SecureSecretStoreError::Corrupted);
|
||||||
}
|
}
|
||||||
let state = bytes[8];
|
let state = bytes[8];
|
||||||
|
if state != RECORD_STAGED && state != RECORD_SEALED {
|
||||||
|
return Err(SecureSecretStoreError::Corrupted);
|
||||||
|
}
|
||||||
let handle_len = usize::from(u16::from_be_bytes([bytes[9], bytes[10]]));
|
let handle_len = usize::from(u16::from_be_bytes([bytes[9], bytes[10]]));
|
||||||
let nonce_len = usize::from(u16::from_be_bytes([bytes[11], bytes[12]]));
|
let nonce_len = usize::from(u16::from_be_bytes([bytes[11], bytes[12]]));
|
||||||
let ciphertext_len = usize::try_from(u32::from_be_bytes([
|
let ciphertext_len = usize::try_from(u32::from_be_bytes([
|
||||||
@@ -258,23 +279,19 @@ fn decode_record(
|
|||||||
.and_then(|value| value.checked_add(nonce_len))
|
.and_then(|value| value.checked_add(nonce_len))
|
||||||
.and_then(|value| value.checked_add(ciphertext_len))
|
.and_then(|value| value.checked_add(ciphertext_len))
|
||||||
.ok_or(SecureSecretStoreError::Corrupted)?;
|
.ok_or(SecureSecretStoreError::Corrupted)?;
|
||||||
if bytes.len() != expected_len || state != RECORD_SEALED {
|
if bytes.len() != expected_len {
|
||||||
return Err(SecureSecretStoreError::Corrupted);
|
return Err(SecureSecretStoreError::Corrupted);
|
||||||
}
|
}
|
||||||
let handle_end = HEADER_LEN + handle_len;
|
let handle_end = HEADER_LEN + handle_len;
|
||||||
let handle = std::str::from_utf8(&bytes[HEADER_LEN..handle_end])
|
let handle = std::str::from_utf8(&bytes[HEADER_LEN..handle_end])
|
||||||
.map_err(|_| SecureSecretStoreError::Corrupted)?;
|
.map_err(|_| SecureSecretStoreError::Corrupted)?;
|
||||||
if handle != expected_handle.as_str() {
|
|
||||||
return Err(SecureSecretStoreError::Corrupted);
|
|
||||||
}
|
|
||||||
let nonce_end = handle_end + nonce_len;
|
let nonce_end = handle_end + nonce_len;
|
||||||
if nonce_len == 0 || ciphertext_len == 0 {
|
Ok((
|
||||||
return Err(SecureSecretStoreError::Corrupted);
|
SecretHandle(handle.to_string()),
|
||||||
}
|
state,
|
||||||
Ok(AndroidSealedValue {
|
bytes[handle_end..nonce_end].to_vec(),
|
||||||
nonce: bytes[handle_end..nonce_end].to_vec(),
|
bytes[nonce_end..].to_vec(),
|
||||||
ciphertext: bytes[nonce_end..].to_vec(),
|
))
|
||||||
})
|
|
||||||
}
|
}
|
||||||
|
|
||||||
fn map_io_error(error: std::io::Error) -> SecureSecretStoreError {
|
fn map_io_error(error: std::io::Error) -> SecureSecretStoreError {
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
use std::{
|
use std::{
|
||||||
fs::{File, OpenOptions},
|
fs::{File, OpenOptions},
|
||||||
|
io,
|
||||||
path::Path,
|
path::Path,
|
||||||
sync::Arc,
|
sync::Arc,
|
||||||
};
|
};
|
||||||
@@ -29,13 +30,51 @@ pub(crate) fn lock_profile(app_data_dir: &Path) -> Result<ProfileLock, VnidropEr
|
|||||||
.write(true)
|
.write(true)
|
||||||
.open(app_data_dir.join("protected-secrets.lock"))
|
.open(app_data_dir.join("protected-secrets.lock"))
|
||||||
.map_err(VnidropError::filesystem)?;
|
.map_err(VnidropError::filesystem)?;
|
||||||
file.try_lock()
|
lock_exclusive_nonblocking(&file)?;
|
||||||
.map_err(|_| VnidropError::SecureStorageUnavailable {
|
|
||||||
reason: "another protected core is already using this profile".to_string(),
|
|
||||||
})?;
|
|
||||||
Ok(ProfileLock { _file: file })
|
Ok(ProfileLock { _file: file })
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Acquire an exclusive advisory lock without blocking.
|
||||||
|
///
|
||||||
|
/// Prefer `libc::flock` on Unix: Rust's `File::try_lock` still returns
|
||||||
|
/// `ErrorKind::Unsupported` on Android even though the kernel supports flock.
|
||||||
|
fn lock_exclusive_nonblocking(file: &File) -> Result<(), VnidropError> {
|
||||||
|
#[cfg(unix)]
|
||||||
|
{
|
||||||
|
use std::os::fd::AsRawFd;
|
||||||
|
let rc = unsafe { libc::flock(file.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) };
|
||||||
|
if rc == 0 {
|
||||||
|
return Ok(());
|
||||||
|
}
|
||||||
|
let err = io::Error::last_os_error();
|
||||||
|
return Err(match err.kind() {
|
||||||
|
io::ErrorKind::WouldBlock => VnidropError::SecureStorageUnavailable {
|
||||||
|
reason: "another protected core is already using this profile".to_string(),
|
||||||
|
},
|
||||||
|
_ => VnidropError::filesystem(err),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
#[cfg(windows)]
|
||||||
|
{
|
||||||
|
match file.try_lock() {
|
||||||
|
Ok(()) => Ok(()),
|
||||||
|
Err(err) if err.kind() == io::ErrorKind::WouldBlock => {
|
||||||
|
Err(VnidropError::SecureStorageUnavailable {
|
||||||
|
reason: "another protected core is already using this profile".to_string(),
|
||||||
|
})
|
||||||
|
}
|
||||||
|
Err(err) => Err(VnidropError::filesystem(err)),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
#[cfg(not(any(unix, windows)))]
|
||||||
|
{
|
||||||
|
let _ = file;
|
||||||
|
Err(VnidropError::SecureStorageUnavailable {
|
||||||
|
reason: "profile locking is unsupported on this platform".to_string(),
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Opens the profile marker without locking so in-process restart tests can
|
/// Opens the profile marker without locking so in-process restart tests can
|
||||||
/// reopen the same directory after dropping the previous core.
|
/// reopen the same directory after dropping the previous core.
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
|
|||||||
@@ -38,13 +38,33 @@ fn a_profile_allows_only_one_protected_core_mutator() {
|
|||||||
|
|
||||||
assert!(matches!(
|
assert!(matches!(
|
||||||
lock_profile(temp.path()),
|
lock_profile(temp.path()),
|
||||||
Err(VnidropError::SecureStorageUnavailable { .. })
|
Err(VnidropError::SecureStorageUnavailable { reason })
|
||||||
|
if reason.contains("already using this profile")
|
||||||
));
|
));
|
||||||
|
|
||||||
drop(first);
|
drop(first);
|
||||||
assert!(lock_profile(temp.path()).is_ok());
|
assert!(lock_profile(temp.path()).is_ok());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn profile_lock_maps_contention_not_generic_io_failures() {
|
||||||
|
// Regression: Android's std File::try_lock returns Unsupported; lock_profile
|
||||||
|
// must use flock and only treat WouldBlock as "already using this profile".
|
||||||
|
let temp = tempfile::tempdir().unwrap();
|
||||||
|
let held = lock_profile(temp.path()).unwrap();
|
||||||
|
match lock_profile(temp.path()) {
|
||||||
|
Err(VnidropError::SecureStorageUnavailable { reason }) => {
|
||||||
|
assert!(
|
||||||
|
reason.contains("already using this profile"),
|
||||||
|
"unexpected reason: {reason}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
Ok(_) => panic!("expected contended lock to fail"),
|
||||||
|
Err(other) => panic!("expected SecureStorageUnavailable, got {other:?}"),
|
||||||
|
}
|
||||||
|
drop(held);
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn reconciliation_is_scoped_to_one_application_profile() {
|
async fn reconciliation_is_scoped_to_one_application_profile() {
|
||||||
let root = tempfile::tempdir().unwrap();
|
let root = tempfile::tempdir().unwrap();
|
||||||
|
|||||||
@@ -178,3 +178,27 @@ fn failed_replacement_keeps_the_previous_secret_readable() {
|
|||||||
));
|
));
|
||||||
assert_eq!(store.get(&handle).unwrap(), original);
|
assert_eq!(store.get(&handle).unwrap(), original);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn scoped_handle_record_names_fit_linux_name_max() {
|
||||||
|
let (_directory, store, _keystore) = fixture();
|
||||||
|
// Mirrors ScopedSecretStore physical handles used on Android profiles.
|
||||||
|
let handle = secret_handle_for_test(&format!(
|
||||||
|
"vnidrop/v1/scope-{}/endpoint-identity/{}",
|
||||||
|
"a".repeat(64),
|
||||||
|
"b".repeat(36),
|
||||||
|
));
|
||||||
|
assert!(handle.as_str().len() > 100);
|
||||||
|
let path = store.record_path_for_test(&handle);
|
||||||
|
let file_name = path.file_name().and_then(|value| value.to_str()).unwrap();
|
||||||
|
assert!(
|
||||||
|
file_name.len() <= 255,
|
||||||
|
"record file name exceeds NAME_MAX: {} bytes ({file_name})",
|
||||||
|
file_name.len()
|
||||||
|
);
|
||||||
|
|
||||||
|
let material = SecretMaterial::new(vec![0x5a; TEST_SECRET_BYTES]).unwrap();
|
||||||
|
store.put(&handle, material.clone()).unwrap();
|
||||||
|
assert_eq!(store.list_handles().unwrap(), vec![handle.clone()]);
|
||||||
|
assert_eq!(store.get(&handle).unwrap(), material);
|
||||||
|
}
|
||||||
|
|||||||
@@ -255,7 +255,7 @@ fun App(
|
|||||||
windowChrome?.invoke()
|
windowChrome?.invoke()
|
||||||
val startingLabel = stringResource(Res.string.app_starting)
|
val startingLabel = stringResource(Res.string.app_starting)
|
||||||
AnimatedVisibility(
|
AnimatedVisibility(
|
||||||
visible = !sendCoreState.isInitialized,
|
visible = !sendCoreState.isInitialized && !appState.startupSettled,
|
||||||
enter = fadeIn(),
|
enter = fadeIn(),
|
||||||
exit = fadeOut(),
|
exit = fadeOut(),
|
||||||
) {
|
) {
|
||||||
|
|||||||
@@ -21,6 +21,8 @@ import kotlinx.coroutines.launch
|
|||||||
data class AppState(
|
data class AppState(
|
||||||
val destination: AppDestination = AppDestination.Send,
|
val destination: AppDestination = AppDestination.Send,
|
||||||
val themeMode: ThemeMode = ThemeMode.System,
|
val themeMode: ThemeMode = ThemeMode.System,
|
||||||
|
/** True after the first core initialize attempt finishes (success or failure). */
|
||||||
|
val startupSettled: Boolean = false,
|
||||||
)
|
)
|
||||||
|
|
||||||
class AppGraphViewModel(dependencies: AppDependencies) : ViewModel() {
|
class AppGraphViewModel(dependencies: AppDependencies) : ViewModel() {
|
||||||
@@ -46,6 +48,7 @@ class AppViewModel(
|
|||||||
viewModelScope.launch {
|
viewModelScope.launch {
|
||||||
val relaySettings = preferencesRepository.preferences.first().relaySettings
|
val relaySettings = preferencesRepository.preferences.first().relaySettings
|
||||||
repository.initialize(environment.defaultCoreDataDir, relaySettings).onFailure(messages::error)
|
repository.initialize(environment.defaultCoreDataDir, relaySettings).onFailure(messages::error)
|
||||||
|
_state.update { it.copy(startupSettled = true) }
|
||||||
}
|
}
|
||||||
viewModelScope.launch {
|
viewModelScope.launch {
|
||||||
preferencesRepository.preferences.collect { preferences ->
|
preferencesRepository.preferences.collect { preferences ->
|
||||||
|
|||||||
@@ -80,6 +80,7 @@ class ViewModelsTest {
|
|||||||
val viewModel = AppViewModel(environment(), core, preferences(), UiMessageController())
|
val viewModel = AppViewModel(environment(), core, preferences(), UiMessageController())
|
||||||
advanceUntilIdle()
|
advanceUntilIdle()
|
||||||
assertTrue(core.state.value.isInitialized)
|
assertTrue(core.state.value.isInitialized)
|
||||||
|
assertTrue(viewModel.state.value.startupSettled)
|
||||||
assertEquals(listOf(RelaySettings()), core.initializedRelaySettings)
|
assertEquals(listOf(RelaySettings()), core.initializedRelaySettings)
|
||||||
viewModel.selectDestination(AppDestination.Settings)
|
viewModel.selectDestination(AppDestination.Settings)
|
||||||
assertEquals(AppDestination.Settings, viewModel.state.value.destination)
|
assertEquals(AppDestination.Settings, viewModel.state.value.destination)
|
||||||
|
|||||||
Reference in New Issue
Block a user