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

diff --git a/src/libostree/ostree-core.c b/src/libostree/ostree-core.c
index d51fa4d59c6b89ea886b98f84994a55f24d0e88a..116f376f2bd3e5797a6c679f2573d1383d8d5edf 100644 (file)
--- 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 5d31e0341a60df4c9d086103afcebda78d4cdcf9..33ea489c3bdda09a59e4761ea31dd4fcd6c5242b 100644 (file)
--- 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 37b5e915909ab8af7a8de990bbfc51f7611a358f..9bbe0fa276ee6fe6d8589f2199f5a7d3578c1a4e 100644 (file)
--- 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 8baee72393ae5e5a3ef3982e461a0a208dfc3916..a59ee8e5ba7a223079d761da7ea1e27fbf4a90ca 100755 (executable)
--- 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'