Revirew fixes

Author: artemgavrilov

contrib/pg_tde/src/include/keyring/keyring_api.h

@@ -15,6 +15,7 @@ typedef enum ProviderType
 
 #define TDE_KEY_NAME_LEN 256
 #define MAX_KEY_DATA_SIZE 32	/* maximum 256 bit encryption */
+#define MIN_KEY_DATA_SIZE 16	/* minimum 128 bit encryption */
 #define INTERNAL_KEY_LEN 16
 
 typedef struct KeyData

contrib/pg_tde/src/keyring/keyring_api.c

@@ -144,7 +144,17 @@ ValidateKey(KeyInfo *key)
 
 	if (key->data.len > MAX_KEY_DATA_SIZE)
 	{
-		ereport(WARNING, errmsg("invalid key: data length exceeds maximum allowed size"));
+		ereport(WARNING,
+			errmsg("invalid key: data length \"%u\" exceeds maximum allowed size \"%d\"",
+				   key->data.len, MAX_KEY_DATA_SIZE));
+		return false;
+	}
+
+	if (key->data.len < MIN_KEY_DATA_SIZE)
+	{
+		ereport(WARNING,
+			errmsg("invalid key: data length \"%u\" is less than minimum required size \"%d\"",
+				   key->data.len, MIN_KEY_DATA_SIZE));
 		return false;
 	}
 

contrib/pg_tde/src/keyring/keyring_kmip.c

@@ -175,6 +175,17 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode
 			return NULL;
 		}
 
+		if (key->data.len > sizeof(key->data.data))
+		{
+			ereport(WARNING, errmsg("keyring provider returned invalid key size: %d", key->data.len));
+			*return_code = KEYRING_CODE_INVALID_KEY;
+			pfree(key);
+			BIO_free_all(ctx.bio);
+			SSL_CTX_free(ctx.ssl);
+			free(keyp);
+			return NULL;
+		}
+
 		memset(key->name, 0, sizeof(key->name));
 		memcpy(key->name, key_name, strnlen(key_name, sizeof(key->name) - 1));
 		memcpy(key->data.data, keyp, key->data.len);

contrib/pg_tde/src/keyring/keyring_vault.c

@@ -273,6 +273,17 @@ get_key_by_name(GenericKeyring *keyring, const char *key_name, KeyringReturnCode
 	memcpy(key->name, key_name, strnlen(key_name, sizeof(key->name) - 1));
 	key->data.len = pg_b64_decode(responseKey, strlen(responseKey), (char *) key->data.data, MAX_KEY_DATA_SIZE);
 
+	if (key->data.len > MAX_KEY_DATA_SIZE)
+	{
+		*return_code = KEYRING_CODE_INVALID_KEY;
+		ereport(WARNING,
+				errmsg("keyring provider \"%s\" returned invalid key size: %d",
+					   vault_keyring->keyring.provider_name, key->data.len));
+		pfree(key);
+		key = NULL;
+		goto cleanup;
+	}
+
 cleanup:
 	if (str.ptr != NULL)
 		pfree(str.ptr);

contrib/pg_tde/t/expected/key_validation.out

@@ -17,11 +17,21 @@ SELECT pg_tde_create_key_using_database_key_provider('key2', 'test-file-provider
 
 (1 row)
 
+SELECT pg_tde_create_key_using_database_key_provider('key3', 'test-file-provider');
+ pg_tde_create_key_using_database_key_provider 
+-----------------------------------------------
+ 
+(1 row)
+
 SELECT pg_tde_set_key_using_database_key_provider('key1', 'test-file-provider');
 psql:<stdin>:1: WARNING:  invalid key: data length is zero
 psql:<stdin>:1: ERROR:  failed to retrieve principal key "key1" from key provider "test-file-provider"
 DETAIL:  Invalid key
 SELECT pg_tde_set_key_using_database_key_provider('key2', 'test-file-provider');
-psql:<stdin>:1: WARNING:  invalid key: data length exceeds maximum allowed size
+psql:<stdin>:1: WARNING:  invalid key: data length "4294967295" exceeds maximum allowed size "32"
 psql:<stdin>:1: ERROR:  failed to retrieve principal key "key2" from key provider "test-file-provider"
 DETAIL:  Invalid key
+SELECT pg_tde_set_key_using_database_key_provider('key3', 'test-file-provider');
+psql:<stdin>:1: WARNING:  invalid key: data length "15" is less than minimum required size "16"
+psql:<stdin>:1: ERROR:  failed to retrieve principal key "key3" from key provider "test-file-provider"
+DETAIL:  Invalid key

contrib/pg_tde/t/key_validation.pl

@@ -27,6 +27,9 @@
 PGTDE::psql($node, 'postgres',
 	"SELECT pg_tde_create_key_using_database_key_provider('key2', 'test-file-provider');"
 );
+PGTDE::psql($node, 'postgres',
+	"SELECT pg_tde_create_key_using_database_key_provider('key3', 'test-file-provider');"
+);
 
 
 corrupt_key_file('/tmp/pg_tde_test_key_validation.per');
@@ -38,6 +41,9 @@
 PGTDE::psql($node, 'postgres',
 	"SELECT pg_tde_set_key_using_database_key_provider('key2', 'test-file-provider');"
 );
+PGTDE::psql($node, 'postgres',
+	"SELECT pg_tde_set_key_using_database_key_provider('key3', 'test-file-provider');"
+);
 
 sub corrupt_key_file
 {
@@ -60,6 +66,12 @@ sub corrupt_key_file
 	  or BAIL_OUT("sysseek failed: $!");
 	syswrite($fh, pack("L*", 0xFFFFFFFF)) or BAIL_OUT("syswrite failed: $!");
 
+	# Corrupt the third page of the key file by setting key data length below minimum.
+	# Offset is TDE_KEY_NAME_LEN + MAX_KEY_DATA_SIZE. See keyring_api.h for details.
+	sysseek($fh, 256 + 32, SEEK_CUR)
+	  or BAIL_OUT("sysseek failed: $!");
+	syswrite($fh, pack("L*", 0x0000000F)) or BAIL_OUT("syswrite failed: $!");
+
 	close($fh)
 	  or BAIL_OUT("close failed: $!");
 }