From c740b7f6d2310105b1a978aa7448d529bacad0c9 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Wed, 19 Jul 2017 05:47:33 -0400
Subject: [PATCH] core: Sanitize error text validating refs (e.g. against HTML)

See: https://github.com/projectatomic/rpm-ostree/issues/885

If we get a successful Apache directory listing HTML when fetching what we
intend to be a ref, we'd dump the HTML into the error.

I did some scanning of the pull code, and this was the only case
I saw offhand where we were dumping text out into an error.  Which
makes sense, since most of our formats are binary, the exeptions I
think are just `repo/config` and `repo/refs/`.

Closes: #1015
Approved by: mbarnes
---
 src/libostree/ostree-core.c      | 44 +++++++++++++++++++++++++++++++-
 src/libostree/ostree-repo-pull.c |  5 ++--
 tests/pull-test.sh               | 13 +++++++++-
 tests/test-delta.sh              |  2 +-
 4 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/src/libostree/ostree-core.c b/src/libostree/ostree-core.c
index d51fa4d5..116f376f 100644
--- a/src/libostree/ostree-core.c
+++ b/src/libostree/ostree-core.c
@@ -36,6 +36,40 @@
 #define ALIGN_VALUE(this, boundary) \
   (( ((unsigned long)(this)) + (((unsigned long)(boundary)) -1)) & (~(((unsigned long)(boundary))-1)))
 
+/* Return a copy of @input suitable for addition to
+ * a GError message; newlines are quashed, the value
+ * is forced to be UTF-8, is truncated to @maxlen (if maxlen != -1).
+ */
+static char *
+quash_string_for_error_message (const char *input,
+                                ssize_t     len,
+                                ssize_t     maxlen)
+{
+  if (len == -1)
+    len = strlen (input);
+  if (maxlen != -1 && maxlen < len)
+    len = maxlen;
+#if GLIB_CHECK_VERSION(2, 52, 0)
+  G_GNUC_BEGIN_IGNORE_DEPRECATIONS
+  char *buf = g_utf8_make_valid (input, len);
+  G_GNUC_END_IGNORE_DEPRECATIONS
+#else
+  char *buf = g_strndup (input, len);
+#endif
+  for (char *iter = buf; iter && *iter; iter++)
+    {
+      char c = *iter;
+      if (c == '\n')
+        *iter = ' ';
+#if !GLIB_CHECK_VERSION(2, 52, 0)
+      /* No g_utf8_make_valid()?  OK, let's just brute force this. */
+      if (!g_ascii_isprint (c))
+        *iter = ' ';
+#endif
+    }
+  return buf;
+}
+
 static gboolean
 file_header_parse (GVariant         *metadata,
                    GFileInfo       **out_file_info,
@@ -1825,7 +1859,15 @@ ostree_validate_structureof_checksum_string (const char *checksum,
   size_t len = strlen (checksum);
 
   if (len != OSTREE_SHA256_STRING_LEN)
-    return glnx_throw (error, "Invalid rev '%s'", checksum);
+    {
+      /* If we happen to get e.g. an Apache directory listing HTML, don't
+       * dump it all to the error.
+       * https://github.com/projectatomic/rpm-ostree/issues/885
+       */
+      g_autofree char *sanitized = quash_string_for_error_message (checksum, len,
+                                                                   OSTREE_SHA256_STRING_LEN);
+      return glnx_throw (error, "Invalid rev %s", sanitized);
+    }
 
   for (i = 0; i < len; i++)
     {
diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c
index 5d31e034..33ea489c 100644
--- a/src/libostree/ostree-repo-pull.c
+++ b/src/libostree/ostree-repo-pull.c
@@ -876,6 +876,7 @@ scan_dirtree_object (OtPullData   *pull_data,
   return TRUE;
 }
 
+/* Given a @ref, fetch its contents (should be a SHA256 ASCII string) */
 static gboolean
 fetch_ref_contents (OtPullData                 *pull_data,
                     const char                 *main_collection_id,
@@ -901,7 +902,7 @@ fetch_ref_contents (OtPullData                 *pull_data,
   g_strchomp (ret_contents);
 
   if (!ostree_validate_checksum_string (ret_contents, error))
-    return FALSE;
+    return glnx_prefix_error (error, "Fetching %s", filename);
 
   ot_transfer_out_value (out_contents, &ret_contents);
   return TRUE;
@@ -1992,7 +1993,7 @@ load_remote_repo_config (OtPullData    *pull_data,
   g_autoptr(GKeyFile) ret_keyfile = g_key_file_new ();
   if (!g_key_file_load_from_data (ret_keyfile, contents, strlen (contents),
                                   0, error))
-    return FALSE;
+    return glnx_prefix_error (error, "Parsing config");
 
   ot_transfer_out_value (out_keyfile, &ret_keyfile);
   return TRUE;
diff --git a/tests/pull-test.sh b/tests/pull-test.sh
index 37b5e915..9bbe0fa2 100644
--- a/tests/pull-test.sh
+++ b/tests/pull-test.sh
@@ -35,7 +35,7 @@ function verify_initial_contents() {
     assert_file_has_content baz/cow '^moo$'
 }
 
-echo "1..26"
+echo "1..27"
 
 # Try both syntaxes
 repo_init --no-gpg-verify
@@ -413,3 +413,14 @@ rm $localsig
 ${CMD_PREFIX} ostree --repo=repo pull origin main
 test -f $localsig
 echo "ok re-pull signature for stored commit"
+
+cd ${test_tmpdir}
+repo_init --no-gpg-verify
+mv ostree-srv/gnomerepo/refs/heads/main{,.orig}
+rm ostree-srv/gnomerepo/summary
+(for x in $(seq 20); do echo "lots of html here "; done) > ostree-srv/gnomerepo/refs/heads/main
+if ${CMD_PREFIX} ostree --repo=repo pull origin main 2>err.txt; then
+    fatal "pull of invalid ref succeeded"
+fi
+assert_file_has_content_literal err.txt 'error: Fetching refs/heads/main: Invalid rev lots of html here  lots of html here  lots of html here  lots of'
+echo "ok pull got HTML for a ref"
diff --git a/tests/test-delta.sh b/tests/test-delta.sh
index 8baee723..a59ee8e5 100755
--- a/tests/test-delta.sh
+++ b/tests/test-delta.sh
@@ -258,6 +258,6 @@ ${CMD_PREFIX} ostree --repo=repo summary -u
 if ${CMD_PREFIX} ostree --repo=repo static-delta show GARBAGE 2> err.txt; then
     assert_not_reached "static-delta show GARBAGE unexpectedly succeeded"
 fi
-assert_file_has_content err.txt "Invalid rev 'GARBAGE'"
+assert_file_has_content err.txt "Invalid rev GARBAGE"
 
 echo 'ok handle bad delta name'