Horton

Thread

Commits

  1. Doc: update libpq's documentation for PQfn().

  2. Disallow calling anything but plain functions via the fastpath API.

  1. Procedures versus the "fastpath" API

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-09T19:15:09Z

    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

  2. Re: Procedures versus the "fastpath" API

    Joe Conway <mail@joeconway.com> — 2021-03-09T19:33:47Z

    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

  3. Re: Procedures versus the "fastpath" API

    Laurenz Albe <laurenz.albe@cybertec.at> — 2021-03-10T09:03:24Z

    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

  4. Re: Procedures versus the "fastpath" API

    Noah Misch <noah@leadboat.com> — 2021-03-12T01:52:28Z

    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.

  5. Re: Procedures versus the "fastpath" API

    Michael Paquier <michael@paquier.xyz> — 2021-03-15T05:07:46Z

    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

  6. Re: Procedures versus the "fastpath" API

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-30T16:57:29Z

    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

  7. Re: Procedures versus the "fastpath" API

    Robert Haas <robertmhaas@gmail.com> — 2021-04-30T18:04:42Z

    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