fix: address security review findings for 1.0 GA#270
Merged
Conversation
Addresses findings 1-5 from the pre-GA security code review: 1. (High) Add superuser() checks to tp_spill_memtable and tp_force_merge so unprivileged users cannot trigger segment writes or compaction. 2. (High) Add cross-field validation in tpvector_recv to ensure total_size is consistent with index_name_len and entry_count, preventing out-of-bounds reads from crafted binary protocol input. 3. (High) Add REVOKE EXECUTE FROM PUBLIC on all administrative and debug functions (bm25_spill_index, bm25_force_merge, bm25_dump_index, bm25_summarize_index, bm25_debug_pageviz) in the install SQL and all upgrade migration scripts. 4. (Medium) Replace Assert-only bounds checks in tp_decompress_block with runtime ereport(ERROR) checks for count, doc_id_bits, and freq_bits. These protect against stack buffer overflows from corrupted on-disk segment data in release builds. 5. (Medium) Change bulk_load_threshold, memtable_spill_threshold, and segments_per_level GUCs from PGC_USERSET to PGC_SUSET so unprivileged users cannot manipulate resource-control parameters. Security test updated to cover all new restrictions.
Replace superuser() with object_ownercheck() for bm25_spill_index and bm25_force_merge. These are operational functions that the index owner (or any superuser) should be able to call — not just superusers. Remove REVOKE FROM PUBLIC on spill/merge since any user who owns the index should be able to call them. REVOKE remains on debug functions (dump, summarize, pageviz) which are superuser-only.
bm25_debug_pageviz was added in the 0.5.0 base install but not in any upgrade script, so it doesn't exist when upgrading from pre-0.5.0 versions (e.g., 0.4.2 → 0.5.0 → 1.0.0-dev). Use a conditional REVOKE to avoid failing on the missing function.
pgspot requires qualified operators in extension SQL.
Address code review feedback on the cross-field validation in tpvector_recv: use Size instead of int for min_payload to prevent integer overflow with large entry_count, and add per-entry validation loop to ensure each entry's lexeme data stays within the buffer. Also harden tpvector_out to check lexeme data bounds before memcpy, and fix security test to use fresh extension install so REVOKE statements are applied.
2603ba2 to
a80a3f0
Compare
- (6) Change log_scores and log_bmw_stats GUCs from PGC_USERSET to PGC_SUSET to prevent info disclosure in connection-pooled environments - (7) Gate file-writing debug functions (bm25_dump_index two-arg and bm25_debug_pageviz) behind #ifdef DEBUG_DUMP_INDEX; remove from GA install SQL and add DROP in upgrade scripts - (8) Document fixed LWLock tranche IDs 1001-1008 in README - (10) Set relocatable = false to prevent schema ambiguity with planner hooks - (11) Fix SQL injection in test/sql/validation.sql: %s -> %L for regconfig arguments in format() calls
mostafa
approved these changes
Mar 13, 2026
Member
mostafa
left a comment
There was a problem hiding this comment.
Thanks for addressing these issues. LGTM! 🚀
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses findings 1-5 from the pre-GA security code review:
superuser()checks totp_spill_memtableandtp_force_mergeso unprivileged users cannot trigger segment writes or full compactiontpvector_recvensuringtotal_sizeis consistent withindex_name_lenandentry_countREVOKE EXECUTE FROM PUBLICon all admin/debug functions in install SQL and all upgrade migration scriptsAssert()with runtimeereport(ERROR)forcount,doc_id_bits, andfreq_bitsintp_decompress_block— prevents stack buffer overflows from corrupted segments in release buildsbulk_load_threshold,memtable_spill_threshold, andsegments_per_leveltoPGC_SUSETLower-severity findings (6-11: logging GUCs, debug file paths, tranche IDs, prerelease warning, relocatable flag, test identifier quoting) are not addressed in this PR.
Testing