Thread: [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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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