diff options
Diffstat (limited to 'librsvg2-CVE-2023-38633.patch')
| -rw-r--r-- | librsvg2-CVE-2023-38633.patch | 414 |
1 files changed, 414 insertions, 0 deletions
diff --git a/librsvg2-CVE-2023-38633.patch b/librsvg2-CVE-2023-38633.patch new file mode 100644 index 0000000..07ffbe6 --- /dev/null +++ b/librsvg2-CVE-2023-38633.patch @@ -0,0 +1,414 @@ +From d1f066bf2198bd46c5ba80cb5123b768ec16e37d Mon Sep 17 00:00:00 2001 +From: Federico Mena Quintero <federico@gnome.org> +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 @@ + * </listitem> + * + * <listitem> +- * All other URL schemes in references require a base URL. For ++ * URLs with queries ("?") or fragment identifiers ("#") are not allowed. ++ * </listitem> ++ * ++ * <listitem> ++ * 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<P: AsRef<Path>>(path: P) -> Result<PathBuf, io::Error> { +- path.as_ref().canonicalize() +-} +-#[cfg(test)] +-fn canonicalize<P: AsRef<Path>>(path: P) -> Result<PathBuf, io::Error> { +- 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 `<feImage>` 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 @@ ++<!-- Empty file, used just to test URL validation --> +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 @@ ++<!-- Empty file, used just to test URL validation --> +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 @@ ++<!-- Empty file, used just to test URL validation --> +-- +GitLab + |