Обсуждение: [PATCH] Align GSS and TLS error handling in PQconnectPoll()

Hi all,

During the gssencmode CVE discussion, we noticed that PQconnectPoll()
handles the error cases for TLS and GSS transport encryption slightly
differently. After TLS fails, the connection handle is dead and future
calls to PQconnectPoll() return immediately. But after GSS encryption
fails, the connection handle can still be used to reenter the GSS
handling code.

This doesn't appear to have any security implications today -- and a
client has to actively try to reuse a handle that's already failed --
but it seems undesirable. Michael (cc'd) came up with a patch, which I
have attached here and will register in the CF.

Thanks,
--Jacob

Patch looks good to me. Definitely an improvement over the status quo.

Looking at the TLS error handling though I see these two lines:

&& conn->allow_ssl_try    /* redundant? */
&& !conn->wait_ssl_try) /* redundant? */

Are they actually redundant like the comment suggests? If so, we
should probably remove them (in another patch). If not (or if we don't
know), should we have these same checks for GSS?

On Thu, Feb 16, 2023 at 3:31 AM Jelte Fennema <me@jeltef.nl> wrote:
>
> Patch looks good to me. Definitely an improvement over the status quo.

Thanks for the review!

> Looking at the TLS error handling though I see these two lines:
>
> && conn->allow_ssl_try    /* redundant? */
> && !conn->wait_ssl_try) /* redundant? */
>
> Are they actually redundant like the comment suggests? If so, we
> should probably remove them (in another patch). If not (or if we don't
> know), should we have these same checks for GSS?

It's a fair point. GSS doesn't have an "allow" encryption mode, so
they can't be the exact same checks. And we're already not checking
the probably-redundant information, so I'd vote against speculatively
adding it back. (try_gss is already based on gssencmode, which we're
using here. So I think rechecking try_gss would only help if we wanted
to clear it manually while in the middle of processing a GSS exchange.
From a quick inspection, I don't think that's happening today -- and
I'm not really sure that it could be useful in the future, because I'd
think prefer-mode is supposed to guarantee a retry on failure.)

I suspect this is a much deeper rabbit hole; I think it's work that
needs to be done, but I can't sign myself up for it at the moment. The
complexity of this function is off the charts (for instance, why do we
recheck conn->try_gss above, if the only apparent way to get to
CONNECTION_GSS_STARTUP is by having try_gss = true to begin with? is
there a goto/retry path I'm missing?). I think it either needs heavy
assistance from a committer who already has intimate knowledge of this
state machine and all of its possible branches, or from a static
analysis tool that can help with a step-by-step simplification.

Thanks,
--Jacob

On Thu, Feb 16, 2023 at 09:59:54AM -0800, Jacob Champion wrote:
> On Thu, Feb 16, 2023 at 3:31 AM Jelte Fennema <me@jeltef.nl> wrote:
>> Patch looks good to me. Definitely an improvement over the status quo.
>
> Thanks for the review!

I was looking at that a second time, and with fresh eyes I can see
that we would miss to mark conn->status with CONNECTION_BAD when using
gssencmode=require when the polling fails in pqsecure_open_gss(),
which is just wrong IMO.  This code has been introduced by b0b39f7,
that has added support for GSS encryption.  I am adding Stephen Frost
in CC to see if he has any comments about all this part of the logic
with gssencmode.

> I suspect this is a much deeper rabbit hole; I think it's work that
> needs to be done, but I can't sign myself up for it at the moment. The
> complexity of this function is off the charts (for instance, why do we
> recheck conn->try_gss above, if the only apparent way to get to
> CONNECTION_GSS_STARTUP is by having try_gss = true to begin with? is
> there a goto/retry path I'm missing?). I think it either needs heavy
> assistance from a committer who already has intimate knowledge of this
> state machine and all of its possible branches, or from a static
> analysis tool that can help with a step-by-step simplification.

The first one of these is from 57c0879, the second from bcd713a, which
I assume is a copy-paste of the first one.  I agree that
PQconnectPoll() has grown beyond the point of making it easy to
maintain.  I am wondering which approach we could take when it comes
to simplify something like that.  Attempting to reduce the number of
flags stored in PGconn would be one.  The second may be to split the
internal logic into more functions, for each state we are going
through?  The first may lead to an even cleaner logic for the second
point.
--
Michael

On Thu, Feb 16, 2023 at 10:59 PM Michael Paquier <michael@paquier.xyz> wrote:
> I am adding Stephen Frost
> in CC to see if he has any comments about all this part of the logic
> with gssencmode.

Sounds good.

> I agree that
> PQconnectPoll() has grown beyond the point of making it easy to
> maintain.  I am wondering which approach we could take when it comes
> to simplify something like that.  Attempting to reduce the number of
> flags stored in PGconn would be one.  The second may be to split the
> internal logic into more functions, for each state we are going
> through?  The first may lead to an even cleaner logic for the second
> point.

Yeah, a mixture of both might be helpful -- the first to reduce the
inputs to the state machine; the second to reduce interdependencies
between cases, the distance of the potential goto jumps, and the
number of state machine outputs. (When cases are heavily dependent on
each other, probably best to handle them in the same function?)
Luckily it looks like the current machine is usually linear.

Thanks,
--Jacob

On Fri, Feb 17, 2023 at 09:01:43AM -0800, Jacob Champion wrote:
> On Thu, Feb 16, 2023 at 10:59 PM Michael Paquier <michael@paquier.xyz> wrote:
>> I am adding Stephen Frost
>> in CC to see if he has any comments about all this part of the logic
>> with gssencmode.
>
> Sounds good.

Hearing nothing on this part, perhaps we should just move on and
adjust the behavior on HEAD?  Thats seems like one step in the good
direction.  If this brews right, we could always discuss a backpatch
at some point, if necessary.

Thoughts from others?
--
Michael

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Fri, Feb 17, 2023 at 09:01:43AM -0800, Jacob Champion wrote:
> > On Thu, Feb 16, 2023 at 10:59 PM Michael Paquier <michael@paquier.xyz> wrote:
> >> I am adding Stephen Frost
> >> in CC to see if he has any comments about all this part of the logic
> >> with gssencmode.
> >
> > Sounds good.
>
> Hearing nothing on this part, perhaps we should just move on and
> adjust the behavior on HEAD?  Thats seems like one step in the good
> direction.  If this brews right, we could always discuss a backpatch
> at some point, if necessary.
>
> Thoughts from others?

I agree with matching how SSL is handled here and in a review of the
patch proposed didn't see any issues with it.  Seems like it's probably
something that should also be back-patched and it doesn't look terribly
risky to do so, is there a specific risk that you see?

Thanks,

Stephen

On Thu, Mar 09, 2023 at 09:51:09AM -0500, Stephen Frost wrote:
> I agree with matching how SSL is handled here and in a review of the
> patch proposed didn't see any issues with it.  Seems like it's probably
> something that should also be back-patched and it doesn't look terribly
> risky to do so, is there a specific risk that you see?

Nothing specific per se, just my usual
be-careful-with-slight-behavior-changes-with-libpq-parameters.
Perhaps you are right and there is no actual reason to worry here.
--
Michael

On Fri, Mar 10, 2023 at 10:42:08AM +0900, Michael Paquier wrote:
> Perhaps you are right and there is no actual reason to worry here.

I have been thinking about that for the last few days, and yes a
backpatch should be OK, so done now down to 12.
--
Michael