Only verify OSTREE_MAX_METADATA_SIZE for HTTP fetches
authorColin Walters <walters@verbum.org>
Mon, 1 Oct 2018 00:10:14 +0000 (20:10 -0400)
committerAtomic Bot <atomic-devel@projectatomic.io>
Mon, 1 Oct 2018 13:23:50 +0000 (13:23 +0000)
There are use cases for libostree as a local content store
for content derived or delivered via other mechanisms (e.g. OCI
images, RPMs, etc.).  rpm-ostree today imports RPMs into OSTree
branches, and puts the RPM header value as commit metadata.
Some of these can be quite large because the header includes
permissions for each file.  Similarly, some OCI metadata is large.

Since there's no security issues with this, support committing
such content.

We still by default limit the size of metadata fetches, although
for good measure we make this configurable too via a new
`max-metadata-size` value.

Closes: https://github.com/ostreedev/ostree/issues/1721
Closes: #1744
Approved by: jlebon

src/libostree/ostree-core.h
src/libostree/ostree-repo-commit.c
src/libostree/ostree-repo-pull.c
tests/test-basic-c.c

index 08b7d451c1f751a0fc56988dc96f1f689060f8d1..69477a75965f597a4cbe8945608e6f1821eedf33 100644 (file)
@@ -31,18 +31,18 @@ G_BEGIN_DECLS
 
 /**
  * OSTREE_MAX_METADATA_SIZE:
- * 
- * Maximum permitted size in bytes of metadata objects.  This is an
- * arbitrary number, but really, no one should be putting humongous
- * data in metadata.
+ *
+ * Default limit for maximum permitted size in bytes of metadata objects fetched
+ * over HTTP (including repo/config files, refs, and commit/dirtree/dirmeta
+ * objects). This is an arbitrary number intended to mitigate disk space
+ * exhaustion attacks.
  */
 #define OSTREE_MAX_METADATA_SIZE (10 * 1024 * 1024)
 
 /**
  * OSTREE_MAX_METADATA_WARN_SIZE:
- * 
- * Objects committed above this size will be allowed, but a warning
- * will be emitted.
+ *
+ * This variable is no longer meaningful, it is kept only for compatibility.
  */
 #define OSTREE_MAX_METADATA_WARN_SIZE (7 * 1024 * 1024)
 
index e883cedce4bb234d5b51ed43a36c6ab1ffc12851..9521dc6c2581e4addc1c8b363199e0bde038d0b5 100644 (file)
@@ -1336,18 +1336,6 @@ write_metadata_object (OstreeRepo         *self,
   gsize len;
   const guint8 *bufp = g_bytes_get_data (buf, &len);
 
-  /* Do the size warning here, to avoid warning for already extant metadata */
-  if (G_UNLIKELY (len > OSTREE_MAX_METADATA_WARN_SIZE))
-    {
-      g_autofree char *metasize = g_format_size (len);
-      g_autofree char *warnsize = g_format_size (OSTREE_MAX_METADATA_WARN_SIZE);
-      g_autofree char *maxsize = g_format_size (OSTREE_MAX_METADATA_SIZE);
-      g_warning ("metadata object %s is %s, which is larger than the warning threshold of %s." \
-                 "  The hard limit on metadata size is %s.  Put large content in the tree itself, not in metadata.",
-                 actual_checksum,
-                 metasize, warnsize, maxsize);
-    }
-
   /* Write the metadata to a temporary file */
   g_auto(GLnxTmpfile) tmpf = { 0, };
   if (!glnx_open_tmpfile_linkable_at (commit_tmp_dfd (self), ".", O_WRONLY|O_CLOEXEC,
@@ -2278,25 +2266,6 @@ ostree_repo_abort_transaction (OstreeRepo     *self,
   return TRUE;
 }
 
-/* These limits were introduced since in some cases we may be processing
- * malicious metadata, and we want to make disk space exhaustion attacks harder.
- */
-static gboolean
-metadata_size_valid (OstreeObjectType objtype,
-                     gsize len,
-                     GError **error)
-{
-  if (G_UNLIKELY (len > OSTREE_MAX_METADATA_SIZE))
-    {
-      g_autofree char *input_bytes = g_format_size (len);
-      g_autofree char *max_bytes = g_format_size (OSTREE_MAX_METADATA_SIZE);
-      return glnx_throw (error, "Metadata object of type '%s' is %s; maximum metadata size is %s",
-                         ostree_object_type_to_string (objtype), input_bytes, max_bytes);
-    }
-
-  return TRUE;
-}
-
 /**
  * ostree_repo_write_metadata:
  * @self: Repo
@@ -2349,9 +2318,6 @@ ostree_repo_write_metadata (OstreeRepo         *self,
       normalized = g_variant_get_normal_form (object);
     }
 
-  if (!metadata_size_valid (objtype, g_variant_get_size (normalized), error))
-    return FALSE;
-
   /* For untrusted objects, verify their structure here */
   if (expected_checksum)
     {
@@ -2389,9 +2355,6 @@ ostree_repo_write_metadata_stream_trusted (OstreeRepo        *self,
                                            GCancellable      *cancellable,
                                            GError           **error)
 {
-  if (length > 0 && !metadata_size_valid (objtype, length, error))
-    return FALSE;
-
   /* This is all pretty ridiculous, but we're keeping this API for backwards
    * compatibility, it doesn't really need to be fast.
    */
index c6b70ffb3856efa07b7b7d74694fecf1749bc7f5..f5c52e4a9602b9382a05c68ae94b86b4d13bcf63 100644 (file)
@@ -150,6 +150,7 @@ typedef struct {
 
   gboolean          timestamp_check; /* Verify commit timestamps */
   int               maxdepth;
+  guint64           max_metadata_size;
   guint64           start_time;
 
   gboolean          is_mirror;
@@ -2193,7 +2194,7 @@ start_fetch (OtPullData *pull_data,
   if (expected_max_size_p)
     expected_max_size = *expected_max_size_p;
   else if (OSTREE_OBJECT_TYPE_IS_META (objtype))
-    expected_max_size = OSTREE_MAX_METADATA_SIZE;
+    expected_max_size = pull_data->max_metadata_size;
   else
     expected_max_size = 0;
 
@@ -3488,6 +3489,7 @@ initiate_request (OtPullData                 *pull_data,
  *   * require-static-deltas (b): Require static deltas
  *   * override-commit-ids (as): Array of specific commit IDs to fetch for refs
  *   * timestamp-check (b): Verify commit timestamps are newer than current (when pulling via ref); Since: 2017.11
+ *   * metadata-size-restriction (t): Restrict metadata objects to a maximum number of bytes; 0 to disable.  Since: 2018.9
  *   * dry-run (b): Only print information on what will be downloaded (requires static deltas)
  *   * override-url (s): Fetch objects from this URL if remote specifies no metalink in options
  *   * inherit-transaction (b): Don't initiate, finish or abort a transaction, useful to do multiple pulls in one transaction.
@@ -3543,6 +3545,9 @@ ostree_repo_pull_with_options (OstreeRepo             *self,
    */
   const char *the_ref_to_fetch = NULL;
 
+  /* Default */
+  pull_data->max_metadata_size = OSTREE_MAX_METADATA_SIZE;
+
   if (options)
     {
       int flags_i = OSTREE_REPO_PULL_FLAGS_NONE;
@@ -3570,6 +3575,7 @@ ostree_repo_pull_with_options (OstreeRepo             *self,
       (void) g_variant_lookup (options, "update-frequency", "u", &update_frequency);
       (void) g_variant_lookup (options, "localcache-repos", "^a&s", &opt_localcache_repos);
       (void) g_variant_lookup (options, "timestamp-check", "b", &pull_data->timestamp_check);
+      (void) g_variant_lookup (options, "max-metadata-size", "t", &pull_data->max_metadata_size);
       (void) g_variant_lookup (options, "append-user-agent", "s", &pull_data->append_user_agent);
       opt_n_network_retries_set =
         g_variant_lookup (options, "n-network-retries", "u", &pull_data->n_network_retries);
index 8515b12901daa3a13c64bb95a10df3b546d85e51..ff6d353c300043ee58632c24b3cdfce3b20633f7 100644 (file)
@@ -430,6 +430,54 @@ test_devino_cache_xattrs (void)
   g_assert_cmpint (stats.content_objects_written, ==, 1);
 }
 
+/* https://github.com/ostreedev/ostree/issues/1721
+ * We should be able to commit large metadata objects now.
+ */
+static void
+test_big_metadata (void)
+{
+  g_autoptr(GError) error = NULL;
+  gboolean ret = FALSE;
+
+  g_autoptr(GFile) repo_path = g_file_new_for_path ("repo");
+
+  /* init as bare-user-only so we run everywhere */
+  ret = ot_test_run_libtest ("setup_test_repository bare-user-only", &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  g_autoptr(OstreeRepo) repo = ostree_repo_new (repo_path);
+  ret = ostree_repo_open (repo, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  g_autoptr(GFile) object_to_commit = NULL;
+  ret = ostree_repo_read_commit (repo, "test2", &object_to_commit, NULL, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  g_autoptr(OstreeMutableTree) mtree = ostree_mutable_tree_new ();
+  ret = ostree_repo_write_directory_to_mtree (repo, object_to_commit, mtree, NULL,
+                                              NULL, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+
+  const size_t len = 20 * 1024 * 1024;
+  g_assert_cmpint (len, >, OSTREE_MAX_METADATA_SIZE);
+  g_autofree char *large_buf = g_malloc (len);
+  memset (large_buf, 0x42, len);
+  g_autoptr(GVariantBuilder) builder =
+    g_variant_builder_new (G_VARIANT_TYPE ("a{sv}"));
+  g_autofree char *commit_checksum = NULL;
+  g_variant_builder_add (builder, "{sv}", "large-value",
+                         g_variant_new_fixed_array ((GVariantType*)"y",
+                                                    large_buf, len, sizeof (char)));
+  ret = ostree_repo_write_commit (repo, NULL, NULL, NULL, g_variant_builder_end (builder),
+                                  OSTREE_REPO_FILE (object_to_commit), &commit_checksum, NULL, &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+}
+
 int main (int argc, char **argv)
 {
   g_autoptr(GError) error = NULL;
@@ -447,6 +495,7 @@ int main (int argc, char **argv)
   g_test_add_func ("/xattrs-devino-cache", test_devino_cache_xattrs);
   g_test_add_func ("/break-hardlink", test_break_hardlink);
   g_test_add_func ("/remotename", test_validate_remotename);
+  g_test_add_func ("/big-metadata", test_big_metadata);
 
   return g_test_run();
  out: