Procedures versus the "fastpath" API
The security team received a report from Theodor-Arsenij
Larionov-Trichkin of PostgresPro that it's possible to crash the
backend with an assertion or null-pointer dereference by trying to
call a window function via the "fast path function call" protocol
message. fastpath.c doesn't set up any WindowObject function context,
of course, but most of the built-in window functions just assume there
will be one. We concluded that there's no possibility of anything
more interesting than an immediate core dump, so per our usual rules
this isn't a CVE-grade bug. However, we poked around to see if there
were any related problems, and soon found that fastpath.c will happily
attempt to call procedures as well as functions. That seems to work,
accidentally IMO, for simple procedures --- but if the procedure tries
to COMMIT or ROLLBACK then you get "ERROR: invalid transaction
termination". (There might be other edge-case problems; I've not
tried subtransactions or OUT parameters for example.)
So the question on the table is what to do about this. As far as
window functions go, it seems clear that fastpath.c should just reject
any attempt to call a window function that way (or an aggregate for
that matter; aggregates fail already, but with relatively obscure
error messages). Perhaps there's also an argument that window
functions should have run-time tests, not just assertions, that
they're called in a valid way.
As for procedures, I'm of the opinion that we should just reject those
too, but some other security team members were not happy with that
idea. Conceivably we could attempt to make the case actually work,
but is it worth the trouble? Given the lack of field complaints
about the "invalid transaction termination" failure, it seems unlikely
that it's occurred to anyone to try to call procedures this way.
We'd need special infrastructure to test the case, too, since psql
offers no access to fastpath calls.
A compromise suggestion was to prohibit calling procedures via
fastpath as of HEAD, but leave existing releases as they are,
in case anyone is using a case that happens to work.
Thoughts?
regards, tom lane
On 3/9/21 2:15 PM, Tom Lane wrote:
So the question on the table is what to do about this. As far as
window functions go, it seems clear that fastpath.c should just reject
any attempt to call a window function that way (or an aggregate for
that matter; aggregates fail already, but with relatively obscure
error messages). Perhaps there's also an argument that window
functions should have run-time tests, not just assertions, that
they're called in a valid way.As for procedures, I'm of the opinion that we should just reject those
too, but some other security team members were not happy with that
idea. Conceivably we could attempt to make the case actually work,
but is it worth the trouble? Given the lack of field complaints
about the "invalid transaction termination" failure, it seems unlikely
that it's occurred to anyone to try to call procedures this way.
We'd need special infrastructure to test the case, too, since psql
offers no access to fastpath calls.
+1
A compromise suggestion was to prohibit calling procedures via
fastpath as of HEAD, but leave existing releases as they are,
in case anyone is using a case that happens to work.Thoughts?
My vote would be reject using fastpath for procedures in all relevant branches.
If someday someone cares enough to make it work, it is a new feature for a new
major release.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
On Tue, 2021-03-09 at 14:15 -0500, Tom Lane wrote:
The security team received a report from Theodor-Arsenij
Larionov-Trichkin of PostgresPro that it's possible to crash the
backend with an assertion or null-pointer dereference by trying to
call a window function via the "fast path function call" protocol
message.So the questthemion on the table is what to do about this.
As for procedures, I'm of the opinion that we should just reject those
too, but some other security team members were not happy with that
idea. Conceivably we could attempt to make the case actually work,
but is it worth the trouble? Given the lack of field complaints
about the "invalid transaction termination" failure, it seems unlikely
that it's occurred to anyone to try to call procedures this way.
We'd need special infrastructure to test the case, too, since psql
offers no access to fastpath calls.A compromise suggestion was to prohibit calling procedures via
fastpath as of HEAD, but leave existing releases as they are,
in case anyone is using a case that happens to work.
The "invalid transaction termination" failure alone doesn't
worry or surprise me - transaction handling in procedures only works
under rather narrow conditions anyway (no SELECT on the call stack,
no explicit transaction was started outside the procedure).
If that is the only problem, I'd just document it. The hard work is
of course that there is no other problem with calling procedures that
way. If anybody wants to do that work, and transaction handling is
the only thing that doesn't work with the fastpath API, we can call
it supported and document the exception.
In case of doubt, I would agree with you and forbid it in HEAD
as a corner case with little real-world use.
Yours,
Laurenz Albe
On Wed, Mar 10, 2021 at 10:03:24AM +0100, Laurenz Albe wrote:
On Tue, 2021-03-09 at 14:15 -0500, Tom Lane wrote:
As for procedures, I'm of the opinion that we should just reject those
too, but some other security team members were not happy with that
idea. Conceivably we could attempt to make the case actually work,
but is it worth the trouble? Given the lack of field complaints
about the "invalid transaction termination" failure, it seems unlikely
that it's occurred to anyone to try to call procedures this way.
We'd need special infrastructure to test the case, too, since psql
offers no access to fastpath calls.A compromise suggestion was to prohibit calling procedures via
fastpath as of HEAD, but leave existing releases as they are,
in case anyone is using a case that happens to work.
(That was my suggestion.)
The "invalid transaction termination" failure alone doesn't
worry or surprise me - transaction handling in procedures only works
under rather narrow conditions anyway (no SELECT on the call stack,
no explicit transaction was started outside the procedure).If that is the only problem, I'd just document it. The hard work is
of course that there is no other problem with calling procedures that
way. If anybody wants to do that work, and transaction handling is
the only thing that doesn't work with the fastpath API, we can call
it supported and document the exception.
I'd be fine with that, too.
In case of doubt, I would agree with you and forbid it in HEAD
as a corner case with little real-world use.
The PQfn(some-procedure) feature has no known bugs and no known users, so I
think the decision carries little weight. Removing the feature would look
wise if we later discover some hard-to-fix bug therein. Removal also obeys
the typical pattern that a given parse context accepts either procedures or
functions, not both. Keeping the feature, at least in back branches, would
look wise if it avoids urgent s/PQfn/PQexecParams/ work for some user trying
to update to 13.3.
On Tue, Mar 09, 2021 at 02:33:47PM -0500, Joe Conway wrote:
My vote would be reject using fastpath for procedures in all relevant branches.
If someday someone cares enough to make it work, it is a new feature for a new
major release.
FWIW, my vote would go for issuing an error if attempting to use a
procedure in the fast path for all the branches. The lack of
complaint about the error you are mentioning sounds like a pretty good
argument to fail properly on existing branches, and work on this as a
new feature in the future if there is anybody willing to make a case
for it.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Tue, Mar 09, 2021 at 02:33:47PM -0500, Joe Conway wrote:
My vote would be reject using fastpath for procedures in all relevant branches.
If someday someone cares enough to make it work, it is a new feature for a new
major release.
FWIW, my vote would go for issuing an error if attempting to use a
procedure in the fast path for all the branches. The lack of
complaint about the error you are mentioning sounds like a pretty good
argument to fail properly on existing branches, and work on this as a
new feature in the future if there is anybody willing to make a case
for it.
I let this thread grow cold because I was hoping for some more votes,
but with the quarterly releases approaching, it's time to close out
the issue one way or the other.
By my count, we have three votes for forbidding procedure calls via
fastpath in all branches (me, Joe, Michael), and two for doing
something laxer (Noah, Laurenz). The former is surely the safer
choice, so I'm going to go do that.
regards, tom lane
On Fri, Apr 30, 2021 at 12:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
By my count, we have three votes for forbidding procedure calls via
fastpath in all branches (me, Joe, Michael), and two for doing
something laxer (Noah, Laurenz). The former is surely the safer
choice, so I'm going to go do that.
FWIW, I'm also for the stricter approach.
--
Robert Haas
EDB: http://www.enterprisedb.com