From 26fc37fe2de5c53175ec1cf79bf29a0777a1d6f8 Mon Sep 17 00:00:00 2001 From: Jakub Wartak Date: Fri, 21 Aug 2026 11:10:45 +0200 Subject: [PATCH vIDEAv1 3/5] Keep clear-text role passwords out of the server log Enabling cleartext_passwords_action to protect passwords could ironically cause them to be logged: an old client's CREATE/ALTER ROLE ... PASSWORD 'secret' fails under "disallow", and the resulting error attaches a STATEMENT line containing the full command text -- password included. The same text also reaches the log via log_statement and duration logging. Two changes address this for the simple query protocol: - The clear-text WARNING/ERROR raised in encrypt_password() now uses errhidestmt(true), so the report records that a clear-text password was used without dragging the offending statement (and its secret) into the log. - A new helper, redact_password_literals() in the parser, rewrites the string literal of every CREATE/ALTER ROLE ... PASSWORD option to '***'. It re-lexes the query to find each literal's exact extent (the technique pg_stat_statements uses) and is allocation-free when there is no such option. exec_simple_query() applies it to the text used for statement logging, duration logging, the error STATEMENT line (debug_query_string) and pg_stat_activity. The audit record is preserved; only the password literal is masked, and non-password literals are left untouched. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/backend/libpq/crypt.c | 15 +- src/backend/parser/parser.c | 159 ++++++++++++++++++ src/backend/tcop/postgres.c | 34 +++- src/include/parser/parser.h | 3 + .../test_misc/t/015_password_command.pl | 38 +++++ 5 files changed, 243 insertions(+), 6 deletions(-) diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c index 9b20efe9ca2..9ba564b26f4 100644 --- a/src/backend/libpq/crypt.c +++ b/src/backend/libpq/crypt.c @@ -211,7 +211,15 @@ encrypt_password(PasswordType target_type, const char *role, errdetail("Sending a password using plain text is deprecated and may be removed in a future release of PostgreSQL."), strncmp(application_name, "psql", 5) == 0 ? errhint("If using psql, you can set the password with \\password") - : errhint("Use a client that can change the password without sending it in clear text"))); + : errhint("Use a client that can change the password without sending it in clear text"), + + /* + * The offending statement text contains the clear text password, + * so keep it out of the log: do not attach the STATEMENT line to + * this report. Otherwise enabling this check to protect passwords + * would itself cause them to be logged. + */ + errhidestmt(true))); } else if (cleartext_passwords_action == CLEARTEXT_ACTION_DISALLOW) { @@ -221,7 +229,10 @@ encrypt_password(PasswordType target_type, const char *role, errdetail("Sending a password using plain text is not allowed."), strncmp(application_name, "psql", 5) == 0 ? errhint("If using psql, you can change the password with \\password") - : errhint("Use a client that can change the password without sending it in clear text"))); + : errhint("Use a client that can change the password without sending it in clear text"), + + /* Keep the clear text password out of the log; see above. */ + errhidestmt(true))); } else { diff --git a/src/backend/parser/parser.c b/src/backend/parser/parser.c index b5c28523971..e941db9397f 100644 --- a/src/backend/parser/parser.c +++ b/src/backend/parser/parser.c @@ -22,6 +22,7 @@ #include "postgres.h" #include "gramparse.h" +#include "lib/stringinfo.h" #include "mb/pg_wchar.h" #include "parser/parser.h" #include "parser/scansup.h" @@ -29,6 +30,7 @@ static bool check_uescapechar(unsigned char escape); static char *str_udeescape(const char *str, char escape, int position, core_yyscan_t yyscanner); +static int int_cmp(const void *a, const void *b); /* @@ -85,6 +87,163 @@ raw_parser(const char *str, RawParseMode mode) return yyextra.parsetree; } +static int +int_cmp(const void *a, const void *b) +{ + return *(const int *) a - *(const int *) b; +} + +/* + * redact_password_literals + * + * Given a query string and the raw parse trees produced from it, return a copy + * of the query string in which the string literal of every + * CREATE/ALTER ROLE ... PASSWORD option has been replaced with '***'. + * + * This keeps clear-text (or client-hashed) passwords out of the server log and + * pg_stat_activity: even when statement or duration logging would otherwise + * print the command text, the password value is masked, while the rest of the + * statement remains visible for auditing. It complements the PASSWORD command, + * which avoids the problem entirely by passing the secret as a bind parameter. + * + * Returns NULL if the query contains no such literal, in which case the caller + * should just use the original string. A non-NULL result is palloc'd in the + * current memory context. + * + * List elements may be RawStmt nodes (as returned by raw_parser) or bare + * statement nodes; RawStmt wrappers are unwrapped automatically. + */ +char * +redact_password_literals(const char *query_string, List *parsetrees) +{ + List *optlist = NIL; + int *optlocs; + int nlocs; + int k; + ListCell *lc; + core_yyscan_t yyscanner; + core_yy_extra_type yyextra; + core_YYSTYPE yylval; + YYLTYPE yylloc; + StringInfoData buf; + int pos; + int tok; + + /* + * Collect the location of every role PASSWORD option in the query. This + * runs for every parsed statement, so take care to do no allocation in the + * common case where there is no such option: optlist stays NIL and we + * return immediately. + */ + foreach(lc, parsetrees) + { + Node *stmt = (Node *) lfirst(lc); + List *options; + ListCell *oc; + + if (stmt != NULL && IsA(stmt, RawStmt)) + stmt = ((RawStmt *) stmt)->stmt; + if (stmt == NULL) + continue; + + if (IsA(stmt, CreateRoleStmt)) + options = ((CreateRoleStmt *) stmt)->options; + else if (IsA(stmt, AlterRoleStmt)) + options = ((AlterRoleStmt *) stmt)->options; + else + continue; + + foreach(oc, options) + { + DefElem *defel = (DefElem *) lfirst(oc); + + /* PASSWORD NULL has a NULL arg and carries no literal to hide. */ + if (strcmp(defel->defname, "password") != 0 || + defel->arg == NULL || + defel->location < 0) + continue; + + optlist = lappend_int(optlist, defel->location); + } + } + + if (optlist == NIL) + return NULL; /* nothing to redact */ + + /* Move the locations into an array and process them in query order. */ + nlocs = list_length(optlist); + optlocs = palloc_array(int, nlocs); + k = 0; + foreach(lc, optlist) + optlocs[k++] = lfirst_int(lc); + list_free(optlist); + if (nlocs > 1) + qsort(optlocs, nlocs, sizeof(int), int_cmp); + + /* + * Re-lex the query to find the string literal that follows each PASSWORD + * option keyword, and copy the query into "buf" with each such literal + * replaced by '***'. We rely, as pg_stat_statements does, on flex having + * placed a NUL after the current token in scanbuf, so that the raw length + * of a literal is strlen(scanbuf + its location). + */ + yyscanner = scanner_init(query_string, &yyextra, + &ScanKeywords, ScanKeywordTokens); + initStringInfo(&buf); + pos = 0; + tok = core_yylex(&yylval, &yylloc, yyscanner); + + for (int i = 0; i < nlocs; i++) + { + int optloc = optlocs[i]; + int litloc = -1; + int litlen = 0; + int steps; + + /* Advance to the option's leading keyword (PASSWORD or ENCRYPTED). */ + while (tok != 0 && yylloc < optloc) + tok = core_yylex(&yylval, &yylloc, yyscanner); + if (tok == 0) + break; + + /* + * The value is the next string literal, within a couple of tokens + * (allowing for a leading ENCRYPTED/UNENCRYPTED keyword). If we do not + * find one that soon, this was PASSWORD NULL or similar, so skip it. + */ + for (steps = 0; tok != 0 && steps < 3; steps++) + { + if (tok == SCONST || tok == USCONST) + { + litloc = yylloc; + litlen = strlen(yyextra.scanbuf + yylloc); + break; + } + tok = core_yylex(&yylval, &yylloc, yyscanner); + } + + if (litloc < 0) + continue; + + if (litloc > pos) + appendBinaryStringInfo(&buf, query_string + pos, litloc - pos); + appendStringInfoString(&buf, "'***'"); + pos = litloc + litlen; + + /* Move past the literal before hunting for the next option. */ + tok = core_yylex(&yylval, &yylloc, yyscanner); + } + + scanner_finish(yyscanner); + + /* Copy whatever remains after the last masked literal. */ + appendStringInfoString(&buf, query_string + pos); + + pfree(optlocs); + + return buf.data; +} + /* * Intermediate filter between parser and core lexer (core_yylex in scan.l). diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index bf90bed9383..86c28585137 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -1040,6 +1040,13 @@ exec_simple_query(const char *query_string) bool use_implicit_block; char msec_str[32]; + /* + * Text used when logging this query. If the query embeds a role password + * as a literal, this is replaced below with a redacted copy so the secret + * is not written to the log. + */ + const char *log_query_string = query_string; + /* * Report query to various monitoring facilities. */ @@ -1106,14 +1113,33 @@ exec_simple_query(const char *query_string) } } + /* + * If the query embeds a role password as a literal (CREATE/ALTER ROLE ... + * PASSWORD '...'), mask it in the text used for logging and monitoring, so + * the secret is not written to the server log, attached to error reports, + * or shown in pg_stat_activity. The rest of the statement is preserved so + * the command remains visible for auditing. + */ + { + char *redacted = redact_password_literals(query_string, + parsetree_list); + + if (redacted != NULL) + { + log_query_string = redacted; + debug_query_string = redacted; + pgstat_report_activity(STATE_RUNNING, redacted); + } + } + /* Log immediately if dictated by log_statement */ if (check_log_statement(parsetree_list)) { - char *truncated_stmt = truncate_query_log(query_string); + char *truncated_stmt = truncate_query_log(log_query_string); ereport(LOG, (errmsg("statement: %s", - (truncated_stmt != NULL) ? truncated_stmt : query_string), + (truncated_stmt != NULL) ? truncated_stmt : log_query_string), errhidestmt(true), errdetail_execute(parsetree_list))); was_logged = true; @@ -1413,12 +1439,12 @@ exec_simple_query(const char *query_string) break; case 2: { - char *truncated_stmt = truncate_query_log(query_string); + char *truncated_stmt = truncate_query_log(log_query_string); ereport(LOG, (errmsg("duration: %s ms statement: %s", msec_str, - (truncated_stmt != NULL) ? truncated_stmt : query_string), + (truncated_stmt != NULL) ? truncated_stmt : log_query_string), errhidestmt(true), errdetail_execute(parsetree_list))); diff --git a/src/include/parser/parser.h b/src/include/parser/parser.h index 93bd7c439f8..0839a3edd23 100644 --- a/src/include/parser/parser.h +++ b/src/include/parser/parser.h @@ -59,6 +59,9 @@ extern PGDLLIMPORT int backslash_quote; /* Primary entry point for the raw parsing functions */ extern List *raw_parser(const char *str, RawParseMode mode); +extern char *redact_password_literals(const char *query_string, + List *parsetrees); + /* Utility functions exported by gram.y (perhaps these should be elsewhere) */ extern List *SystemFuncName(char *name); extern TypeName *SystemTypeName(char *name); diff --git a/src/test/modules/test_misc/t/015_password_command.pl b/src/test/modules/test_misc/t/015_password_command.pl index f38f81face3..3e5c1b9202c 100644 --- a/src/test/modules/test_misc/t/015_password_command.pl +++ b/src/test/modules/test_misc/t/015_password_command.pl @@ -118,4 +118,42 @@ isnt($ret, 0, "unprivileged role cannot change another role's password"); like($err, qr/permission denied to alter role/, "changing another role's password is denied"); +# +# 4. Old clients that send a clear-text password inline via CREATE/ALTER ROLE +# must not have it written to the log: the password literal is masked while +# the rest of the statement is preserved for auditing. (log_statement is +# 'all' for this node, so the statement text is logged.) +# +note "clear-text password in ALTER ROLE is redacted from the log"; +my $cleartext = 'cleartext_ALTER_TOKEN_do_not_log'; +$log_offset = -s $node->logfile; +$node->safe_psql('postgres', + "SET cleartext_passwords_action = allow; " + . "ALTER ROLE regress_pw_alice PASSWORD '$cleartext';"); +ok(!$node->log_contains(qr/\Q$cleartext\E/, $log_offset), + "clear-text password in ALTER ROLE is not logged"); +ok( $node->log_contains( + qr/ALTER ROLE regress_pw_alice PASSWORD '\*\*\*'/, $log_offset), + "ALTER ROLE is logged with the password masked"); + +# A non-password string literal in the same message must be left intact. +note "non-password literals are not masked"; +$log_offset = -s $node->logfile; +$node->safe_psql('postgres', "SELECT 'ordinary_literal_TOKEN' AS x"); +ok( $node->log_contains(qr/ordinary_literal_TOKEN/, $log_offset), + "an ordinary string literal is logged unchanged"); + +# Under disallow the rejection must also keep the secret out of the log, +# including the error's STATEMENT line. +note "rejected clear-text ALTER ROLE does not leak the password"; +my $cleartext2 = 'cleartext_DISALLOW_TOKEN_zzz'; +$log_offset = -s $node->logfile; +($ret, $out, $err) = $node->psql('postgres', + "SET cleartext_passwords_action = disallow; " + . "ALTER ROLE regress_pw_alice PASSWORD '$cleartext2';"); +isnt($ret, 0, "disallow rejects a clear-text ALTER ROLE"); +like($err, qr/using a clear text password/, "disallow error is reported"); +ok(!$node->log_contains(qr/\Q$cleartext2\E/, $log_offset), + "rejected clear-text password is not logged"); + done_testing(); -- 2.43.0