A comment after the last semicolon ("SELECT 1; -- note") became a
statement of its own. It is not a read, so the whole batch asked for
confirmation and the comment was then sent to the database. Drop parts
that contain only comments.
PostgreSQL accepts a function name in double quotes, so
SELECT "pg_read_file"('/etc/passwd') passed the forbidden-construct
check and SELECT "setval"('seq', 1) skipped the confirmation: the
patterns expect the name right before the parenthesis. The scanner now
unquotes a plain identifier when a parenthesis follows it, so a quoted
call is checked like the bare function name; quoted column names stay
as they are.
Only dblink() and dblink_connect() were blocked, so dblink_exec(),
dblink_send_query() and the other functions of the extension could
still reach other databases. Match every function whose name starts
with dblink.
SET PASSWORD, SET GLOBAL and SET PERSIST (also as @@GLOBAL.x) change
the server or the database account rather than the session, so they
need confirmation like other writes. Session variables stay a read.
SHOW, EXPLAIN, VALUES and TABLE count as reads now but their rows were
dropped and reported as a write; every read except SET now returns its
rows. nextval() advances a sequence, so it needs confirmation like
setval().
removeComments() used regexes that also matched "--" and "/*" inside
string literals. A literal like '--' or '/*' then removed the code after
it from the checks while PostgreSQL still ran it, e.g.
COPY (SELECT '--') TO PROGRAM 'sh' passed the forbidden-construct check,
and a DELETE in a CTE between '/*' and '*/' literals skipped the
confirmation.
Comments and literals are now found by one scanner that walks the SQL
the way the database does ('...', E'...', "...", $tag$...$tag$, -- and
block comments), shared by removeComments(), removeLiterals() and
splitStatements().
splitStatements() only knew 'strings' and "identifiers", so a DO block
or function body ($$ ... ; ... $$) was cut at its inner semicolons and
failed, and a quote inside a comment (-- don't) opened a string that
never closed. It now also skips dollar-quoted bodies, E'...' strings
with backslash escapes, and comments. It moves to SqlGuard so it is
tested with the other checks.
OccController::run() now catches Throwable, so an Error thrown by a
command is logged and returned like an Exception instead of ending in
an HTTP 500.
Not changed on purpose: showing rows for statements with RETURNING or
data-modifying CTEs, and failure handling in the Makefile release
targets. Both came up in the same review but are unrelated to the
security fixes in this release.
A security review found three issues:
- Comments were removed without a space before matching the forbidden
patterns, so "TO/**/PROGRAM" became "TOPROGRAM" and passed the
COPY ... PROGRAM check while PostgreSQL still ran it. Comments are
now replaced by a space, and the COPY file check also covers E'...',
U&'...' and dollar-quoted file names.
- Only statements starting with DELETE or UPDATE asked for
confirmation, so a leading block comment, a data-modifying CTE
(WITH x AS (DELETE ...)) or DROP/TRUNCATE/INSERT ran without it.
Every statement that is not a plain read now needs confirmation,
decided on the server.
- occ output could contain escape sequences, backspaces and URLs from
user-controlled text (display names, file names) that rewrite or hide
text or show a clickable link. Only color codes are kept now, and
URLs are no longer turned into links.
The checks move to SqlGuard so they can be tested on their own. They
remain defense in depth: dynamic SQL can always get around a text
check, so the database role should not have superuser rights,
pg_execute_server_program or pg_read/write_server_files. A read that
starts with WITH now shows its rows like a SELECT.
dillard
Adphi