rpms
/
librsvg2
7
0
Fork 0
Code Issues Pull Requests Releases Activity
librsvg2/librsvg2-CVE-2023-38633.patch

415 lines
15 KiB
Diff
Raw Blame History

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
Reference in New Issue View Git Blame Copy Permalink