From b9e01316c296d08658a76955c28cbf8550321096 Mon Sep 17 00:00:00 2001 From: LucaCappelletti94 Date: Sun, 8 Nov 2026 22:36:38 +0100 Subject: [PATCH] Validate the supplied store divider --- src/by_store/store.rs | 35 ++++++++++++++++++++------ src/by_store/vault.rs | 5 ++-- src/tests/store_tests.rs | 54 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/src/by_store/store.rs b/src/by_store/store.rs index d54519c..4d957f3 100644 --- a/src/by_store/store.rs +++ b/src/by_store/store.rs @@ -90,10 +90,9 @@ impl StoreConfig { config.filename = format!("keyring-{}", config.name); } if let Some(divider) = mods.get("divider") { - // Vault dividers must have a non-alphabetic character so that the - // vault's config key is guaranteed not to match any credential's key. - if config.divider.chars().all(char::is_alphanumeric) { - let err = "must contain a non-alphabetic character".to_string(); + // A non-alphanumeric divider separates credential keys from the config key. + if divider.chars().all(char::is_alphanumeric) { + let err = "must contain a non-alphanumeric character".to_string(); return Err(Error::Invalid("divider".to_string(), err)); } config.divider = divider.to_string(); @@ -164,7 +163,7 @@ impl Store { /// The value of `divider` defaults to `\u{feff}@\u{feff}` which, /// when printed as part of a string, looks like `@` because /// the BOM character is considered a non-spacing word-joining - /// character. The divider _must_ contain a non-alphabetic character. + /// character. The divider _must_ contain a non-alphanumeric character. /// /// For `user-auth-required` and `user-auth-timeout`, see /// [the module docs](super#user-authentication). @@ -395,18 +394,40 @@ mod tests { #[test] fn user_auth_timeout_must_fit_the_keystore_range() { - for timeout in ["-1", "2147483648", "soon"] { + for timeout in ["-1", "2147483648", "4294967295", "4294967296", "soon"] { assert_invalid( &[NAME, REQUIRED, ("user-auth-timeout", timeout)], "user-auth-timeout", ); } - for (timeout, seconds) in [("0", 0), ("2147483647", 2147483647)] { + for (timeout, seconds) in [("0", 0), ("1", 1), ("2147483647", 2147483647)] { let config = parse(&[NAME, REQUIRED, ("user-auth-timeout", timeout)]).unwrap(); assert_eq!(config.user_auth_timeout, Some(seconds)); } } + #[test] + fn divider_rejects_alphanumeric_and_empty() { + for divider in ["abc", "abc123", "123", "é1", ""] { + assert_invalid(&[NAME, ("divider", divider)], "divider"); + } + } + + #[test] + fn divider_accepts_punctuation() { + for divider in ["@", "~", "a@b"] { + let config = parse(&[NAME, ("divider", divider)]).unwrap(); + assert_eq!(config.divider, divider); + } + } + + #[test] + fn unknown_keys_and_malformed_values_are_rejected() { + assert_invalid(&[NAME, ("color", "blue")], "color"); + assert_invalid(&[("name", "")], "name"); + assert_invalid(&[NAME, ("user-auth-required", "yes")], "user-auth-required"); + } + #[test] fn stored_config_without_user_auth_keeps_its_earlier_format() { let stored = r#"{"name":"vault","filename":"keyring-vault","divider":"@"}"#; diff --git a/src/by_store/vault.rs b/src/by_store/vault.rs index 69dcbc1..c337b4a 100644 --- a/src/by_store/vault.rs +++ b/src/by_store/vault.rs @@ -217,10 +217,9 @@ impl Vault { } fn new(config: &StoreConfig) -> Result { - // Vault dividers must have a non-alphabetic character so that the - // vault's config key is guaranteed not to match any credential's key. + // A non-alphanumeric divider separates credential keys from the config key. if config.divider.chars().all(char::is_alphanumeric) { - let err = "must contain a non-alphabetic character".to_string(); + let err = "must contain a non-alphanumeric character".to_string(); return Err(Error::Invalid("divider".to_string(), err)); } log::debug!("Creating new vault with config {config:?}"); diff --git a/src/tests/store_tests.rs b/src/tests/store_tests.rs index 572b51e..f898431 100644 --- a/src/tests/store_tests.rs +++ b/src/tests/store_tests.rs @@ -6,6 +6,7 @@ use android_log_sys::LogPriority; use keyring_core::{Entry, api::CredentialStoreApi}; use super::bad_result; +use crate::shared_preferences::Context; pub fn run_tests() -> (usize, usize) { let testing = [ @@ -23,6 +24,8 @@ pub fn run_tests() -> (usize, usize) { ("exists", exists), ("configuration_mismatch", configuration_mismatch), ("default_store", default_store), + ("oversized_auth_timeout", oversized_auth_timeout), + ("direct_invalid_divider", direct_invalid_divider), ] .iter() // user_auth_timeout needs a secure lock screen and a device unlocked in the last 300 seconds. @@ -461,3 +464,54 @@ fn default_store() -> keyring_core::Result<()> { } Ok(()) } + +fn oversized_auth_timeout() -> keyring_core::Result<()> { + let name = "oversized-timeout-test"; + let config = crate::by_store::store::StoreConfig { + name: name.to_string(), + filename: format!("keyring-{name}"), + divider: "@".to_string(), + user_auth_timeout: Some(u32::MAX), + }; + match crate::Store::new_with_store_config(config) { + Err(keyring_core::Error::Invalid(key, _)) if key == "user-auth-timeout" => {} + other => { + return bad_result( + "new_with_store_config", + &format!("Invalid(user-auth-timeout), got {other:?}"), + ); + } + } + // The parser cannot construct this configuration for `Store::delete`. + let (vm, context) = crate::android_context()?; + let mut env = vm.attach_current_thread().expect("attach_current_thread"); + let ctx = Context::from_raw(context); + let filename = format!("keyring-{name}"); + ctx.delete_shared_preferences(&mut env, &filename) + .expect("delete_shared_preferences"); + Ok(()) +} + +fn direct_invalid_divider() -> keyring_core::Result<()> { + let name = "direct-divider-test"; + let config = crate::by_store::store::StoreConfig { + name: name.to_string(), + filename: format!("keyring-{name}"), + divider: "abc".to_string(), + ..Default::default() + }; + match crate::Store::new_with_store_config(config) { + Err(keyring_core::Error::Invalid(key, _)) if key == "divider" => {} + other => { + return bad_result( + "new_with_store_config", + &format!("Invalid(divider), got {other:?}"), + ); + } + } + let valid = HashMap::from([("name", name), ("divider", "@")]); + if crate::Store::exists(&valid)? { + return bad_result("exists", "false after a rejected divider"); + } + Ok(()) +}