Skip to content

Fix use-after-free in pgsodium.getkey_script boot value - #126

Open
msakrejda wants to merge 2 commits into
michelp:mainfrom
msakrejda:fix-getkey-script-boot-val
Open

Fix use-after-free in pgsodium.getkey_script boot value#126
msakrejda wants to merge 2 commits into
michelp:mainfrom
msakrejda:fix-getkey-script-boot-val

Conversation

@msakrejda

Copy link
Copy Markdown

Disclaimer: this was analyzed and fixed with Claude Opus. I'm not much
of a Postgres hacker (or C hacker, for that matter), but its analysis and the
fix make sense to me, and I was able to observe the problem reading the
reset_value of the setting.

I put the failing test first in its own commit; these should probably be
combined.


_PG_init() built the default value for pgsodium.getkey_script in a
palloc0'd buffer and passed it to DefineCustomStringVariable().

DefineCustomStringVariable() stores the bootValue pointer as-is
without copying the string, and retains it for the lifetime of the
process. Because _PG_init() runs from
process_shared_preload_libraries() with PostmasterContext current,
that buffer is allocated in PostmasterContext. This is freed at
backend startup during InitPostgres().

GetConfigOptionValues() dereferences boot_val lazily on each read, so
after backend startup

SELECT boot_val FROM pg_settings
 WHERE name = 'pgsodium.getkey_script';

reads freed memory and returns whatever bytes have since reused that
block. It also means any user with access to pg_settings can observe
freed heap contents from their own backend.

Note that "setting" and "reset_val" are unaffected:
InitializeOneGUCOption() guc_strdup()s boot_val into GUC memory in the
postmaster, while the buffer is still valid.

Use a static buffer for the boot value so it outlives PostmasterContext.
Static storage also keeps this correct under EXEC_BACKEND, where
_PG_init() runs again in each child.

This was originally introduced in 5c965ea (v1.3.0-alpha) along with the
GUC itself, then fixed in f57bd69 ("fix for using palloc in init") by
switching to malloc(), which leaves the buffer alive for the life of the
process. It was reintroduced in 06e7ca4 when that malloc() was changed
back to palloc0().

Asserts that pg_settings.boot_val for pgsodium.getkey_script is the
built-in default path, which is only true if the buffer backing the GUC's
boot value outlives PostmasterContext.

Guarded on :serverkeys, since the GUC only exists when pgsodium is in
shared_preload_libraries. The assertion holds whether or not
pgsodium.getkey_script has been overridden in the config, so it is valid
in both preloaded configurations exercised by test.sh.
_PG_init() built the default value for pgsodium.getkey_script in a
palloc0'd buffer and passed it to DefineCustomStringVariable().

DefineCustomStringVariable() stores the bootValue pointer as-is
without copying the string, and retains it for the lifetime of the
process. Because _PG_init() runs from
process_shared_preload_libraries() with PostmasterContext current,
that buffer is allocated in PostmasterContext. This is freed at
backend startup during InitPostgres().

GetConfigOptionValues() dereferences boot_val lazily on each read, so
after backend startup

    SELECT boot_val FROM pg_settings
     WHERE name = 'pgsodium.getkey_script';

reads freed memory and returns whatever bytes have since reused that
block. It also means any user with access to pg_settings can observe
freed heap contents from their own backend.

Note that "setting" and "reset_val" are unaffected:
InitializeOneGUCOption() guc_strdup()s boot_val into GUC memory in the
postmaster, while the buffer is still valid.

Use a static buffer for the boot value so it outlives PostmasterContext.
Static storage also keeps this correct under EXEC_BACKEND, where
_PG_init() runs again in each child.

This was originally introduced in 5c965ea (v1.3.0-alpha) along with the
GUC itself, then fixed in f57bd69 ("fix for using palloc in init") by
switching to malloc(), which leaves the buffer alive for the life of the
process. It was reintroduced in 06e7ca4 when that malloc() was changed
back to palloc0().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant