From d1f066bf2198bd46c5ba80cb5123b768ec16e37d Mon Sep 17 00:00:00 2001 From: Federico Mena Quintero Date: Thu, 20 Jul 2023 11:12:53 -0600 Subject: [PATCH] (#996): Fix arbitrary file read when href has special characters In UrlResolver::resolve_href() we now explicitly disallow URLs that have a query string ("?") or a fragment identifier ("#"). We also explicitly check for a base URL and not resolving to a path, for example, "file:///base/foo.svg" + "." would resolve to "file:///base/" - this is technically correct, but we don't want to resolve to directories. Also, we pass a canonicalized path name as a URL upstream, so that g_file_new_from_url() will consume it later, instead of passing the original and potentially malicious URL. Fixes https://gitlab.gnome.org/GNOME/librsvg/-/issues/996 --- librsvg/rsvg-handle.c | 6 +- rsvg_internals/src/allowed_url.rs | 229 +++++++++++++----- .../src/filters/component_transfer.rs | 2 +- tests/Makefile.am | 1 + tests/fixtures/loading/bar.svg | 1 + tests/fixtures/loading/foo.svg | 1 + tests/fixtures/loading/subdir/baz.svg | 1 + 7 files changed, 180 insertions(+), 61 deletions(-) create mode 100644 tests/fixtures/loading/bar.svg create mode 100644 tests/fixtures/loading/foo.svg create mode 100644 tests/fixtures/loading/subdir/baz.svg diff --git a/librsvg/rsvg-handle.c b/librsvg/rsvg-handle.c index 95364db34..f49e4d30e 100644 --- a/librsvg/rsvg-handle.c +++ b/librsvg/rsvg-handle.c @@ -78,7 +78,11 @@ * * * - * All other URL schemes in references require a base URL. For + * URLs with queries ("?") or fragment identifiers ("#") are not allowed. + * + * + * + * All other URL schemes other than data: in references require a base URL. For * example, this means that if you load an SVG with * rsvg_handle_new_from_data() without calling rsvg_handle_set_base_uri(), * then any referenced files will not be allowed (e.g. raster images to be diff --git a/rsvg_internals/src/allowed_url.rs b/rsvg_internals/src/allowed_url.rs index 3a99e00b8..ffa9a2315 100644 --- a/rsvg_internals/src/allowed_url.rs +++ b/rsvg_internals/src/allowed_url.rs @@ -2,9 +2,7 @@ use std::error; use std::fmt; -use std::io; use std::ops::Deref; -use std::path::{Path, PathBuf}; use url::Url; use crate::error::HrefError; @@ -37,6 +35,12 @@ pub enum AllowedUrlError { /// or in one directory below the base file. NotSiblingOrChildOfBaseFile, + /// Loaded file:// URLs cannot have a query part, e.g. `file:///foo?blah` + NoQueriesAllowed, + + /// URLs may not have fragment identifiers at this stage + NoFragmentIdentifierAllowed, + /// Error when obtaining the file path or the base file path InvalidPath, @@ -59,6 +63,17 @@ impl AllowedUrl { return Ok(AllowedUrl(url)); } + // Queries are not allowed. + if url.query().is_some() { + return Err(AllowedUrlError::NoQueriesAllowed); + } + + // Fragment identifiers are not allowed. They should have been stripped + // upstream, by NodeId. + if url.fragment().is_some() { + return Err(AllowedUrlError::NoFragmentIdentifierAllowed); + } + // All other sources require a base url if base_url.is_none() { return Err(AllowedUrlError::BaseRequired); @@ -81,6 +96,26 @@ impl AllowedUrl { return Err(AllowedUrlError::DisallowedScheme); } + // The rest of this function assumes file: URLs; guard against + // incorrect refactoring. + assert!(url.scheme() == "file"); + + // If we have a base_uri of "file:///foo/bar.svg", and resolve an href of ".", + // Url.parse() will give us "file:///foo/". We don't want that, so check + // if the last path segment is empty - it will not be empty for a normal file. + + if let Some(segments) = url.path_segments() { + if segments + .last() + .expect("URL path segments always contain at last 1 element") + .is_empty() + { + return Err(AllowedUrlError::NotSiblingOrChildOfBaseFile); + } + } else { + unreachable!("the file: URL cannot have an empty path"); + } + // We have two file: URIs. Now canonicalize them (remove .. and symlinks, etc.) // and see if the directories match @@ -98,13 +133,17 @@ impl AllowedUrl { let base_parent = base_parent.unwrap(); - let url_canon = - canonicalize(&url_path).map_err(|_| AllowedUrlError::CanonicalizationError)?; - let parent_canon = - canonicalize(&base_parent).map_err(|_| AllowedUrlError::CanonicalizationError)?; - - if url_canon.starts_with(parent_canon) { - Ok(AllowedUrl(url)) + let path_canon = url_path + .canonicalize() + .map_err(|_| AllowedUrlError::CanonicalizationError)?; + let parent_canon = base_parent + .canonicalize() + .map_err(|_| AllowedUrlError::CanonicalizationError)?; + + if path_canon.starts_with(parent_canon) { + // Finally, convert the canonicalized path back to a URL. + let path_to_url = Url::from_file_path(path_canon).unwrap(); + Ok(AllowedUrl(path_to_url)) } else { Err(AllowedUrlError::NotSiblingOrChildOfBaseFile) } @@ -129,32 +168,22 @@ impl error::Error for AllowedUrlError {} impl fmt::Display for AllowedUrlError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match *self { - AllowedUrlError::HrefParseError(e) => write!(f, "href parse error: {}", e), - AllowedUrlError::BaseRequired => write!(f, "base required"), - AllowedUrlError::DifferentURISchemes => write!(f, "different URI schemes"), - AllowedUrlError::DisallowedScheme => write!(f, "disallowed scheme"), - AllowedUrlError::NotSiblingOrChildOfBaseFile => { - write!(f, "not sibling or child of base file") - } - AllowedUrlError::InvalidPath => write!(f, "invalid path"), - AllowedUrlError::BaseIsRoot => write!(f, "base is root"), - AllowedUrlError::CanonicalizationError => write!(f, "canonicalization error"), + use AllowedUrlError::*; + match self { + HrefParseError(e) => write!(f, "URL parse error: {e}"), + BaseRequired => write!(f, "base required"), + DifferentUriSchemes => write!(f, "different URI schemes"), + DisallowedScheme => write!(f, "disallowed scheme"), + NotSiblingOrChildOfBaseFile => write!(f, "not sibling or child of base file"), + NoQueriesAllowed => write!(f, "no queries allowed"), + NoFragmentIdentifierAllowed => write!(f, "no fragment identifier allowed"), + InvalidPath => write!(f, "invalid path"), + BaseIsRoot => write!(f, "base is root"), + CanonicalizationError => write!(f, "canonicalization error"), } } } -// For tests, we don't want to touch the filesystem. In that case, -// assume that we are being passed canonical file names. -#[cfg(not(test))] -fn canonicalize>(path: P) -> Result { - path.as_ref().canonicalize() -} -#[cfg(test)] -fn canonicalize>(path: P) -> Result { - Ok(path.as_ref().to_path_buf()) -} - /// Parsed result of an href from an SVG or CSS file /// /// Sometimes in SVG element references (e.g. the `href` in the `` element) we @@ -234,6 +263,8 @@ impl Href { mod tests { use super::*; + use std::path::PathBuf; + #[test] fn disallows_relative_file_with_no_base_file() { assert_eq!( @@ -284,56 +315,136 @@ mod tests { ); } + fn url_from_test_fixtures(filename_relative_to_librsvg_srcdir: &str) -> Url { + let path = PathBuf::from(filename_relative_to_librsvg_srcdir); + let absolute = path + .canonicalize() + .expect("files from test fixtures are supposed to canonicalize"); + Url::from_file_path(absolute).unwrap() + } + #[test] fn allows_relative() { - assert_eq!( - AllowedUrl::from_href( - "foo.svg", - Some(Url::parse("file:///example/bar.svg").unwrap()).as_ref() - ) - .unwrap() - .as_ref(), - "file:///example/foo.svg", - ); + let resolved = AllowedUrl::from_href( + "foo.svg", + Some(url_from_test_fixtures("../tests/fixtures/loading/bar.svg")).as_ref() + ).unwrap(); + + let resolved_str = resolved.as_str(); + assert!(resolved_str.ends_with("/loading/foo.svg")); } #[test] fn allows_sibling() { - assert_eq!( - AllowedUrl::from_href( - "file:///example/foo.svg", - Some(Url::parse("file:///example/bar.svg").unwrap()).as_ref() - ) - .unwrap() - .as_ref(), - "file:///example/foo.svg", - ); + let sibling = url_from_test_fixtures("../tests/fixtures/loading/foo.svg"); + let resolved = AllowedUrl::from_href( + sibling.as_str(), + Some(url_from_test_fixtures("../tests/fixtures/loading/bar.svg")).as_ref() + ).unwrap(); + + let resolved_str = resolved.as_str(); + assert!(resolved_str.ends_with("/loading/foo.svg")); } #[test] fn allows_child_of_sibling() { - assert_eq!( - AllowedUrl::from_href( - "file:///example/subdir/foo.svg", - Some(Url::parse("file:///example/bar.svg").unwrap()).as_ref() - ) - .unwrap() - .as_ref(), - "file:///example/subdir/foo.svg", - ); + let child_of_sibling = url_from_test_fixtures("../tests/fixtures/loading/subdir/baz.svg"); + let resolved = AllowedUrl::from_href( + child_of_sibling.as_str(), + Some(url_from_test_fixtures("../tests/fixtures/loading/bar.svg")).as_ref() + ).unwrap(); + + let resolved_str = resolved.as_str(); + assert!(resolved_str.ends_with("/loading/subdir/baz.svg")); } + // Ignore on Windows since we test for /etc/passwd + #[cfg(unix)] #[test] fn disallows_non_sibling() { assert_eq!( AllowedUrl::from_href( "file:///etc/passwd", - Some(Url::parse("file:///example/bar.svg").unwrap()).as_ref() + Some(url_from_test_fixtures("../tests/fixtures/loading/bar.svg")).as_ref() ), Err(AllowedUrlError::NotSiblingOrChildOfBaseFile) ); } + #[test] + fn disallows_queries() { + assert!(matches!( + AllowedUrl::from_href( + ".?../../../../../../../../../../etc/passwd", + Some(url_from_test_fixtures("../tests/fixtures/loading/bar.svg")).as_ref(), + ), + Err(AllowedUrlError::NoQueriesAllowed) + )); + } + + #[test] + fn disallows_weird_relative_uris() { + let base_url = url_from_test_fixtures("../tests/fixtures/loading/bar.svg"); + + assert!( + AllowedUrl::from_href( + ".@../../../../../../../../../../etc/passwd", + Some(&base_url), + ).is_err() + ); + assert!( + AllowedUrl::from_href( + ".$../../../../../../../../../../etc/passwd", + Some(&base_url), + ).is_err() + ); + assert!( + AllowedUrl::from_href( + ".%../../../../../../../../../../etc/passwd", + Some(&base_url), + ).is_err() + ); + assert!( + AllowedUrl::from_href( + ".*../../../../../../../../../../etc/passwd", + Some(&base_url), + ).is_err() + ); + assert!( + AllowedUrl::from_href( + "~/../../../../../../../../../../etc/passwd", + Some(&base_url), + ).is_err() + ); + } + + #[test] + fn disallows_dot_sibling() { + println!("cwd: {:?}", std::env::current_dir()); + let base_url = url_from_test_fixtures("../tests/fixtures/loading/bar.svg"); + + assert!(matches!( + AllowedUrl::from_href(".", Some(&base_url)), + Err(AllowedUrlError::NotSiblingOrChildOfBaseFile) + )); + assert!(matches!( + AllowedUrl::from_href(".#../../../../../../../../../../etc/passwd", Some(&base_url)), + Err(AllowedUrlError::NoFragmentIdentifierAllowed) + )); + } + + #[test] + fn disallows_fragment() { + // AllowedUrl::from_href() explicitly disallows fragment identifiers. + // This is because they should have been stripped before calling that function, + // by the Iri machinery. + + assert!(matches!( + AllowedUrl::from_href("bar.svg#fragment", Some(Url::parse("https://example.com/foo.svg").unwrap()).as_ref()), + Err(AllowedUrlError::NoFragmentIdentifierAllowed) + )); + } + #[test] fn parses_href() { assert_eq!( diff --git a/rsvg_internals/src/filters/component_transfer.rs b/rsvg_internals/src/filters/component_transfer.rs index 235435ffa..6845eac18 100644 --- a/rsvg_internals/src/filters/component_transfer.rs +++ b/rsvg_internals/src/filters/component_transfer.rs @@ -261,7 +261,7 @@ macro_rules! func_or_default { } } _ => &$func_default, - }; + } }; } diff --git a/tests/Makefile.am b/tests/Makefile.am index 13c2d51f2..b3faf2da5 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -82,6 +82,7 @@ dist_installed_test_data = \ $(wildcard $(srcdir)/fixtures/errors/*) \ $(wildcard $(srcdir)/fixtures/infinite-loop/*) \ $(wildcard $(srcdir)/fixtures/loading/*) \ + $(wildcard $(srcdir)/fixtures/loading/subdir/*) \ $(wildcard $(srcdir)/fixtures/reftests/*.css) \ $(wildcard $(srcdir)/fixtures/reftests/*.svg) \ $(wildcard $(srcdir)/fixtures/reftests/*.png) \ diff --git a/tests/fixtures/loading/bar.svg b/tests/fixtures/loading/bar.svg new file mode 100644 index 000000000..304670099 --- /dev/null +++ b/tests/fixtures/loading/bar.svg @@ -0,0 +1 @@ + diff --git a/tests/fixtures/loading/foo.svg b/tests/fixtures/loading/foo.svg new file mode 100644 index 000000000..304670099 --- /dev/null +++ b/tests/fixtures/loading/foo.svg @@ -0,0 +1 @@ + diff --git a/tests/fixtures/loading/subdir/baz.svg b/tests/fixtures/loading/subdir/baz.svg new file mode 100644 index 000000000..304670099 --- /dev/null +++ b/tests/fixtures/loading/subdir/baz.svg @@ -0,0 +1 @@ + -- GitLab