From 19bef897e6afc06959a99ed697575ba3eb71b0f9 Mon Sep 17 00:00:00 2001 From: Andreas Karlsson Date: Mon, 14 Apr 2025 15:54:51 +0200 Subject: [PATCH] PG-1510 Use a unique IV per relation fork The security of the encryption is reduced if we reuse the same initiation vector more than necessary so we make sure to use a unique IV per relation fork, with the exception of the initialization fork which is used by unlogged indexes when restarting the server after a crash. It is copied with low-level file system functions to the main fork on crash recovery so it needs to use the same IV as the main fork. The init fork issue is in no way more a security issue than to the extent that ideally we should pick a new IV when truncating unlogged tables on crash recovery but to fix this we would need to change the SMGR API and moving the copying of the intialization fork into that. And in the long term this might be what we want to do. --- contrib/pg_tde/meson.build | 1 + contrib/pg_tde/src/smgr/pg_tde_smgr.c | 16 +++-- contrib/pg_tde/t/011_unlogged_tables.pl | 69 +++++++++++++++++++ .../pg_tde/t/expected/011_unlogged_tables.out | 12 ++++ 4 files changed, 92 insertions(+), 6 deletions(-) create mode 100644 contrib/pg_tde/t/011_unlogged_tables.pl create mode 100644 contrib/pg_tde/t/expected/011_unlogged_tables.out diff --git a/contrib/pg_tde/meson.build b/contrib/pg_tde/meson.build index 17974dcc05b..5a95a4c8ba7 100644 --- a/contrib/pg_tde/meson.build +++ b/contrib/pg_tde/meson.build @@ -114,6 +114,7 @@ tap_tests = [ 't/008_key_rotate_tablespace.pl', 't/009_wal_encrypt.pl', 't/010_change_key_provider.pl', + 't/011_unlogged_tables.pl', ] tests += { diff --git a/contrib/pg_tde/src/smgr/pg_tde_smgr.c b/contrib/pg_tde/src/smgr/pg_tde_smgr.c index fd7d20e7092..a83fb9ebda8 100644 --- a/contrib/pg_tde/src/smgr/pg_tde_smgr.c +++ b/contrib/pg_tde/src/smgr/pg_tde_smgr.c @@ -26,7 +26,7 @@ typedef struct TDESMgrRelationData typedef TDESMgrRelationData *TDESMgrRelation; -static void CalcBlockIv(BlockNumber bn, const unsigned char *base_iv, unsigned char *iv); +static void CalcBlockIv(ForkNumber forknum, BlockNumber bn, const unsigned char *base_iv, unsigned char *iv); static InternalKey * tde_smgr_get_key(SMgrRelation reln, RelFileLocator *old_locator, bool can_create) @@ -102,7 +102,7 @@ tde_mdwritev(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, local_buffers[i] = &local_blocks_aligned[i * BLCKSZ]; - CalcBlockIv(bn, int_key->base_iv, iv); + CalcBlockIv(forknum, bn, int_key->base_iv, iv); AesEncrypt(int_key->key, iv, ((unsigned char **) buffers)[i], BLCKSZ, local_buffers[i]); } @@ -134,7 +134,7 @@ tde_mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, AesInit(); - CalcBlockIv(blocknum, int_key->base_iv, iv); + CalcBlockIv(forknum, blocknum, int_key->base_iv, iv); AesEncrypt(int_key->key, iv, ((unsigned char *) buffer), BLCKSZ, local_blocks_aligned); @@ -184,7 +184,7 @@ tde_mdreadv(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum, if (allZero) continue; - CalcBlockIv(bn, int_key->base_iv, iv); + CalcBlockIv(forknum, bn, int_key->base_iv, iv); AesDecrypt(int_key->key, iv, ((unsigned char **) buffers)[i], BLCKSZ, ((unsigned char **) buffers)[i]); } @@ -297,13 +297,17 @@ RegisterStorageMgr(void) /* * The intialization vector of a block is its block number conmverted to a - * 128 bit big endian number XOR the base IV of the relation file. + * 128 bit big endian number plus the forknumber XOR the base IV of the + * relation file. */ static void -CalcBlockIv(BlockNumber bn, const unsigned char *base_iv, unsigned char *iv) +CalcBlockIv(ForkNumber forknum, BlockNumber bn, const unsigned char *base_iv, unsigned char *iv) { memset(iv, 0, 16); + /* The init fork is copied to the main fork so we must use the same IV */ + iv[7] = forknum == INIT_FORKNUM ? MAIN_FORKNUM : forknum; + iv[12] = bn >> 24; iv[13] = bn >> 16; iv[14] = bn >> 8; diff --git a/contrib/pg_tde/t/011_unlogged_tables.pl b/contrib/pg_tde/t/011_unlogged_tables.pl new file mode 100644 index 00000000000..22e7c5f10c6 --- /dev/null +++ b/contrib/pg_tde/t/011_unlogged_tables.pl @@ -0,0 +1,69 @@ +#!/usr/bin/perl + +use strict; +use warnings; +use File::Basename; +use File::Compare; +use File::Copy; +use Test::More; +use lib 't'; +use pgtde; + +# Get file name and CREATE out file name and dirs WHERE requried +PGTDE::setup_files_dir(basename($0)); + +# CREATE new PostgreSQL node and do initdb +my $node = PGTDE->pgtde_init_pg(); +my $pgdata = $node->data_dir; + +# UPDATE postgresql.conf to include/load pg_tde library +open my $conf, '>>', "$pgdata/postgresql.conf"; +print $conf "shared_preload_libraries = 'pg_tde'\n"; +close $conf; + +# Start server +my $rt_value = $node->start; +ok($rt_value == 1, "Start Server"); + +my ($cmdret, $stdout, $stderr) = $node->psql('postgres', 'CREATE EXTENSION IF NOT EXISTS pg_tde;', extra_params => ['-a']); +ok($cmdret == 0, "CREATE PGTDE EXTENSION"); +PGTDE::append_to_file($stdout); +$stdout = $node->safe_psql('postgres', "SELECT pg_tde_add_database_key_provider_file('file-vault', '/tmp/unlogged_tables.per');", extra_params => ['-a']); +PGTDE::append_to_file($stdout); +$stdout = $node->safe_psql('postgres', "SELECT pg_tde_set_key_using_database_key_provider('test-key', 'file-vault');", extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', "CREATE UNLOGGED TABLE t (x int PRIMARY KEY) USING tde_heap;", extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', "INSERT INTO t SELECT generate_series(1, 4);", extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', "CHECKPOINT;", extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +PGTDE::append_to_file("-- kill -9"); +$node->kill9(); + +# Start server +PGTDE::append_to_file("-- server start"); +$rt_value = $node->start; +ok($rt_value == 1, "Start Server"); + +$stdout = $node->safe_psql('postgres', "TABLE t;", extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', "INSERT INTO t SELECT generate_series(1, 4);", extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +# Stop the server +$node->stop(); + +# compare the expected and out file +my $compare = PGTDE->compare_results(); + +# Test/check if expected and result/out file match. If Yes, test passes. +is($compare, 0, "Compare Files: $PGTDE::expected_filename_with_path and $PGTDE::out_filename_with_path files."); + +# Done testing for this testcase file. +done_testing(); diff --git a/contrib/pg_tde/t/expected/011_unlogged_tables.out b/contrib/pg_tde/t/expected/011_unlogged_tables.out new file mode 100644 index 00000000000..031fd4f80b2 --- /dev/null +++ b/contrib/pg_tde/t/expected/011_unlogged_tables.out @@ -0,0 +1,12 @@ +CREATE EXTENSION IF NOT EXISTS pg_tde; +SELECT pg_tde_add_database_key_provider_file('file-vault', '/tmp/unlogged_tables.per'); +1 +SELECT pg_tde_set_key_using_database_key_provider('test-key', 'file-vault'); + +CREATE UNLOGGED TABLE t (x int PRIMARY KEY) USING tde_heap; +INSERT INTO t SELECT generate_series(1, 4); +CHECKPOINT; +-- kill -9 +-- server start +TABLE t; +INSERT INTO t SELECT generate_series(1, 4);