From 2095efe45ddae3959ad6a48c38365ac4883b29fa Mon Sep 17 00:00:00 2001 From: Firstyear Date: Tue, 28 Mar 2023 12:42:06 +1000 Subject: [PATCH] Improve string validation (#1497) --- server/lib/src/value.rs | 142 ++++++++++++++++++++++++----- server/lib/src/valueset/binary.rs | 4 +- server/lib/src/valueset/cred.rs | 16 +++- server/lib/src/valueset/iname.rs | 6 +- server/lib/src/valueset/iutf8.rs | 4 +- server/lib/src/valueset/mod.rs | 3 +- server/lib/src/valueset/session.rs | 4 +- server/lib/src/valueset/spn.rs | 7 +- server/lib/src/valueset/ssh.rs | 8 +- server/lib/src/valueset/utf8.rs | 4 +- 10 files changed, 161 insertions(+), 37 deletions(-) diff --git a/server/lib/src/value.rs b/server/lib/src/value.rs index 098f53b68..2e2de026b 100644 --- a/server/lib/src/value.rs +++ b/server/lib/src/value.rs @@ -54,19 +54,32 @@ lazy_static! { m.insert("administrator"); m }; + + /// Only lowercase+numbers, with limited chars. pub static ref INAME_RE: Regex = { #[allow(clippy::expect_used)] Regex::new("^[a-z][a-z0-9-_\\.]+$").expect("Invalid Iname regex found") - // Only lowercase+numbers, with limited chars. }; pub static ref NSUNIQUEID_RE: Regex = { #[allow(clippy::expect_used)] Regex::new("^[0-9a-fA-F]{8}-[0-9a-fA-F]{8}-[0-9a-fA-F]{8}-[0-9a-fA-F]{8}$").expect("Invalid Nsunique regex found") }; + + /// Must not contain whitespace. pub static ref OAUTHSCOPE_RE: Regex = { #[allow(clippy::expect_used)] Regex::new("^[0-9a-zA-Z_]+$").expect("Invalid oauthscope regex found") - // Must not contain whitespace. + }; + + pub static ref SINGLELINE_RE: Regex = { + #[allow(clippy::expect_used)] + Regex::new("[\n\r\t]").expect("Invalid singleline regex found") + }; + + pub static ref ESCAPES_RE: Regex = { + #[allow(clippy::expect_used)] + Regex::new(r"\x1b\[([\x30-\x3f]*[\x20-\x2f]*[\x40-\x7e])") + .expect("Invalid escapes regex found") }; } @@ -337,7 +350,6 @@ pub enum PartialValue { UiHint(UiHint), Passkey(Uuid), DeviceKey(Uuid), - TrustedDeviceEnrollment(Uuid), // The label, if any. } @@ -707,7 +719,6 @@ impl PartialValue { PartialValue::Address(a) => a.to_string(), PartialValue::PhoneNumber(a) => a.to_string(), PartialValue::IntentToken(u) => u.clone(), - PartialValue::TrustedDeviceEnrollment(u) => u.as_hyphenated().to_string(), PartialValue::UiHint(u) => (*u as u16).to_string(), } } @@ -847,7 +858,6 @@ pub enum Value { Passkey(Uuid, String, PasskeyV4), DeviceKey(Uuid, String, DeviceKeyV4), - TrustedDeviceEnrollment(Uuid), Session(Uuid, Session), ApiToken(Uuid, ApiToken), Oauth2Session(Uuid, Oauth2Session), @@ -1546,13 +1556,6 @@ impl Value { } } - pub fn to_trusteddeviceenrollment(self) -> Option<(Uuid, ())> { - match self { - Value::TrustedDeviceEnrollment(u) => Some((u, ())), - _ => None, - } - } - pub fn to_session(self) -> Option<(Uuid, Session)> { match self { Value::Session(u, s) => Some((u, s)), @@ -1591,34 +1594,83 @@ impl Value { } } - // !!! relocate to value set !!! pub(crate) fn validate(&self) -> bool { // Validate that extra-data constraints on the type exist and are // valid. IE json filter is really a filter, or cred types have supplemental // data. match &self { - Value::Iname(s) => Value::validate_iname(s), - /* - Value::Cred(_) => match &self.data { - Some(v) => matches!(v.as_ref(), DataValue::Cred(_)), - None => false, - }, - */ - Value::SshKey(_, key) => SshPublicKey::from_string(key).is_ok(), + // String security is required here + Value::Utf8(s) + | Value::Iutf8(s) + | Value::Cred(s, _) + | Value::PublicBinary(s, _) + | Value::IntentToken(s, _) + | Value::Passkey(_, s, _) + | Value::DeviceKey(_, s, _) + | Value::TotpSecret(s, _) => { + Value::validate_str_escapes(s) && Value::validate_singleline(s) + } + + Value::Spn(a, b) => { + Value::validate_str_escapes(a) + && Value::validate_str_escapes(b) + && Value::validate_singleline(a) + && Value::validate_singleline(b) + } + + Value::Iname(s) => { + Value::validate_str_escapes(s) + && Value::validate_iname(s) + && Value::validate_singleline(s) + } + + Value::SshKey(s, key) => { + SshPublicKey::from_string(key).is_ok() + && Value::validate_str_escapes(s) + && Value::validate_singleline(s) + } + + Value::ApiToken(_, at) => { + Value::validate_str_escapes(&at.label) && Value::validate_singleline(&at.label) + } + + // These have stricter validators so not needed. Value::Nsuniqueid(s) => NSUNIQUEID_RE.is_match(s), Value::DateTime(odt) => odt.offset() == time::UtcOffset::UTC, Value::EmailAddress(mail, _) => validator::validate_email(mail.as_str()), - // PartialValue::Url validated through parsing. Value::OauthScope(s) => OAUTHSCOPE_RE.is_match(s), Value::OauthScopeMap(_, m) => m.iter().all(|s| OAUTHSCOPE_RE.is_match(s)), - _ => true, + + Value::PhoneNumber(_, _) => true, + Value::Address(_) => true, + + Value::Uuid(_) + | Value::Bool(_) + | Value::Syntax(_) + | Value::Index(_) + | Value::Refer(_) + | Value::JsonFilt(_) + | Value::SecretValue(_) + | Value::Uint32(_) + | Value::Url(_) + | Value::Cid(_) + | Value::PrivateBinary(_) + | Value::RestrictedString(_) + | Value::JwsKeyEs256(_) + | Value::Session(_, _) + | Value::Oauth2Session(_, _) + | Value::JwsKeyRs256(_) + | Value::UiHint(_) => true, } } pub(crate) fn validate_iname(s: &str) -> bool { match Uuid::parse_str(s) { // It is a uuid, disallow. - Ok(_) => false, + Ok(_) => { + warn!("iname values may not contain uuids"); + false + } // Not a uuid, check it against the re. Err(_) => { if !INAME_RE.is_match(s) { @@ -1633,6 +1685,31 @@ impl Value { } } } + + pub(crate) fn validate_singleline(s: &str) -> bool { + if !SINGLELINE_RE.is_match(s) { + true + } else { + warn!( + "value contains invalid whitespace chars forbidden by \"{}\"", + *SINGLELINE_RE + ); + false + } + } + + pub(crate) fn validate_str_escapes(s: &str) -> bool { + // Look for and prevent certain types of string escapes and injections. + if !ESCAPES_RE.is_match(s) { + true + } else { + warn!( + "value contains invalid escape chars forbidden by \"{}\"", + *ESCAPES_RE + ); + false + } + } } #[cfg(test)] @@ -1867,4 +1944,21 @@ mod tests { assert!(val2.is_some()); assert!(val3.is_some()); } + + #[test] + fn test_singleline() { + assert!(Value::validate_singleline("no new lines")); + + assert!(!Value::validate_singleline("contains a \n new line")); + assert!(!Value::validate_singleline("contains a \r return feed")); + assert!(!Value::validate_singleline("contains a \t tab char")); + } + + #[test] + fn test_str_escapes() { + assert!(Value::validate_str_escapes("safe str")); + assert!(Value::validate_str_escapes("🙃 emoji are 👍")); + + assert!(!Value::validate_str_escapes("naughty \x1b[31mred")); + } } diff --git a/server/lib/src/valueset/binary.rs b/server/lib/src/valueset/binary.rs index d52a80bdf..fb6389c8c 100644 --- a/server/lib/src/valueset/binary.rs +++ b/server/lib/src/valueset/binary.rs @@ -245,7 +245,9 @@ impl ValueSetT for ValueSetPublicBinary { } fn validate(&self, _schema_attr: &SchemaAttribute) -> bool { - true + self.map + .iter() + .all(|(s, _)| Value::validate_str_escapes(s) && Value::validate_singleline(s)) } fn to_proto_string_clone_iter(&self) -> Box + '_> { diff --git a/server/lib/src/valueset/cred.rs b/server/lib/src/valueset/cred.rs index 50a319be0..3ccbe8a7b 100644 --- a/server/lib/src/valueset/cred.rs +++ b/server/lib/src/valueset/cred.rs @@ -119,7 +119,9 @@ impl ValueSetT for ValueSetCredential { } fn validate(&self, _schema_attr: &SchemaAttribute) -> bool { - true + self.map + .iter() + .all(|(s, _)| Value::validate_str_escapes(s) && Value::validate_singleline(s)) } fn to_proto_string_clone_iter(&self) -> Box + '_> { @@ -333,7 +335,9 @@ impl ValueSetT for ValueSetIntentToken { } fn validate(&self, _schema_attr: &SchemaAttribute) -> bool { - true + self.map + .iter() + .all(|(s, _)| Value::validate_str_escapes(s) && Value::validate_singleline(s)) } fn to_proto_string_clone_iter(&self) -> Box + '_> { @@ -540,7 +544,9 @@ impl ValueSetT for ValueSetPasskey { } fn validate(&self, _schema_attr: &SchemaAttribute) -> bool { - true + self.map + .iter() + .all(|(_, (s, _))| Value::validate_str_escapes(s) && Value::validate_singleline(s)) } fn to_proto_string_clone_iter(&self) -> Box + '_> { @@ -724,7 +730,9 @@ impl ValueSetT for ValueSetDeviceKey { } fn validate(&self, _schema_attr: &SchemaAttribute) -> bool { - true + self.map + .iter() + .all(|(_, (s, _))| Value::validate_str_escapes(s) && Value::validate_singleline(s)) } fn to_proto_string_clone_iter(&self) -> Box + '_> { diff --git a/server/lib/src/valueset/iname.rs b/server/lib/src/valueset/iname.rs index 2369828f2..2104afee9 100644 --- a/server/lib/src/valueset/iname.rs +++ b/server/lib/src/valueset/iname.rs @@ -102,7 +102,11 @@ impl ValueSetT for ValueSetIname { } fn validate(&self, _schema_attr: &SchemaAttribute) -> bool { - self.set.iter().all(|s| Value::validate_iname(s.as_str())) + self.set.iter().all(|s| { + Value::validate_str_escapes(s) + && Value::validate_singleline(s) + && Value::validate_iname(s.as_str()) + }) } fn to_proto_string_clone_iter(&self) -> Box + '_> { diff --git a/server/lib/src/valueset/iutf8.rs b/server/lib/src/valueset/iutf8.rs index d714928d2..5c25b13b4 100644 --- a/server/lib/src/valueset/iutf8.rs +++ b/server/lib/src/valueset/iutf8.rs @@ -103,7 +103,9 @@ impl ValueSetT for ValueSetIutf8 { } fn validate(&self, _schema_attr: &SchemaAttribute) -> bool { - true + self.set + .iter() + .all(|s| Value::validate_str_escapes(s) && Value::validate_singleline(s)) } fn to_proto_string_clone_iter(&self) -> Box + '_> { diff --git a/server/lib/src/valueset/mod.rs b/server/lib/src/valueset/mod.rs index c3111619b..7bf213246 100644 --- a/server/lib/src/valueset/mod.rs +++ b/server/lib/src/valueset/mod.rs @@ -583,7 +583,6 @@ pub fn from_result_value_iter( | Value::Passkey(_, _, _) | Value::DeviceKey(_, _, _) | Value::TotpSecret(_, _) - | Value::TrustedDeviceEnrollment(_) | Value::Session(_, _) | Value::ApiToken(_, _) | Value::Oauth2Session(_, _) @@ -646,7 +645,7 @@ pub fn from_value_iter(mut iter: impl Iterator) -> Result ValueSetOauth2Session::new(u, m), Value::UiHint(u) => ValueSetUiHint::new(u), Value::TotpSecret(l, t) => ValueSetTotpSecret::new(l, t), - Value::PhoneNumber(_, _) | Value::TrustedDeviceEnrollment(_) => { + Value::PhoneNumber(_, _) => { debug_assert!(false); return Err(OperationError::InvalidValueState); } diff --git a/server/lib/src/valueset/session.rs b/server/lib/src/valueset/session.rs index f47d369a7..51ecb1fb7 100644 --- a/server/lib/src/valueset/session.rs +++ b/server/lib/src/valueset/session.rs @@ -1088,7 +1088,9 @@ impl ValueSetT for ValueSetApiToken { } fn validate(&self, _schema_attr: &SchemaAttribute) -> bool { - true + self.map.iter().all(|(_, at)| { + Value::validate_str_escapes(&at.label) && Value::validate_singleline(&at.label) + }) } fn to_proto_string_clone_iter(&self) -> Box + '_> { diff --git a/server/lib/src/valueset/spn.rs b/server/lib/src/valueset/spn.rs index 8e8a36590..90989361d 100644 --- a/server/lib/src/valueset/spn.rs +++ b/server/lib/src/valueset/spn.rs @@ -96,7 +96,12 @@ impl ValueSetT for ValueSetSpn { } fn validate(&self, _schema_attr: &SchemaAttribute) -> bool { - true + self.set.iter().all(|(a, b)| { + Value::validate_str_escapes(a) + && Value::validate_str_escapes(b) + && Value::validate_singleline(a) + && Value::validate_singleline(b) + }) } fn to_proto_string_clone_iter(&self) -> Box + '_> { diff --git a/server/lib/src/valueset/ssh.rs b/server/lib/src/valueset/ssh.rs index f8edfb3a7..f7dedfe87 100644 --- a/server/lib/src/valueset/ssh.rs +++ b/server/lib/src/valueset/ssh.rs @@ -7,6 +7,8 @@ use crate::repl::proto::ReplAttrV1; use crate::schema::SchemaAttribute; use crate::valueset::{DbValueSetV2, ValueSet}; +use sshkeys::PublicKey as SshPublicKey; + #[derive(Debug, Clone)] pub struct ValueSetSshKey { map: BTreeMap, @@ -102,7 +104,11 @@ impl ValueSetT for ValueSetSshKey { } fn validate(&self, _schema_attr: &SchemaAttribute) -> bool { - true + self.map.iter().all(|(s, key)| { + SshPublicKey::from_string(key).is_ok() + && Value::validate_str_escapes(s) + && Value::validate_singleline(s) + }) } fn to_proto_string_clone_iter(&self) -> Box + '_> { diff --git a/server/lib/src/valueset/utf8.rs b/server/lib/src/valueset/utf8.rs index 19916a07d..f1c2f82df 100644 --- a/server/lib/src/valueset/utf8.rs +++ b/server/lib/src/valueset/utf8.rs @@ -88,7 +88,9 @@ impl ValueSetT for ValueSetUtf8 { } fn validate(&self, _schema_attr: &SchemaAttribute) -> bool { - true + self.set + .iter() + .all(|s| Value::validate_str_escapes(s) && Value::validate_singleline(s)) } fn to_proto_string_clone_iter(&self) -> Box + '_> {