Keep the typed server after browser sign-in, and open only https
Login Flow v2 saved the account under the `server` field of the poll response, not under the address the person typed. That field is the server's idea of its own URL. Behind a TLS-terminating proxy without `overwriteprotocol` (a common setup) it says http://, and the account then sent its app password in the clear on every request after that. The typed address, already upgraded to https by normalise_endpoint, has just carried the whole flow, so it is the one kept. The flow's other two URLs come from the server as well, and are now upgraded from http to https, and refused if they use any other scheme: - The login URL is handed to the OS to open. On Windows that is `rundll32 url.dll,FileProtocolHandler`, which runs a file: or UNC path rather than showing a web page, so a hostile server could launch a program when the user starts signing in. open_in_browser also refuses anything that is not https, as the last check before a process starts. - The poll endpoint is where the app password comes back from. The host is not checked. A server reached by its LAN address can answer with its public name, and refusing that would break a working setup without protecting anything: the account is stored under the typed address whatever the server says. Part of #65.
This commit is contained in:
@@ -22,6 +22,11 @@ pub struct LoginFlow {
|
||||
pub login_url: String,
|
||||
#[serde(rename = "poll")]
|
||||
pub poll: PollInfo,
|
||||
/// The server the flow was started against — the address the user typed,
|
||||
/// already normalised. Not part of the response: [`begin`] fills it in so
|
||||
/// [`poll`] can put it in the credentials instead of the server's answer.
|
||||
#[serde(skip)]
|
||||
pub server: String,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize)]
|
||||
@@ -66,9 +71,57 @@ pub async fn begin(
|
||||
});
|
||||
}
|
||||
|
||||
resp.json::<LoginFlow>()
|
||||
let mut flow = resp
|
||||
.json::<LoginFlow>()
|
||||
.await
|
||||
.map_err(|e| RemoteError::Protocol(e.to_string()))
|
||||
.map_err(|e| RemoteError::Protocol(e.to_string()))?;
|
||||
|
||||
// Both URLs are the server's to choose, and neither may be trusted as
|
||||
// sent. The login URL is handed to the operating system to open, where a
|
||||
// `file:` or UNC path is a program launch rather than a web page; the poll
|
||||
// endpoint is where the app password comes back from.
|
||||
flow.login_url = upgraded("login URL", &flow.login_url)?;
|
||||
flow.poll.endpoint = upgraded("poll endpoint", &flow.poll.endpoint)?;
|
||||
flow.server = server.trim_end_matches('/').to_string();
|
||||
Ok(flow)
|
||||
}
|
||||
|
||||
/// TRACES: NFR-SEC-3
|
||||
/// A URL the server sent, upgraded to HTTPS, or refused.
|
||||
///
|
||||
/// `http` is upgraded rather than refused: a Nextcloud behind a TLS-terminating
|
||||
/// proxy without `overwriteprotocol` builds every absolute URL it returns with
|
||||
/// `http`, and the same path over `https` is the one that works. Any other
|
||||
/// scheme is refused, because the only thing it could be for is reaching
|
||||
/// something that is not this server.
|
||||
///
|
||||
/// The host is not checked. A server reached by its LAN address may answer with
|
||||
/// its public name, and nothing here is safer for refusing that: the account
|
||||
/// is stored under the address the user typed (see [`poll`]), not under
|
||||
/// anything the server said.
|
||||
pub(crate) fn upgraded(what: &str, url: &str) -> Result<String, RemoteError> {
|
||||
let mut parsed = url::Url::parse(url).map_err(|e| {
|
||||
RemoteError::Protocol(format!(
|
||||
"the server sent a {what} that is not a URL ({e}): {url}"
|
||||
))
|
||||
})?;
|
||||
match parsed.scheme() {
|
||||
"https" => {}
|
||||
"http" => parsed.set_scheme("https").map_err(|()| {
|
||||
RemoteError::Protocol(format!("{what} cannot be upgraded to https: {url}"))
|
||||
})?,
|
||||
other => {
|
||||
return Err(RemoteError::Protocol(format!(
|
||||
"the server sent a {what} using {other}:, and only https is accepted: {url}"
|
||||
)))
|
||||
}
|
||||
}
|
||||
if parsed.host_str().is_none_or(str::is_empty) {
|
||||
return Err(RemoteError::Protocol(format!(
|
||||
"the server sent a {what} with no host: {url}"
|
||||
)));
|
||||
}
|
||||
Ok(parsed.into())
|
||||
}
|
||||
|
||||
/// Poll until the user finishes authenticating in the browser.
|
||||
@@ -98,10 +151,11 @@ pub async fn poll(
|
||||
|
||||
match resp.status().as_u16() {
|
||||
200 => {
|
||||
return resp
|
||||
let creds = resp
|
||||
.json::<AppCredentials>()
|
||||
.await
|
||||
.map_err(|e| RemoteError::Protocol(e.to_string()))
|
||||
.map_err(|e| RemoteError::Protocol(e.to_string()))?;
|
||||
return Ok(under_typed_server(creds, &flow.server));
|
||||
}
|
||||
// Still waiting for the user.
|
||||
404 => tokio::time::sleep(POLL_INTERVAL).await,
|
||||
@@ -117,6 +171,26 @@ pub async fn poll(
|
||||
Err(RemoteError::AuthFailed)
|
||||
}
|
||||
|
||||
/// TRACES: NFR-SEC-3
|
||||
/// Credentials filed under the address the user typed, not the one the server
|
||||
/// reports.
|
||||
///
|
||||
/// That report is the server's idea of its own URL, and behind a proxy
|
||||
/// without `overwriteprotocol` it says `http://` — which, stored, would send
|
||||
/// the app password in the clear on every request from then on. The typed
|
||||
/// address has just carried the whole flow, so it is known to reach the
|
||||
/// server.
|
||||
fn under_typed_server(mut creds: AppCredentials, server: &str) -> AppCredentials {
|
||||
if creds.server.trim_end_matches('/') != server {
|
||||
log::info!(
|
||||
"server reports itself as {}; keeping {server}",
|
||||
creds.server
|
||||
);
|
||||
}
|
||||
creds.server = server.to_string();
|
||||
creds
|
||||
}
|
||||
|
||||
/// Percent-encode a form value.
|
||||
fn urlencode(s: &str) -> String {
|
||||
s.bytes()
|
||||
@@ -167,6 +241,49 @@ mod tests {
|
||||
assert!(flow.login_url.contains("/login/v2/flow/"));
|
||||
}
|
||||
|
||||
/// TRACES: NFR-SEC-3
|
||||
#[test]
|
||||
fn server_urls_are_upgraded_to_https_or_refused() {
|
||||
assert_eq!(
|
||||
upgraded("login URL", "http://cloud.example/login/v2/flow/xyz").unwrap(),
|
||||
"https://cloud.example/login/v2/flow/xyz"
|
||||
);
|
||||
// A non-default port stays, and only the scheme changes.
|
||||
assert_eq!(
|
||||
upgraded("poll endpoint", "http://cloud.example:8443/login/v2/poll").unwrap(),
|
||||
"https://cloud.example:8443/login/v2/poll"
|
||||
);
|
||||
assert_eq!(
|
||||
upgraded("login URL", "https://cloud.example/x").unwrap(),
|
||||
"https://cloud.example/x"
|
||||
);
|
||||
// What `rundll32 url.dll,FileProtocolHandler` would run, and what
|
||||
// `xdg-open` would hand to whatever claims it.
|
||||
for hostile in [
|
||||
"file:///C:/Windows/System32/calc.exe",
|
||||
"\\\\evil\\share\\x.exe",
|
||||
"C:\\x.exe",
|
||||
"javascript:alert(1)",
|
||||
"-v",
|
||||
"",
|
||||
] {
|
||||
assert!(upgraded("login URL", hostile).is_err(), "{hostile:?}");
|
||||
}
|
||||
}
|
||||
|
||||
/// TRACES: NFR-SEC-3
|
||||
#[test]
|
||||
fn credentials_keep_the_typed_server_not_the_reported_one() {
|
||||
let reported = AppCredentials {
|
||||
server: "http://cloud.example".into(),
|
||||
login_name: "duncan".into(),
|
||||
app_password: "secret-token".into(),
|
||||
};
|
||||
let c = under_typed_server(reported, "https://cloud.example");
|
||||
assert_eq!(c.server, "https://cloud.example");
|
||||
assert_eq!(c.app_password, "secret-token");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn form_values_are_encoded() {
|
||||
assert_eq!(urlencode("abc123"), "abc123");
|
||||
|
||||
Reference in New Issue
Block a user