Skip to content

Stricter origin parsing and validation #133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iinuwa wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Stricter origin parsing and validation #133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5cd7204
daemon: Use stronger types for origin checks
iinuwa Jan 23, 2026
ec338e9
daemon: More strictly parse hostname from HTTPS origins
iinuwa Jan 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions credentialsd/src/credential_service/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ mod test {
credential_service::usb::InProcessUsbHandler,
dbus::test::{DummyFlowServer, DummyUiServer},
model::CredentialRequest,
webauthn,
webauthn::{self, NavigationContext},
};
use credentialsd_common::model::Operation;

Expand Down Expand Up @@ -452,18 +452,13 @@ mod test {

fn create_credential_request() -> CredentialRequest {
let challenge = "Ox0AXQz7WUER7BGQFzvVrQbReTkS3sepVGj26qfUhhrWSarkDbGF4T4NuCY1aAwHYzOzKMJJ2YRSatetl0D9bQ";
let origin = "webauthn.io".to_string();
let is_cross_origin = false;
let client_data_json = webauthn::format_client_data_json(
Operation::Create,
challenge,
&origin,
is_cross_origin,
);
let origin = NavigationContext::SameOrigin("https://webauthn.io".parse().unwrap());
let client_data_json =
webauthn::format_client_data_json(Operation::Create, challenge, &origin);
let client_data_hash = webauthn::create_client_data_hash(&client_data_json);
let make_request = MakeCredentialRequest {
hash: client_data_hash,
origin: "webauthn.io".to_string(),
origin: "https://webauthn.io".to_string(),
relying_party: Ctap2PublicKeyCredentialRpEntity {
id: "webauthn.io".to_string(),
name: Some("webauthn.io".to_string()),
Expand Down
241 changes: 131 additions & 110 deletions credentialsd/src/dbus/gateway.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use crate::{
CredentialRequestController,
},
model::{CredentialRequest, CredentialResponse},
webauthn::Origin,
webauthn::{AppId, NavigationContext, Origin},
};

pub const SERVICE_NAME: &str = "xyz.iinuwa.credentialsd.Credentials";
Expand Down Expand Up @@ -224,26 +224,49 @@ impl<C: CredentialRequestController + Send + Sync + 'static> CredentialGateway<C
parent_window: Optional<WindowHandle>,
request: CreateCredentialRequest,
) -> Result<CreateCredentialResponse, Error> {
let (_origin, is_same_origin, _top_origin) =
check_origin(request.origin.as_deref(), request.is_same_origin)
.await
.map_err(Error::from)?;
// TODO: Add authorization check for privileged client.
let top_origin = if request.is_same_origin.unwrap_or_default() {
None
} else {
// TODO: Once we modify the models to convey the top-origin in cross origin requests to the UI, we can remove this error message.
// We should still reject cross-origin requests for conditionally-mediated requests.
tracing::warn!("Client attempted to issue cross-origin request for credentials, which are not supported by this platform.");
return Err(WebAuthnError::NotAllowedError.into());
};
let Some(origin) = request
.origin
.as_ref()
.map(|o| {
o.parse::<Origin>().map_err(|_| {
tracing::warn!("Invalid origin specified: {:?}", request.origin);
Error::SecurityError
})
})
.transpose()?
else {
tracing::warn!(
"Caller requested implicit origin, which is not yet implemented. Rejecting request."
);
return Err(Error::SecurityError);
};
let request_environment = check_origin_from_privileged_client(origin, top_origin)?;
if let ("publicKey", Some(_)) = (request.r#type.as_ref(), &request.public_key) {
if !is_same_origin {
// TODO: Once we modify the models to convey the top-origin in cross origin requests to the UI, we can remove this error message.
// We should still reject cross-origin requests for conditionally-mediated requests.
tracing::warn!("Client attempted to issue cross-origin request for credentials, which are not supported by this platform.");
return Err(WebAuthnError::NotAllowedError.into());
}
let (make_cred_request, client_data_json) =
create_credential_request_try_into_ctap2(&request).map_err(|e| {
if let WebAuthnError::TypeError = e {
tracing::error!(
"Could not parse passkey creation request. Rejecting request."
);
}
e
})?;
// TODO: assert that RP ID is bound to origin:
// - if RP ID is not set, set the RP ID to the origin's effective domain
// - if RP ID is set, assert that it matches origin's effective domain
// - if RP ID is set, but origin's effective domain doesn't match
// - query for related origins, if supported
// - fail if not supported, or if RP ID doesn't match any related origins.
let (make_cred_request, client_data_json) = create_credential_request_try_into_ctap2(
&request,
&request_environment,
)
.map_err(|e| {
if let WebAuthnError::TypeError = e {
tracing::error!("Could not parse passkey creation request. Rejecting request.");
Copy link
Collaborator

@msirringhaus msirringhaus Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is only copy&paste from what was before, but I find this logging a bit weird. We are rejecting the request for all other errors as well, don't we?

Copy link
Member Author

@iinuwa iinuwa Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we are. Are you saying that it's weird that we're not consistent, or specifically that it's weird that we're including "Rejecting request." here?

I think that it's useful for admins to know from the logs both what caused the error, and what action was taken. I usually try to put that on the same line if possible, so it's easy to find in the logs. But I do notice that we're not consistent in this method, nor is this method responsible for the decision to reject the request or not. I'd be fine with removing "Rejecting request" here and pushing that up into the higher-level method that actually returns the error back to D-Bus.

Copy link
Collaborator

@msirringhaus msirringhaus Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just wondering why we print a tracing for TypeError only, and not for all other errors. We tell the admin that we are rejecting the request for one specific error only, but silently (?) reject it in all other cases.

}
e
Copy link
Collaborator

@msirringhaus msirringhaus Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, very much nit picking but: I hate single letter variables :D Can we name this err instead?

})?;
if make_cred_request.algorithms.is_empty() {
tracing::info!("No supported algorithms given in request. Rejecting request.");
return Err(Error::NotSupportedError);
Expand Down Expand Up @@ -291,16 +314,33 @@ impl<C: CredentialRequestController + Send + Sync + 'static> CredentialGateway<C
parent_window: Optional<WindowHandle>,
request: GetCredentialRequest,
) -> Result<GetCredentialResponse, Error> {
let (_origin, is_same_origin, _top_origin) =
check_origin(request.origin.as_deref(), request.is_same_origin)
.await
.map_err(Error::from)?;
// TODO: Add authorization check for privileged client.
let top_origin = if request.is_same_origin.unwrap_or_default() {
None
} else {
// TODO: Once we modify the models to convey the top-origin in cross origin requests to the UI, we can remove this error message.
// We should still reject cross-origin requests for conditionally-mediated requests.
tracing::warn!("Client attempted to issue cross-origin request for credentials, which are not supported by this platform.");
return Err(WebAuthnError::NotAllowedError.into());
};
let Some(origin) = request
.origin
.as_ref()
.map(|o| {
o.parse::<Origin>().map_err(|_| {
tracing::warn!("Invalid origin specified: {:?}", request.origin);
Error::SecurityError
})
})
.transpose()?
else {
tracing::warn!(
"Caller requested implicit origin, which is not yet implemented. Rejecting request."
);
return Err(Error::SecurityError);
};
let request_env = check_origin_from_privileged_client(origin, top_origin)?;
if let ("publicKey", Some(_)) = (request.r#type.as_ref(), &request.public_key) {
if !is_same_origin {
// TODO: Once we modify the models to convey the top-origin in cross origin requests to the UI, we can remove this error message.
tracing::warn!("Client attempted to issue cross-origin request for credentials, which are not supported by this platform.");
return Err(WebAuthnError::NotAllowedError.into());
}
// Setup request

// TODO: assert that RP ID is bound to origin:
Expand All @@ -310,7 +350,7 @@ impl<C: CredentialRequestController + Send + Sync + 'static> CredentialGateway<C
// - query for related origins, if supported
// - fail if not supported, or if RP ID doesn't match any related origins.
let (get_cred_request, client_data_json) =
get_credential_request_try_into_ctap2(&request).map_err(|e| {
get_credential_request_try_into_ctap2(&request, &request_env).map_err(|e| {
tracing::error!("Could not parse passkey assertion request: {e:?}");
WebAuthnError::TypeError
})?;
Expand Down Expand Up @@ -371,7 +411,7 @@ async fn validate_app_details(
claimed_app_display_name: Option<String>,
claimed_origin: Option<String>,
claimed_top_origin: Option<String>,
) -> Result<(RequestingApplication, Origin), Error> {
) -> Result<(RequestingApplication, NavigationContext), Error> {
let Some(unique_name) = header.sender() else {
return Err(Error::SecurityError);
};
Expand All @@ -385,21 +425,40 @@ async fn validate_app_details(
return Err(Error::SecurityError);
}
// Now we can trust these app detail parameters.
let app_id = format!("app:{claimed_app_id}");
let Ok(app_id) = claimed_app_id.parse::<AppId>() else {
tracing::warn!("Invalid app ID passed: {claimed_app_id}");
return Err(Error::SecurityError);
};
let display_name = claimed_app_display_name.unwrap_or_default();

// Verify that the origin is valid for the given app ID.
let origin = check_origin_from_app(
&app_id,
claimed_origin.as_deref(),
claimed_top_origin.as_deref(),
)?;
let claimed_origin = claimed_origin
.map(|o| {
o.parse().map_err(|_| {
tracing::warn!("Invalid origin passed: {o}");
Error::SecurityError
})
})
.transpose()?;
let request_env = if let Some(claimed_origin) = claimed_origin {
let claimed_top_origin = claimed_top_origin
.map(|o| {
o.parse().map_err(|_| {
tracing::warn!("Invalid origin passed: {o}");
Error::SecurityError
})
})
.transpose()?;
check_origin_from_app(&app_id, claimed_origin, claimed_top_origin)?
} else {
NavigationContext::SameOrigin(Origin::AppId(app_id))
};
let app_details = RequestingApplication {
name: display_name,
path: app_id,
path: claimed_app_id,
pid,
};
Ok((app_details, origin))
Ok((app_details, request_env))
}

async fn should_trust_app_id(pid: u32) -> bool {
Expand Down Expand Up @@ -449,10 +508,10 @@ async fn should_trust_app_id(pid: u32) -> bool {
}

fn check_origin_from_app<'a>(
app_id: &str,
origin: Option<&str>,
top_origin: Option<&str>,
) -> Result<Origin, WebAuthnError> {
app_id: &AppId,
origin: Origin,
top_origin: Option<Origin>,
) -> Result<NavigationContext, WebAuthnError> {
let trusted_clients = [
"org.mozilla.firefox",
"xyz.iinuwa.credentialsd.DemoCredentialsUi",
Expand All @@ -461,75 +520,28 @@ fn check_origin_from_app<'a>(
if is_privileged_client {
check_origin_from_privileged_client(origin, top_origin)
} else {
Ok(Origin::AppId(app_id.to_string()))
Ok(NavigationContext::SameOrigin(Origin::AppId(app_id.clone())))
}
}

fn check_origin_from_privileged_client(
origin: Option<&str>,
top_origin: Option<&str>,
) -> Result<Origin, WebAuthnError> {
let origin = match (origin, top_origin) {
(Some(origin), top_origin) => {
if !origin.starts_with("https://") {
tracing::warn!(
"Caller requested non-HTTPS schemed origin, which is not supported."
);
return Err(WebAuthnError::SecurityError);
}
if let Some(top_origin) = top_origin {
if origin == top_origin {
Origin::SameOrigin(origin.to_string())
} else {
Origin::CrossOrigin((origin.to_string(), top_origin.to_string()))
}
origin: Origin,
top_origin: Option<Origin>,
) -> Result<NavigationContext, WebAuthnError> {
match (origin, top_origin) {
(origin @ Origin::Https { .. }, None) => Ok(NavigationContext::SameOrigin(origin)),
(origin @ Origin::Https { .. }, Some(top_origin @ Origin::Https { .. })) => {
if origin == top_origin {
Ok(NavigationContext::SameOrigin(origin))
} else {
Origin::SameOrigin(origin.to_string())
Ok(NavigationContext::CrossOrigin((origin, top_origin)))
}
}
(None, Some(_)) => {
tracing::warn!("Top origin cannot be set if origin is not set.");
_ => {
tracing::warn!("Caller requested non-HTTPS schemed origin, which is not supported.");
return Err(WebAuthnError::SecurityError);
}
(None, None) => {
tracing::warn!("No origin given. Rejecting request.");
return Err(WebAuthnError::SecurityError);
}
};

if let Origin::CrossOrigin(_) = origin {
tracing::warn!("Client attempted to issue cross-origin request for credentials, which are not supported by this platform.");
return Err(WebAuthnError::NotAllowedError);
};
Ok(origin)
}

async fn check_origin(
origin: Option<&str>,
is_same_origin: Option<bool>,
// TODO: Replace is_same_origin with explicit top_origin
// top_origin: Option<&str>,
) -> Result<(String, bool, String), WebAuthnError> {
let origin = if let Some(origin) = origin {
origin.to_string()
} else {
tracing::warn!(
"Caller requested implicit origin, which is not yet implemented. Rejecting request."
);
return Err(WebAuthnError::SecurityError);
};
if !origin.starts_with("https://") {
tracing::warn!("Caller requested non-HTTPS schemed origin, which is not supported.");
return Err(WebAuthnError::SecurityError);
}
let is_same_origin = is_same_origin.unwrap_or(false);
let top_origin = if is_same_origin {
origin.clone()
} else {
tracing::warn!("Client attempted to issue cross-origin request for credentials, which are not supported by this platform.");
return Err(WebAuthnError::NotAllowedError);
};
Ok((origin, true, top_origin))
}

#[allow(clippy::enum_variant_names)]
Expand Down Expand Up @@ -593,17 +605,26 @@ impl From<WebAuthnError> for Error {
mod test {
use credentialsd_common::model::WebAuthnError;

use crate::dbus::gateway::check_origin;
use crate::webauthn::{NavigationContext, Origin};

#[tokio::test]
async fn test_only_https_origins() {
let check = |origin: &'static str| async { check_origin(Some(origin), Some(true)).await };
use super::check_origin_from_privileged_client;
fn check_same_origin(origin: &str) -> Result<NavigationContext, WebAuthnError> {
let origin = origin.parse().unwrap();
check_origin_from_privileged_client(origin, None)
}

#[test]
fn test_only_https_origins() {
assert!(matches!(
check("https://example.com").await,
Ok((o, ..)) if o == "https://example.com"
));
check_same_origin("https://example.com"),
Ok(NavigationContext::SameOrigin(Origin::Https { host, .. })) if host == "example.com"
))
}

#[test]
fn test_privileged_client_cannot_set_http_origins() {
assert!(matches!(
check("http://example.com").await,
check_same_origin("http://example.com"),
Err(WebAuthnError::SecurityError)
));
}
Expand Down
Loading
Loading