simon-git: putty (master): Simon Tatham

Sun May 5 10:28:53 BST 2019

TL;DR:
  5f90427e Turn off hardware AES for the Coverity build.
  c787e626 fxp_fstat_recv: remove unreachable cleanup code.
  e82ba498 Fix broken error path on open failure in PROXY_FUZZ.
  64fdc85b Fix miscellaneous minor memory leaks.
  0f6ce9bd Remove some spurious null pointer tests.
  cddec53a pscp: robustness fix in backend cleanup.
  f1fe0c7d openssh_new_read: fix misaimed null pointer check.
  2b1e0a5e GSS kex: remove spurious no-op assignment.
  5f35f5b4 testsc.c: fix further memory leaks.

Repository:     https://git.tartarus.org/simon/putty.git
On the web:     https://git.tartarus.org/?p=simon/putty.git
Branch updated: master
Committer:      Simon Tatham <anakin at pobox.com>
Date:           2019-05-05 10:28:53

commit 5f90427e0dc5f0b3860bfd26cfc50199ef43ff5f
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=5f90427e0dc5f0b3860bfd26cfc50199ef43ff5f;hp=fbd2c360fab9f29197aca747493db1752e861e14
Author: Simon Tatham <anakin at pobox.com>
Date:   Sat May 4 15:18:04 2019 +0100

    Turn off hardware AES for the Coverity build.

    It seems to have caused a compile error, apparently due to a mismatch
    between compiler predefines (__SSE2__) and header files (emmintrin.h).
    The easiest thing is to just turn off the hardware version completely,
    so the rest of the code can still be scanned.

 Buildscr.cv | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

commit c787e626516b358d4d48309aa199bdac0f7cb736
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=c787e626516b358d4d48309aa199bdac0f7cb736;hp=5f90427e0dc5f0b3860bfd26cfc50199ef43ff5f
Author: Simon Tatham <anakin at pobox.com>
Date:   Sat May 4 15:35:20 2019 +0100

    fxp_fstat_recv: remove unreachable cleanup code.

    Coverity pointed out a call to sftp_pkt_free(pktin) straight after an
    unconditional return statement, which is obviously silly.

    Fortunately, it was benign: pktin was freed anyway by the function
    being called _by_ the return statement, so the unreachability of this
    free operation prevented a double-free rather than allowing a memory
    leak. So the right fix is to just remove the code, rather than
    arranging for it to be run.

 sftp.c | 2 --
 1 file changed, 2 deletions(-)

commit e82ba498ffb7be40c192f38004698a38f44b0749
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=e82ba498ffb7be40c192f38004698a38f44b0749;hp=c787e626516b358d4d48309aa199bdac0f7cb736
Author: Simon Tatham <anakin at pobox.com>
Date:   Sat May 4 15:47:33 2019 +0100

    Fix broken error path on open failure in PROXY_FUZZ.

    We have to use the file name we just failed to open to format an error
    message _before_ freeing it, not after. If that use-after-free managed
    not to cause a crash, we'd also leak the file descriptor 'outfd'.

    Both spotted by Coverity (which is probably the first thing in years
    to look seriously at any of the code designed for Ben's AFL exercise).

 unix/uxproxy.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

commit 64fdc85b2deddf0595189296d93ecf70ffb67ead
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=64fdc85b2deddf0595189296d93ecf70ffb67ead;hp=e82ba498ffb7be40c192f38004698a38f44b0749
Author: Simon Tatham <anakin at pobox.com>
Date:   Sat May 4 16:19:13 2019 +0100

    Fix miscellaneous minor memory leaks.

    All found by Coverity.

 pageant.c       | 4 ++++
 testsc.c        | 1 +
 unix/uxcons.c   | 4 +++-
 unix/uxpgnt.c   | 1 +
 unix/uxserver.c | 4 +++-
 unix/uxsftp.c   | 4 +++-
 6 files changed, 15 insertions(+), 3 deletions(-)

commit 0f6ce9bd01ac104ab72ad33b05812f374a86eeac
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=0f6ce9bd01ac104ab72ad33b05812f374a86eeac;hp=64fdc85b2deddf0595189296d93ecf70ffb67ead
Author: Simon Tatham <anakin at pobox.com>
Date:   Sun May 5 08:20:24 2019 +0100

    Remove some spurious null pointer tests.

    In load_openssh_new_key, ret->keyblob is never null any more: now that
    it's a strbuf instead of a bare realloc()ed string, it's at worst an
    _empty_ strbuf. Secondly, as Coverity pointed out, the null pointer
    check was too late to do any good in the first place - the previous
    clause of the same if condition would already have dereferenced it!

    In test_mac in the auxiliary testsc program, there's no actual reason
    I would ever have called it with null ssh_mac pointer - it would mean
    'don't test anything'! Looks as if I just copy-pasted the MAC parts of
    the cipher+MAC setup code in test_cipher.

 import.c | 2 +-
 testsc.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

commit cddec53a40f6430816d9572fcc20639ac4ae9254
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=cddec53a40f6430816d9572fcc20639ac4ae9254;hp=0f6ce9bd01ac104ab72ad33b05812f374a86eeac
Author: Simon Tatham <anakin at pobox.com>
Date:   Sun May 5 08:30:33 2019 +0100

    pscp: robustness fix in backend cleanup.

    Coverity points out that in (the misnamed) psftp_main(), I allow for
    the possibility that 'backend' might be null, up until the final
    unconditional backend_free().

    I haven't actually been able to find a way to make backend() come out
    as NULL at all in that part of the code: obvious failure modes like
    trying to scp to a nonexistent host trigger a connection_fatal() which
    terminates the program without going through that code anyway. But I'm
    not confident I tried everything, so I can't be sure there _isn't_ a
    way to get NULL to that location, so let's put in the missing check
    just in case.

 pscp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

commit f1fe0c7d8dc216665ea8a9d86eaa28d5bb99b963
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=f1fe0c7d8dc216665ea8a9d86eaa28d5bb99b963;hp=cddec53a40f6430816d9572fcc20639ac4ae9254
Author: Simon Tatham <anakin at pobox.com>
Date:   Sun May 5 08:32:49 2019 +0100

    openssh_new_read: fix misaimed null pointer check.

    If the input key_wanted field were set to an out-of-range value, the
    parent structure retkey would not become NULL as a whole: instead, its
    field 'key' would never be set to a non-null pointer. So I was testing
    the wrong pointer.

    Fortunately, this couldn't have come up, because we don't actually
    have any support yet for loading the nth key from an OpenSSH new-style
    key file containing more than one. So key_wanted was always set to 0
    by load_openssh_new_key(), which also checked that the file's key
    count was exactly 1 (guarding against the possibility that even 0
    might have been an out-of-range index).

 import.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

commit 2b1e0a5e05befb2176fc4f8f4c2966ba7135bd55
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=2b1e0a5e05befb2176fc4f8f4c2966ba7135bd55;hp=f1fe0c7d8dc216665ea8a9d86eaa28d5bb99b963
Author: Simon Tatham <anakin at pobox.com>
Date:   Sun May 5 09:41:24 2019 +0100

    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(-)

commit 5f35f5b4aca8eb879e374cfd9370ac7b0489cf94
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=5f35f5b4aca8eb879e374cfd9370ac7b0489cf94;hp=2b1e0a5e05befb2176fc4f8f4c2966ba7135bd55
Author: Simon Tatham <anakin at pobox.com>
Date:   Sun May 5 10:13:10 2019 +0100

    testsc.c: fix further memory leaks.

    These were spotted by Leak Sanitiser rather than Coverity: it reported
    them while I was checking the fixes for Coverity-spotted issues.

 testsc.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)