From 5b7e540900ebdfa6a5d6c5475193f5025160dbc5 Mon Sep 17 00:00:00 2001
From: Jeremy Stashewsky <jstash@gmail.com>
Date: Mon, 1 May 2023 10:36:40 -0700
Subject: [PATCH] Limit XInclude processing to shortcut keys files.

This patch fixes a security vulnerability when using the batch or
command-line processing features of Inkscape. The flaw allows an author
of a malicious SVG file to trivially specify the href of a local or
remote file to bring in as a `<text>` body or other element. The exact impact
to security will depend a lot on the context in which inkscape is being
run, but in the worst-case scenario this can lead to leaking of private
information, credentials, etc. Formally, this is a Local File Inclusion
(LFI) and Server Side Request Forgery (SSRF) vulnerability vector.

XInclude processing is retained for shortcut "keys" files _only_, which
seems to have been the original intent behind the commit that introduced
the vulnerability:
https://gitlab.com/inkscape/inkscape/-/commit/e6eee384cc7202d63136a3a1a8a5ccb990989d78
---
 src/ui/shortcuts.cpp |  4 ++--
 src/xml/repr-io.cpp  | 19 ++++++++++++-------
 src/xml/repr.h       |  2 +-
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/ui/shortcuts.cpp b/src/ui/shortcuts.cpp
index 9c67d6a5db..f7d4c6017b 100644
--- a/src/ui/shortcuts.cpp
+++ b/src/ui/shortcuts.cpp
@@ -231,7 +231,7 @@ Shortcuts::read(Glib::RefPtr<Gio::File> file, bool user_set)
         return false;
     }
 
-    XML::Document *document = sp_repr_read_file(file->get_path().c_str(), nullptr);
+    XML::Document *document = sp_repr_read_file(file->get_path().c_str(), nullptr, true);
     if (!document) {
         std::cerr << "Shortcut::read: could not parse file: " << file->get_path() << std::endl;
         return false;
@@ -734,7 +734,7 @@ Shortcuts::get_file_names()
         std::string label = Glib::path_get_basename(filename);
         Glib::ustring filename_relative = sp_relative_path_from_path(filename, std::string(get_path(SYSTEM, KEYS)));
 
-        XML::Document *document = sp_repr_read_file(filename.c_str(), nullptr);
+        XML::Document *document = sp_repr_read_file(filename.c_str(), nullptr, true);
         if (!document) {
             std::cerr << "Shortcut::get_file_names: could not parse file: " << filename.raw() << std::endl;
             continue;
diff --git a/src/xml/repr-io.cpp b/src/xml/repr-io.cpp
index 98b02b9243..249f62fa54 100644
--- a/src/xml/repr-io.cpp
+++ b/src/xml/repr-io.cpp
@@ -178,12 +178,7 @@ xmlDocPtr XmlSource::readXml()
     bool allowNetAccess = prefs->getBool("/options/externalresources/xml/allow_net_access", false);
     if (!allowNetAccess) parse_options |= XML_PARSE_NONET;
 
-    auto doc = xmlReadIO(readCb, closeCb, this, filename, getEncoding(), parse_options);
-    if (doc && doc->properties && xmlXIncludeProcessFlags(doc, XML_PARSE_NOXINCNODE) < 0) {
-        g_warning("XInclude processing failed for %s", filename);
-    }
-
-    return doc;
+    return xmlReadIO(readCb, closeCb, this, filename, getEncoding(), parse_options);
 }
 
 int XmlSource::readCb( void * context, char * buffer, int len )
@@ -268,8 +263,15 @@ int XmlSource::close()
 /**
  * Reads XML from a file, and returns the Document.
  * The default namespace can also be specified, if desired.
+ * XIncude is dangerous to support during use-cases like automated file format conversion, so it is off by default.
+ *
+ * \param filename The actual file to read from.
+ *
+ * \param default_ns Default namespace for the document, can be nullptr.
+ *
+ * \param xinclude Process XInclude directives, which is off by default for security.
  */
-Document *sp_repr_read_file (const gchar * filename, const gchar *default_ns)
+Document *sp_repr_read_file (const gchar * filename, const gchar *default_ns, bool xinclude)
 {
     xmlDocPtr doc = nullptr;
     Document * rdoc = nullptr;
@@ -299,6 +301,9 @@ Document *sp_repr_read_file (const gchar * filename, const gchar *default_ns)
 
     if (src.setFile(filename) == 0) {
         doc = src.readXml();
+        if (xinclude && doc && doc->properties && xmlXIncludeProcessFlags(doc, XML_PARSE_NOXINCNODE) < 0) {
+            g_warning("XInclude processing failed for %s", filename);
+        }
         rdoc = sp_repr_do_read(doc, default_ns);
     }
 
diff --git a/src/xml/repr.h b/src/xml/repr.h
index c560d989f4..9cfc68791a 100644
--- a/src/xml/repr.h
+++ b/src/xml/repr.h
@@ -52,7 +52,7 @@ Inkscape::XML::Document *sp_repr_document_new(char const *rootname);
 
 /* IO */
 
-Inkscape::XML::Document *sp_repr_read_file(char const *filename, char const *default_ns);
+Inkscape::XML::Document *sp_repr_read_file(char const *filename, char const *default_ns, bool xinclude = false);
 Inkscape::XML::Document *sp_repr_read_mem(char const *buffer, int length, char const *default_ns);
 void sp_repr_write_stream(Inkscape::XML::Node *repr, Inkscape::IO::Writer &out,
                           int indent_level,  bool add_whitespace, Glib::QueryQuark elide_prefix,
-- 
GitLab