From 2b1e0a5e05befb2176fc4f8f4c2966ba7135bd55 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 5 May 2019 09:41:24 +0100 Subject: [PATCH] GSS kex: remove spurious no-op assignment. Coverity points out that under a carefully checked compound condition, we assign s->gss_cred_expiry to itself. :-) Before commit 2ca0070f8 split the SSH code into separate modules, that assignment read 'ssh->gss_cred_expiry = s->gss_cred_expiry', and the point was that the value had to be copied out of the private state of the transport-layer coroutine into the general state of the SSH protocol as a whole so that ssh2_timer() (or rather, ssh2_gss_update, called from ssh2_timer) could check it. But now that timer function is _also_ local to the transport layer, and shares the transport coroutine's state. So on the one hand, we could just remove that assignment, folding the two variables into one; on the other hand, we could reinstate the two variables and the explicit 'commit' action that copies one into the other. The question is, which? Any _successful_ key exchange must have gone through that commit step of the kex that copied the one into the other. And an unsuccessful key exchange always aborts the whole SSH connection - there's nothing in the SSH transport protocol that allows recovering from a failed rekey by carrying on with the previous session keys. So if I made two variables, then between key exchanges, they would always have the same value anyway. So the only effect of the separation between those variables is _during_ a GSS key exchange: should the version of gss_cred_expiry checked by the timer function be the new one, or the old one? This can only make a difference if a timer goes off _during_ a rekey, and happens to notice that the GSS credentials have expired. And actually I think it's mildly _better_ if that checks the new expiry time rather than the old one - otherwise, the timer routine would set the flag indicating a need for another rekey, when actually the currently running one was going to get the job done anyway. So, in summary, I think conflating those two variables is actually an improvement to the code. I did it by accident, but if I'd realised, I would have done the same thing on purpose! Hence, I've just removed the pointless self-assignment, with no functional change. --- ssh2kex-client.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/ssh2kex-client.c b/ssh2kex-client.c index 1192a26a..64ec0331 100644 --- a/ssh2kex-client.c +++ b/ssh2kex-client.c @@ -412,9 +412,6 @@ void ssh2kex_coroutine(struct ssh2_transport_state *s, bool *aborted) data = get_string(pktin); s->mic.value = (char *)data.ptr; s->mic.length = data.len; - /* Save expiration time of cred when delegating */ - if (s->gss_delegate && s->gss_cred_expiry != GSS_NO_EXPIRATION) - s->gss_cred_expiry = s->gss_cred_expiry; /* If there's a final token we loop to consume it */ if (get_bool(pktin)) { data = get_string(pktin);