diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d352978..b530dfec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Fixed +- Replace control and bidirectional formatting characters in a download's + filename, which let a title disguise the file's real extension. - Reject off-origin and scheme-relative `Referer` values when redirecting back after a theme change, closing an open-redirect bypass via `//`-prefixed paths. - Unlink Unix domain socket after shutdown. diff --git a/crates/wastebin_server/src/handlers/download.rs b/crates/wastebin_server/src/handlers/download.rs index 1f4b1720..f3bb6a7e 100644 --- a/crates/wastebin_server/src/handlers/download.rs +++ b/crates/wastebin_server/src/handlers/download.rs @@ -47,14 +47,34 @@ pub async fn get( .map_err(|err| make_error(err, page, theme, lang)) } +/// Return `true` if `c` reorders the text around it rather than showing a glyph of its own. +/// +/// Percent-encoding carries these through `filename*` intact, and the client decodes them back +/// before showing the name, so `evilgnp.exe` is offered as `evil.exe.png` — the extension +/// the user reads is not the one the file has. +fn reorders_text(c: char) -> bool { + matches!(c, '\u{200e}' | '\u{200f}' | '\u{202a}'..='\u{202e}' | '\u{2066}'..='\u{2069}') +} + fn make_content_disposition(filename: &str) -> HeaderValue { let mut value = String::from("attachment; filename*=UTF-8''"); + let mut encoded = [0u8; 4]; - for b in filename.bytes() { - if b.is_ascii_alphanumeric() || matches!(b, b'.' | b'-' | b'_' | b'~' | b'+') { - value.push(b as char); + // The title is attacker-controlled, so the characters that would misrepresent the name are + // replaced before it is encoded. + for c in filename.chars() { + let c = if c.is_control() || reorders_text(c) { + '_' } else { - write!(value, "%{b:02X}").expect("writing to String"); + c + }; + + for &b in c.encode_utf8(&mut encoded).as_bytes() { + if b.is_ascii_alphanumeric() || matches!(b, b'.' | b'-' | b'_' | b'~' | b'+') { + value.push(b as char); + } else { + write!(value, "%{b:02X}").expect("writing to String"); + } } } @@ -141,6 +161,32 @@ mod tests { Ok(()) } + /// Percent-encoding is transport, not sanitisation: the client decodes `filename*` back before + /// showing the name, so a right-to-left override survived into it and offered + /// `evilgnp.exe` as `evil.exe.png`. + #[tokio::test] + async fn download_title_reordering_the_extension() -> Result<(), Box> { + let client = Client::new(StoreCookies(false)).await; + let data = Entry { + text: String::from("content"), + title: String::from("evil\u{202e}gnp\u{7}.exe"), + ..Default::default() + }; + + let res = client.post_form().form(&data).send().await?; + let location = res.headers().get("location").unwrap().to_str()?; + let res = client.get(&format!("/dl{location}")).send().await?; + assert_eq!(res.status(), StatusCode::OK); + + let content_disposition = res.headers().get(header::CONTENT_DISPOSITION).unwrap(); + assert_eq!( + content_disposition.to_str()?, + "attachment; filename*=UTF-8''evil_gnp_.exe", + ); + + Ok(()) + } + #[tokio::test] async fn download_title_with_non_ascii() -> Result<(), Box> { let client = Client::new(StoreCookies(false)).await;