diff --git a/src/libostree/ostree-core-private.h b/src/libostree/ostree-core-private.h index 08809e4a26..162677c35c 100644 --- a/src/libostree/ostree-core-private.h +++ b/src/libostree/ostree-core-private.h @@ -164,6 +164,17 @@ _ostree_loose_path (char *buf, OstreeObjectType objtype, OstreeRepoMode repo_mode); +gboolean _ostree_validate_structureof_metadata (OstreeObjectType objtype, + GVariant *commit, + GError **error); + +gboolean +_ostree_verify_metadata_object (OstreeObjectType objtype, + const char *expected_checksum, + GVariant *metadata, + GError **error); + + #define _OSTREE_METADATA_GPGSIGS_NAME "ostree.gpgsigs" #define _OSTREE_METADATA_GPGSIGS_TYPE G_VARIANT_TYPE ("aay") diff --git a/src/libostree/ostree-core.c b/src/libostree/ostree-core.c index 679c952914..2256a2c063 100644 --- a/src/libostree/ostree-core.c +++ b/src/libostree/ostree-core.c @@ -2071,6 +2071,74 @@ validate_variant (GVariant *variant, return TRUE; } +/* TODO: make this public later; just wraps the previously public + * commit/dirtree/dirmeta verifiers. + */ +gboolean +_ostree_validate_structureof_metadata (OstreeObjectType objtype, + GVariant *metadata, + GError **error) +{ + g_assert (OSTREE_OBJECT_TYPE_IS_META (objtype)); + + switch (objtype) + { + case OSTREE_OBJECT_TYPE_COMMIT: + if (!ostree_validate_structureof_commit (metadata, error)) + return FALSE; + break; + case OSTREE_OBJECT_TYPE_DIR_TREE: + if (!ostree_validate_structureof_dirtree (metadata, error)) + return FALSE; + break; + case OSTREE_OBJECT_TYPE_DIR_META: + if (!ostree_validate_structureof_dirmeta (metadata, error)) + return FALSE; + break; + case OSTREE_OBJECT_TYPE_TOMBSTONE_COMMIT: + case OSTREE_OBJECT_TYPE_COMMIT_META: + /* TODO */ + break; + case OSTREE_OBJECT_TYPE_FILE: + g_assert_not_reached (); + break; + } + + return TRUE; +} + +/* Used by fsck as well as pull. Verify the checksum of a metadata object + * and its "structure" or the additional schema we impose on GVariants such + * as ensuring the "ay" checksum entries are of length 32. Another important + * one is checking for path traversal in dirtree objects. + */ +gboolean +_ostree_verify_metadata_object (OstreeObjectType objtype, + const char *expected_checksum, + GVariant *metadata, + GError **error) +{ + g_assert (expected_checksum); + + g_auto(OtChecksum) hasher = { 0, }; + ot_checksum_init (&hasher); + ot_checksum_update (&hasher, g_variant_get_data (metadata), g_variant_get_size (metadata)); + + char actual_checksum[OSTREE_SHA256_STRING_LEN+1]; + ot_checksum_get_hexdigest (&hasher, actual_checksum, sizeof (actual_checksum)); + if (!_ostree_compare_object_checksum (objtype, expected_checksum, actual_checksum, error)) + return FALSE; + + /* Add the checksum + objtype prefix here */ + { const char *error_prefix = glnx_strjoina (expected_checksum, ".", ostree_object_type_to_string (objtype)); + GLNX_AUTO_PREFIX_ERROR(error_prefix, error); + if (!_ostree_validate_structureof_metadata (objtype, metadata, error)) + return FALSE; + } + + return TRUE; +} + /** * ostree_validate_structureof_commit: * @commit: A commit object, %OSTREE_OBJECT_TYPE_COMMIT diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 4392f700cd..44434c667e 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2027,6 +2027,13 @@ ostree_repo_write_metadata (OstreeRepo *self, if (!metadata_size_valid (objtype, g_variant_get_size (normalized), error)) return FALSE; + /* For untrusted objects, verify their structure here */ + if (expected_checksum) + { + if (!_ostree_validate_structureof_metadata (objtype, object, error)) + return FALSE; + } + g_autoptr(GBytes) vdata = g_variant_get_data_as_bytes (normalized); if (!write_metadata_object (self, objtype, expected_checksum, vdata, out_csum, cancellable, error)) @@ -4101,6 +4108,7 @@ _ostree_repo_import_object (OstreeRepo *self, &variant, error)) return FALSE; + /* Note this one also now verifies structure in the !trusted case */ g_autofree guchar *real_csum = NULL; if (!ostree_repo_write_metadata (self, objtype, checksum, variant, diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index b2a00ea2cd..ab59c33a93 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -736,6 +736,12 @@ scan_dirtree_object (OtPullData *pull_data, g_variant_get_child (files_variant, i, "(&s@ay)", &filename, &csum); + /* Note this is now obsoleted by the _ostree_validate_structureof_metadata() + * but I'm keeping this since: + * 1) It's cheap + * 2) We want to continue to do validation for objects written to disk + * before libostree's validation was strengthened. + */ if (!ot_util_filename_validate (filename, error)) return FALSE; @@ -810,6 +816,7 @@ scan_dirtree_object (OtPullData *pull_data, g_variant_get_child (dirs_variant, i, "(&s@ay@ay)", &dirname, &tree_csum, &meta_csum); + /* See comment above for files */ if (!ot_util_filename_validate (dirname, error)) return FALSE; @@ -1222,24 +1229,20 @@ meta_fetch_on_complete (GObject *object, FALSE, &metadata, error)) goto out; - /* For commit objects, compute the hash and check the GPG signature before - * writing to the repo, and also write the .commitpartial to say that - * we're still processing this commit. + /* Compute checksum and verify structure now. Note this is a recent change + * (Jan 2018) - we used to verify the checksum only when writing down + * below. But we want to do "structure" verification early on as well + * before the object is written even to the staging directory. + */ + if (!_ostree_verify_metadata_object (objtype, checksum, metadata, error)) + goto out; + + /* For commit objects, check the GPG signature before writing to the repo, + * and also write the .commitpartial to say that we're still processing + * this commit. */ if (objtype == OSTREE_OBJECT_TYPE_COMMIT) { - /* Verify checksum */ - OtChecksum hasher = { 0, }; - ot_checksum_init (&hasher); - { g_autoptr(GBytes) bytes = g_variant_get_data_as_bytes (metadata); - ot_checksum_update_bytes (&hasher, bytes); - } - char hexdigest[OSTREE_SHA256_STRING_LEN+1]; - ot_checksum_get_hexdigest (&hasher, hexdigest, sizeof (hexdigest)); - - if (!_ostree_compare_object_checksum (objtype, checksum, hexdigest, error)) - goto out; - /* Do GPG verification. `detached_data` may be NULL if no detached * metadata was found during pull; that's handled by * gpg_verify_unwritten_commit(). If we ever change the pull code to @@ -1256,7 +1259,13 @@ meta_fetch_on_complete (GObject *object, goto out; } - ostree_repo_write_metadata_async (pull_data->repo, objtype, checksum, metadata, + /* Note that we now (Jan 2018) pass NULL for checksum, which means "don't + * verify checksum", since we just did it above. Related to this...now + * that we're doing all the verification here, one thing we could do later + * just `glnx_link_tmpfile_at()` into the repository, like the content + * fetch path does for trusted commits. + */ + ostree_repo_write_metadata_async (pull_data->repo, objtype, NULL, metadata, pull_data->cancellable, on_metadata_written, fetch_data); pull_data->n_outstanding_metadata_write_requests++; diff --git a/src/libostree/ostree-repo-static-delta-processing.c b/src/libostree/ostree-repo-static-delta-processing.c index 3b9fd49f72..bb6238e41f 100644 --- a/src/libostree/ostree-repo-static-delta-processing.c +++ b/src/libostree/ostree-repo-static-delta-processing.c @@ -497,8 +497,13 @@ dispatch_open_splice_and_close (OstreeRepo *repo, goto out; } - metadata = g_variant_new_from_data (ostree_metadata_variant_type (state->output_objtype), - state->payload_data + offset, length, TRUE, NULL, NULL); + /* Unfortunately we need a copy because GVariant wants pointer-alignment + * and we didn't guarantee that in static deltas. We can do so in the + * future. + */ + g_autoptr(GBytes) metadata_copy = g_bytes_new (state->payload_data + offset, length); + metadata = g_variant_new_from_bytes (ostree_metadata_variant_type (state->output_objtype), + metadata_copy, FALSE); { g_autofree guchar *actual_csum = NULL; diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index ec509e9d75..858a5555ca 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -3941,6 +3941,7 @@ ostree_repo_delete_object (OstreeRepo *self, return TRUE; } +/* Thin wrapper for _ostree_verify_metadata_object() */ static gboolean fsck_metadata_object (OstreeRepo *self, OstreeObjectType objtype, @@ -3956,39 +3957,7 @@ fsck_metadata_object (OstreeRepo *self, cancellable, error)) return FALSE; - g_auto(OtChecksum) hasher = { 0, }; - ot_checksum_init (&hasher); - ot_checksum_update (&hasher, g_variant_get_data (metadata), g_variant_get_size (metadata)); - - char actual_checksum[OSTREE_SHA256_STRING_LEN+1]; - ot_checksum_get_hexdigest (&hasher, actual_checksum, sizeof (actual_checksum)); - if (!_ostree_compare_object_checksum (objtype, sha256, actual_checksum, error)) - return FALSE; - - switch (objtype) - { - case OSTREE_OBJECT_TYPE_COMMIT: - if (!ostree_validate_structureof_commit (metadata, error)) - return FALSE; - break; - case OSTREE_OBJECT_TYPE_DIR_TREE: - if (!ostree_validate_structureof_dirtree (metadata, error)) - return FALSE; - break; - case OSTREE_OBJECT_TYPE_DIR_META: - if (!ostree_validate_structureof_dirmeta (metadata, error)) - return FALSE; - break; - case OSTREE_OBJECT_TYPE_TOMBSTONE_COMMIT: - case OSTREE_OBJECT_TYPE_COMMIT_META: - /* TODO */ - break; - case OSTREE_OBJECT_TYPE_FILE: - g_assert_not_reached (); - break; - } - - return TRUE; + return _ostree_verify_metadata_object (objtype, sha256, metadata, error); } static gboolean diff --git a/tests/pull-test.sh b/tests/pull-test.sh index 463b41efb5..8018e91a64 100644 --- a/tests/pull-test.sh +++ b/tests/pull-test.sh @@ -227,7 +227,10 @@ ${CMD_PREFIX} ostree --repo=corruptrepo remote add --set=gpg-verify=false pathtr if ${CMD_PREFIX} ostree --repo=corruptrepo pull pathtraverse pathtraverse-test 2>err.txt; then fatal "Pulled a repo with path traversal in dirtree" fi -assert_file_has_content_literal err.txt 'Invalid / in filename ../afile' +assert_file_has_content_literal err.txt 'ae9a5d2701a02740aa2ee317ba53b13e3efb0f29609cd4896e1bafeee4caddb5.dirtree: Invalid / in filename ../afile' +# And verify we didn't write the object into the staging directory even +find corruptrepo/tmp -name '9a5d2701a02740aa2ee317ba53b13e3efb0f29609cd4896e1bafeee4caddb5.dirtree' >find.txt +assert_not_file_has_content find.txt '9a5d2701a02740aa2ee317ba53b13e3efb0f29609cd4896e1bafeee4caddb5' rm corruptrepo -rf echo "ok path traversal checked on pull" diff --git a/tests/test-pull-untrusted.sh b/tests/test-pull-untrusted.sh index 247a34f9f1..5e35c1c3da 100755 --- a/tests/test-pull-untrusted.sh +++ b/tests/test-pull-untrusted.sh @@ -1,6 +1,7 @@ #!/bin/bash # # Copyright (C) 2014 Alexander Larsson +# Copyright (C) 2018 Red Hat, Inc. # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -22,7 +23,7 @@ set -euo pipefail . $(dirname $0)/libtest.sh -echo '1..3' +echo '1..4' setup_test_repository "bare" @@ -60,10 +61,20 @@ else fi rm -rf repo2 -mkdir repo2 ostree_repo_init repo2 --mode="bare" if ${CMD_PREFIX} ostree --repo=repo2 pull-local --untrusted repo; then assert_not_reached "corrupted untrusted pull unexpectedly failed!" else echo "ok untrusted pull with corruption failed" fi + + +cd ${test_tmpdir} +tar xf ${test_srcdir}/ostree-path-traverse.tar.gz +rm -rf repo2 +ostree_repo_init repo2 --mode=archive +if ${CMD_PREFIX} ostree --repo=repo2 pull-local --untrusted ostree-path-traverse/repo pathtraverse-test 2>err.txt; then + fatal "pull-local unexpectedly succeeded" +fi +assert_file_has_content_literal err.txt 'Invalid / in filename ../afile' +echo "ok untrusted pull-local path traversal"