GNOME Bugzilla – Bug 725921
pull: MITM attacker can exhaust disk space
Last modified: 2018-08-17 19:00:17 UTC
All they'd need to do is just keep returning content from a pull. We could fix this by recording the total uncompressed size of all objects per commit, and verifying that we're not exceeding it during a pull.
This is split into metadata and data. For metadata, we should define some reasonable upper limit like say 10MB. If we look at: https://github.com/theupdateframework/tuf/blob/develop/docs/tuf-spec.txt They pretty consistently use the pair of (checksum, size) in referring to content. Which in hindsight I should have done as well. We can still do this for static deltas. But I don't see any limits they define for snapshot.json? For us, refs are a fixed size, so that's easy to limit. Commit objects are variable, but I think we could still reasonably enforce 10MB there.
Created attachment 277316 [details] [review] fetcher: Unref pending result when completing Otherwise we were just leaking it.
Created attachment 277317 [details] [review] Limit metadata to 10 MiB If fetching GPG-signed commits over plain HTTP, a MitM attacker can fill up the drive of targets by simply returning an enormous stream for the commit object. Related to this, an attacker can also cause OSTree to perform large memory allocations by returning enormous GVariants in the metadata. This helps close that attack by limiting all metadata objects to 10 MiB, so the initial fetch will be truncated. But now the attack is only slightly more difficult as the attacker will have to return a correctly formed commit object, then return a large stream of < 10 MiB dirmeta/dirtree objects.
Review of attachment 277316 [details] [review]: OK.
Review of attachment 277317 [details] [review]: ::: src/libostree/ostree-repo-commit.c @@ +554,3 @@ + if (OSTREE_OBJECT_TYPE_IS_META (objtype)) + { + if (file_object_length > (gsize)((OSTREE_MAX_METADATA_SIZE) * 0.8)) The *0.8 is quite missable. Either we have a separate define for OSTREE_WARN_METADATA_SIZE, or we have a comment saying "warn when we get close". @@ +558,3 @@ + gs_free char *metasize = g_format_size (file_object_length); + gs_free char *maxsize = g_format_size (OSTREE_MAX_METADATA_SIZE); + g_warning ("warning: metadata object %s is %s bytes, maximum metadata size is %s", g_warning already adds a "WARNING:", no? Doesn't g_format_size give us something like "1GB"? So this would be "metadata object foo is 1GB bytes"? This also isn't a very actionable warning. It just says spews some info. I'd put something like: "Metadata object foo is %s, nearing maximum size %s. Please double-check that your metadata won't grow too large". @@ +1071,3 @@ + { + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, + "Metadata object exceeds maximum size of %" G_GUINT64_FORMAT " bytes", You should probably give the name and size of the too-large metadata here as well for debugging purposes.
Comment on attachment 277316 [details] [review] fetcher: Unref pending result when completing Attachment 277316 [details] pushed as 6002356 - fetcher: Unref pending result when completing
Created attachment 277328 [details] [review] Limit metadata to 10 MiB Update for review comments
Review of attachment 277328 [details] [review]: ::: src/libostree/ostree-core.h @@ +35,3 @@ */ +#define OSTREE_MAX_METADATA_SIZE (10 * 1024 * 1024) +/** Missing whitespace? @@ +41,3 @@ + * will be emitted. + */ +#define OSTREE_MAX_METADATA_WARN_SIZE (7 * 1024 * 1024) I would have also been fine with (OSTREE_MAX_METADATA_SIZE * 0.8), but whatever. ::: src/libostree/ostree-repo-commit.c @@ +559,3 @@ + gs_free char *warnsize = g_format_size (OSTREE_MAX_METADATA_WARN_SIZE); + gs_free char *maxsize = g_format_size (OSTREE_MAX_METADATA_SIZE); + g_warning ("metadata object %s is %s bytes, which is larger than the warning threshold of %s bytes." \ Still has "1GB bytes". @@ +1073,3 @@ + { + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, + "Metadata object exceeds maximum size of %" G_GUINT64_FORMAT " bytes", Still doesn't have the name/size.
(In reply to comment #8) > > @@ +1073,3 @@ > + { > + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, > + "Metadata object exceeds maximum size of %" > G_GUINT64_FORMAT " bytes", > > Still doesn't have the name/size. The thing is it doesn't have a *name* until it's committed. I could checksum it by hand in the error path I guess, but practically speaking the people hitting this are going to be trying to "ostree commit" with --add-metadata-string=<megs of data> or the like. And in that case the error message should be clear.
Created attachment 277331 [details] [review] Limit metadata to 10 MiB A few more review fixes
Review of attachment 277331 [details] [review]: OK.
Attachment 277331 [details] pushed as 47610b4 - Limit metadata to 10 MiB
This isn't fixed yet, this patch is just the start.
One approach for addressing this would be similar to my comment on the "sizes" work (https://mail.gnome.org/archives/ostree-list/2013-September/msg00004.html). We don't need the exact byte count per object - something approximate would be OK as long as we bound the error. If we know both the total number of objects and the size of each individual object, we could compute a perfect hash of [checksum] -> [approximate size], where the goal is that [approximate size] is shared between many objects. Say that we have 1000 metadata objects that all fit under 1KiB - we can just hash all their checksums to the 1KiB entry. The error bound would be just ~1MiB which is likely fine for many deployments.
OSTree has moved to Github a while ago. Furthermore, GNOME Bugzilla will be shut down and replaced by gitlab.gnome.org. If the problem reported in this Bugzilla ticket is still valid, please report it to https://github.com/ostreedev/ostree/issues instead. Thank you! Closing this report as WONTFIX as part of Bugzilla Housekeeping.