simon-git: putty (main): Simon Tatham

[Commits to Tartarus hosted VCS]

TL;DR:
  52fa23c7 Argon2 hprime: remove pointless bounds check.
  525b767c gtkwin: remove dead code in cut buffer handling.
  c5724c46 unifontsel: add extra double-checks of fontinfo values.
  fc8550c0 Fix a few memory leaks spotted by Coverity.
  165f630a winpgntc: fix mishandling of named-pipe errors.
  edadfa70 Impose an upper bound on incoming SFTP packet length.
  d53b3bcd pageant_get_keylist: fix handling of bad SSH-1 data from agent.
  ed1d64b4 Fix failure handling when loading a PPK file.
  597e4731 winctrls: fix warning about uninitialised variable.
  d33f889a gtkwin: remove a redundant test in delete_window.

Repository:     https://git.tartarus.org/simon/putty.git
On the web:     https://git.tartarus.org/?p=simon/putty.git
Branch updated: main
Committer:      Simon Tatham <anakin at pobox.com>
Date:           2021-04-10 09:26:26

commit 52fa23c7fe7c8b6448b396acc6fc8013b030918a
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=52fa23c7fe7c8b6448b396acc6fc8013b030918a;hp=bb59f27386dae8f97efcac4163708c486c23fba4
Author: Simon Tatham <anakin at pobox.com>
Date:   Fri Apr 9 17:48:28 2021 +0100

    Argon2 hprime: remove pointless bounds check.
    
    Coverity points out that we don't need to check the output buffer
    bound before writing out the first 32 bytes of each full-length
    BLAKE2b invocation, because the only time we're doing a full-length
    one in the first place is if the output buffer bound was at least 64
    bytes.
    
    (More specifically: whenever we're in the while loop, length > 64, so
    setting chunk = 32 and then checking if chunk > length has a totally
    predictable answer.)

 sshargon2.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

commit 525b767c35d678562398eb16ba2b72e513b878ce
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=525b767c35d678562398eb16ba2b72e513b878ce;hp=52fa23c7fe7c8b6448b396acc6fc8013b030918a
Author: Simon Tatham <anakin at pobox.com>
Date:   Fri Apr 9 18:05:09 2021 +0100

    gtkwin: remove dead code in cut buffer handling.
    
    Commit d851df486f066b3 deleted a #if / #else / #endif on the grounds
    that the condition would now always be true, without also deleting the
    code inside the #else. Happily, the then-branch ended with a return,
    so it was a benign mistake - the erroneously left-in else-clause code
    was unreachable. But now Coverity has pointed it out, let's remove it.

 unix/gtkwin.c | 2 --
 1 file changed, 2 deletions(-)

commit c5724c46a0f2bc4db5e29887b84c1dc25e3d86b0
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=c5724c46a0f2bc4db5e29887b84c1dc25e3d86b0;hp=525b767c35d678562398eb16ba2b72e513b878ce
Author: Simon Tatham <anakin at pobox.com>
Date:   Fri Apr 9 18:01:50 2021 +0100

    unifontsel: add extra double-checks of fontinfo values.
    
    Coverity objected to several similar cases in this code in which I'd
    checked a pointer for NULL after already having done things to it. I
    think all the cases are benign, in that (as the comments tersely
    mention) those checks could only fail if the unifontsel system had got
    _really_ confused, in which case probably some other bug would have
    been on the point of manifesting anyway. But Coverity has a point
    anyway: if I'm _going_ to check those values for NULL, let's check
    them consistently.

 unix/gtkfont.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

commit fc8550c07b698dcc5dcfa52965c99954ce2daebc
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=fc8550c07b698dcc5dcfa52965c99954ce2daebc;hp=c5724c46a0f2bc4db5e29887b84c1dc25e3d86b0
Author: Simon Tatham <anakin at pobox.com>
Date:   Fri Apr 9 17:55:31 2021 +0100

    Fix a few memory leaks spotted by Coverity.

 pageant.c            |  1 +
 unix/uxser.c         | 12 ++++++++----
 windows/wincliloop.c |  2 ++
 3 files changed, 11 insertions(+), 4 deletions(-)

commit 165f630ae9d7d73eb5ae25f3a000a47593f339ef
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=165f630ae9d7d73eb5ae25f3a000a47593f339ef;hp=fc8550c07b698dcc5dcfa52965c99954ce2daebc
Author: Simon Tatham <anakin at pobox.com>
Date:   Fri Apr 9 17:57:10 2021 +0100

    winpgntc: fix mishandling of named-pipe errors.
    
    If named_pipe_agent_gotdata was called with an error or EOF status, it
    would call agent_cancel_query(pq), but then accidentally fall through
    to the non-error handler which would dereference pq. I meant to return
    early in that situation, and Coverity spotted that I'd left out the
    early return statement.

 windows/winpgntc.c | 1 +
 1 file changed, 1 insertion(+)

commit edadfa7093854437a1c506815f2e6aac7d3c11b0
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=edadfa7093854437a1c506815f2e6aac7d3c11b0;hp=165f630ae9d7d73eb5ae25f3a000a47593f339ef
Author: Simon Tatham <anakin at pobox.com>
Date:   Fri Apr 9 18:19:44 2021 +0100

    Impose an upper bound on incoming SFTP packet length.
    
    Coverity was unhappy that I'd used the packet length as a loop bound
    without sanitising it first (on the basis that it had decided anything
    coming from GET_32BIT_MSB_FIRST was potentially tainted).
    
    I think this is not a security issue: all that will happen if the
    server sends a huge packet length is that we'll try to allocate space
    for it. On a 64-bit machine we might even _succeed_; on 32-bit, we'll
    fail, and snewn() will abort the program rather than return NULL. So
    *technically* this is a remote-triggered crash. But it can only happen
    in a situation where the same server could have triggered the
    termination of the SFTP connection just as easily by simply closing it
    - the only difference is that the client would die with a different
    fatal error message.
    
    (In particular, it isn't even a DoS against other processes
    participating in a connection-shared SSH session. The upstream will
    pass the SFTP data stream through without parsing it, so it and the
    other downstreams will be unaffected. Only the particular downstream
    operating the SFTP client will run into this problem.)

 sftp.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

commit d53b3bcd22ace9062e0be66c28f868dcc77c2b34
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=d53b3bcd22ace9062e0be66c28f868dcc77c2b34;hp=edadfa7093854437a1c506815f2e6aac7d3c11b0
Author: Simon Tatham <anakin at pobox.com>
Date:   Fri Apr 9 18:21:40 2021 +0100

    pageant_get_keylist: fix handling of bad SSH-1 data from agent.
    
    Coverity points out that if rsa_ssh1_public_blob_len sees data it
    doesn't like, it returns -1 to indicate an error. But the code that
    uses it to parse the SSH1_AGENT_RSA_IDENTITIES_ANSWER payload was
    passing it directly to get_data() as a length field, without checking
    for that case. Now we do check it, and use it to set the existing
    kl->broken flag that indicates that the key list was not correctly
    formatted.

 pageant.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

commit ed1d64b48a999a387acfe798f5af46fed6667833
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=ed1d64b48a999a387acfe798f5af46fed6667833;hp=d53b3bcd22ace9062e0be66c28f868dcc77c2b34
Author: Simon Tatham <anakin at pobox.com>
Date:   Fri Apr 9 18:25:55 2021 +0100

    Fix failure handling when loading a PPK file.
    
    Coverity pointed out that I'd checked if the LoadedFile was NULL, set
    an error message ... and then accidentally fallen through to the
    success handler anyway.

 sshpubk.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

commit 597e4731f9178d9e9141049f832adbe0a68eedf9
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=597e4731f9178d9e9141049f832adbe0a68eedf9;hp=ed1d64b48a999a387acfe798f5af46fed6667833
Author: Simon Tatham <anakin at pobox.com>
Date:   Fri Apr 9 17:52:49 2021 +0100

    winctrls: fix warning about uninitialised variable.
    
    Coverity points out that it's theoretically possible for the main loop
    in radioline_common() to read r.bottom without having gone through the
    conditional setup at the start of the function _or_ a previous
    iteration of the main loop. I think this can only happen in some silly
    case that doesn't actually come up, but on the other hand, it's easy
    to add the necessary robustness.

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

commit d33f889a560f64e22d4729ec04e227ee8ebc72a6
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=d33f889a560f64e22d4729ec04e227ee8ebc72a6;hp=597e4731f9178d9e9141049f832adbe0a68eedf9
Author: Simon Tatham <anakin at pobox.com>
Date:   Fri Apr 9 18:10:45 2021 +0100

    gtkwin: remove a redundant test in delete_window.
    
    We never expect to be passed a NULL GtkFrontend pointer, and even if
    we were, we'd have crashed several lines above this test.
    
    It was benign, of course, but Coverity (which pointed it out) dislikes
    this kind of thing on the basis that it's confusing - you ought to
    either test it for NULL properly, or not at all - and I see its point.

 unix/gtkwin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)