From 0c738b2619851f881791718a80a2fbb64d617752 Mon Sep 17 00:00:00 2001 From: Kevin Albertson Date: Thu, 30 Jul 2026 17:28:29 -0400 Subject: [PATCH 1/2] add test --- test/test-gcp-auth.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/test/test-gcp-auth.c b/test/test-gcp-auth.c index 02f0d016a..f582671f8 100644 --- a/test/test-gcp-auth.c +++ b/test/test-gcp-auth.c @@ -242,7 +242,58 @@ static void _test_encrypt_with_accesstoken(_mongocrypt_tester_t *tester) { mongocrypt_destroy(crypt); } +// Test a CRLF in a key vault document's keyName. Regression test for MONGOCRYPT-960. +static void _test_crlf_in_gcp_masterkey(_mongocrypt_tester_t *tester) { + mongocrypt_t *crypt; + mongocrypt_ctx_t *ctx; + mongocrypt_binary_t *uuid; + const char *uuid_data = "\x61\x61\x61\x61\x61\x61\x61\x61\x61\x61\x61\x61\x61\x61\x61\x61"; + + // A key vault document whose masterKey.keyName contains a CRLF. + // clang-format off + const char *key_doc = BSON_STR({ + "status" : 1, + "_id" : {"$binary" : {"base64" : "YWFhYWFhYWFhYWFhYWFhYQ==", "subType" : "04"}}, + "masterKey" : { + "provider" : "gcp", + "projectId" : "test", + "location" : "global", + "keyRing" : "test", + "keyName" : "test\r\nFOOBAR: injected" + }, + "updateDate" : {"$date" : {"$numberLong" : "1557827033449"}}, + "creationDate" : {"$date" : {"$numberLong" : "1557827033449"}}, + "keyMaterial" : { + "$binary" : { + "base64" : "PwwOESbKs57YTJtGSCsuAJbv9VWRHjLdPdziUVxH0K9woZUka4SghSwJlw9n6TU/dYHFODvSa4p7bfKzS6U8kGFvOnd7LcPU63IkTek4qZEbGpTRwI5lMT4FFWdpYpUf", + "subType" : "00" + } + } + }); + // clang-format on + + crypt = mongocrypt_new(); + ASSERT_OK(mongocrypt_setopt_kms_providers(crypt, TEST_BSON("{'gcp': {'accessToken': 'foobar'}}")), crypt); + ASSERT_OK(_mongocrypt_init_for_test(crypt), crypt); + ctx = mongocrypt_ctx_new(crypt); + uuid = mongocrypt_binary_new_from_data((uint8_t *)uuid_data, UUID_LEN); + ASSERT_OK(mongocrypt_ctx_setopt_key_id(ctx, uuid), ctx); + ASSERT_OK(mongocrypt_ctx_setopt_algorithm(ctx, MONGOCRYPT_ALGORITHM_DETERMINISTIC_STR, -1), ctx); + ASSERT_OK(mongocrypt_ctx_explicit_encrypt_init(ctx, TEST_BSON("{'v': 1}")), ctx); + + ASSERT_STATE_EQUAL(mongocrypt_ctx_state(ctx), MONGOCRYPT_CTX_NEED_MONGO_KEYS); + /* Constructing the KMS request fails. No request is sent. */ + ASSERT_FAILS(mongocrypt_ctx_mongo_feed(ctx, TEST_BSON_STR(key_doc)), + ctx, + "Invalid character in GCP KMS key identifier: keyName"); + + mongocrypt_binary_destroy(uuid); + mongocrypt_ctx_destroy(ctx); + mongocrypt_destroy(crypt); +} + void _mongocrypt_tester_install_gcp_auth(_mongocrypt_tester_t *tester) { + INSTALL_TEST(_test_crlf_in_gcp_masterkey); INSTALL_TEST(_test_createdatakey_with_credentials); INSTALL_TEST(_test_encrypt_with_credentials); INSTALL_TEST(_test_createdatakey_with_accesstoken); From eefbfdd6103ab8ff305c7040246af3cc425795bb Mon Sep 17 00:00:00 2001 From: Kevin Albertson Date: Thu, 30 Jul 2026 17:28:43 -0400 Subject: [PATCH 2/2] add validation --- kms-message/src/kms_gcp_request.c | 42 +++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/kms-message/src/kms_gcp_request.c b/kms-message/src/kms_gcp_request.c index b2111db99..04d1b112c 100644 --- a/kms-message/src/kms_gcp_request.c +++ b/kms-message/src/kms_gcp_request.c @@ -20,6 +20,8 @@ #include "kms_message_private.h" #include "kms_request_opt_private.h" +#include + /* Set a default expiration of 5 minutes for JSON Web Tokens (GCP allows up to * one hour) */ #define JWT_EXPIRATION_SECS 5 * 60 @@ -145,6 +147,38 @@ kms_gcp_request_oauth_new (const char *host, return req; } +/* _check_key_identifier returns true if `value` is safe to use as a URL path segment. + * + * Key identifiers may originate from key vault documents or caller-supplied options. They are + * formatted into the request path, so an unescaped '/', '?', '#', space, or CR/LF would rewrite + * the request target or inject header lines into the request. + * + * Only the RFC 3986 unreserved characters are permitted. This is a superset of what GCP itself + * accepts, so it does not reject any identifier that would otherwise work: + * - keyRing and keyName must match `[a-zA-Z0-9_-]{1,63}`. See + * https://cloud.google.com/kms/docs/reference/rest/v1/projects.locations.keyRings/create and + * https://cloud.google.com/kms/docs/reference/rest/v1/projects.locations.keyRings.cryptoKeys/create + * - projectId is 6-30 characters of lowercase letters, numbers, and hyphens. See + * https://cloud.google.com/resource-manager/docs/creating-managing-projects + * - location is a Cloud location name (e.g. "global", "us-east1"). + * - keyVersion is a positive integer. + * + * `value` is not included in the error message: it is untrusted, and the error may be logged. */ +static bool +_check_key_identifier (kms_request_t *req, const char *name, const char *value) +{ + const char *c; + + for (c = value; *c != '\0'; c++) { + if (!isalnum ((unsigned char) *c) && *c != '-' && *c != '.' && *c != '_' && + *c != '~') { + KMS_ERROR (req, "Invalid character in GCP KMS key identifier: %s", name); + return false; + } + } + return true; +} + static kms_request_t * _encrypt_decrypt_common (const char *encrypt_decrypt, const char *host, @@ -192,6 +226,14 @@ _encrypt_decrypt_common (const char *encrypt_decrypt, goto done; } + if (!_check_key_identifier (req, "projectId", project_id) || + !_check_key_identifier (req, "location", location) || + !_check_key_identifier (req, "keyRing", key_ring_name) || + !_check_key_identifier (req, "keyName", key_name) || + (key_version && !_check_key_identifier (req, "keyVersion", key_version))) { + goto done; + } + value_base64 = kms_message_raw_to_b64 (value, value_len); if (!value_base64) { KMS_ERROR (req, "Could not bases64-encode plaintext");