simon-git: putty (master): Simon Tatham

Sat Jul 20 07:59:47 BST 2019

TL;DR:
  01bcae8c stripctrl: be more careful with wcwidth.
  0d0b0a45 Fix server-triggered DoS in keyboard-interactive auth.
  dbd9a07f keyboard-interactive auth: use a uint32 for num_prompts.
  70fd577e Fall back to not sorting large dirs in pscp -ls or psftp 'ls'.
  921613ff GUI PuTTY: fix format-string goof on connect failure.
  03153709 Fix integer underflow in SSH-1 BPP.
  c191ff12 Fix too-short buffer in SSH-1 key exchange.
  81609be0 Check for overflow in the addition in snew_plus().
  453cf993 Fix minor server-triggered DoS in get_fxp_attrs.
  721650bc Fix dodgy strcats in access_random_seed().
  b38d47e9 winpgntc: check the length field in agent responses.
  75cd6c8b Update version number for 0.72 release.

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-07-20 07:59:47

commit 01bcae8c5df3d2780dde0996e55699cd25cd4504
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=01bcae8c5df3d2780dde0996e55699cd25cd4504;hp=04b6db55f20b7057a5503c0a9f348bfba66b44f7
Author: Simon Tatham <anakin at pobox.com>
Date:   Sun May 5 09:04:39 2019 +0100

    stripctrl: be more careful with wcwidth.

    Coverity points out that wcwidth is capable of returning a negative
    number, which suggests that it's a mistake to pass its return value
    unchecked to stripctrl_check_line_limit.

    This shouldn't cause a problem _in principle_, because by the time
    we're making that call, we should already have ruled out the kind of
    dangerous control character that might provoke that return value from
    wcwidth. But in practice, I couldn't absolutely guarantee that
    everyone's idea of what is or is not a control character agrees in
    every detail, so I think Coverity is right to urge caution.

    Fixed by calling wcwidth (or its wrapper term_char_width) up front,
    and ensuring that any character provoking a negative return value is
    included in the 'control characters to sanitise out' branch of the
    preceding logic.

 stripctrl.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

commit 0d0b0a45bcbe4cba107d6359f6f4ca70865047dc
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=0d0b0a45bcbe4cba107d6359f6f4ca70865047dc;hp=01bcae8c5df3d2780dde0996e55699cd25cd4504
Author: Simon Tatham <anakin at pobox.com>
Date:   Sat May 4 15:58:21 2019 +0100

    Fix server-triggered DoS in keyboard-interactive auth.

    If a server sends a very large number as the num-prompts field, we'd
    loop round calling get_string and get_bool, which would run off the
    end of the buffer but still carefully return legal default values (the
    empty string and false), so we'd carry on piling pointless stuff into
    s->cur_prompt and using up lots of memory.

    Coverity pointed this out by warning that an untrusted server-provided
    value was being used as a loop bound. I'm not convinced that's an
    error in every case, but I must admit it pointed out a useful thing
    here!

    The fix is to check the error indicator on the BinarySource after
    reading each prompt from the input packet. Then we'll only keep
    allocating memory as long as there's actually data in the packet.

 ssh2userauth.c | 7 +++++++
 1 file changed, 7 insertions(+)

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

    keyboard-interactive auth: use a uint32 for num_prompts.

    While testing the previous fix, I also noticed that s->num_prompts is
    an ordinary signed int. So if the server sends a _really_ large value,
    it'll be treated as negative.

    That's kind of harmless: our loop wouldn't read any prompts at all
    from the packet, and then it would send back the same nonsense count
    with no responses. But it's inelegant: now, if the server violates the
    protocol in this way, we respond by sending an even wronger packet in
    return.

    Changed the type of num_prompts and the loop variable to uint32_t, so
    now we'll respond by actually trying to read that many prompts, which
    will fail by reaching the new error check. I think that's a more
    sensible way to handle this one.

 ssh2userauth.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

commit 70fd577e40d8b16fd3ba6aaf0c0c332af72be684
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=70fd577e40d8b16fd3ba6aaf0c0c332af72be684;hp=dbd9a07fd089c81b6470b9cc7d33bc9040b0da78
Author: Simon Tatham <anakin at pobox.com>
Date:   Wed May 15 14:57:06 2019 +0100

    Fall back to not sorting large dirs in pscp -ls or psftp 'ls'.

    This mitigates a borderline-DoS in which a malicious SFTP server sends
    a ludicrously large number of file names in response to a SFTP
    opendir/readdir request sequence, causing the client to buffer them
    all and use up all the system's memory simply so that it can produce
    the output in sorted order.

    I call it a 'borderline' DoS because it's very likely that this is the
    same server that you'll also trust to actually send you the _contents_
    of some entire file or directory, in which case, if they want to DoS
    you they can do that anyway at that point and you have no way to tell
    a legit very large file from a bad one. So it's unclear to me that
    anyone would get any real advantage out of 'exploiting' this that they
    couldn't have got anyway by other means.

    That said, it may have practical benefits in the occasional case.
    Imagine a _legit_ gigantic directory (something like a maildir,
    perhaps, and perhaps stored on a server-side filesystem specialising
    in not choking on really huge single directories), together with a
    client workflow that involves listing the whole directory but then
    downloading only one particular file in it.

    For the moment, the threshold size is fixed at 8Mb of total data
    (counting the lengths of the file names as well as just the number of
    files). If that needs to become configurable later, we can always add
    an option.

 Recipe        |   2 +-
 pscp.c        |  45 +++++++++-----------------
 psftp.c       |  54 ++++++++++---------------------
 psftp.h       |  30 +++++++++++++++--
 psftpcommon.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 164 insertions(+), 69 deletions(-)

commit 921613ff088a40b639c34918f383f274af3f04f1
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=921613ff088a40b639c34918f383f274af3f04f1;hp=70fd577e40d8b16fd3ba6aaf0c0c332af72be684
Author: Simon Tatham <anakin at pobox.com>
Date:   Wed May 15 15:04:02 2019 +0100

    GUI PuTTY: fix format-string goof on connect failure.

    Introduced by 61f3e3e29, as part of my periodic efforts to make the
    GTK front end work usefully on OS X: the 'Unable to open connection to
    [host]: [error]' message box is accidentally passed through two layers
    of printf format-string parsing, the second of which has no argument
    list. So if you pass in a host name like '%s' on the command line,
    bad things will happen when that error message is constructed.

    This happened because in that commit I changed a call to
    fatal_message_box() into a call to connection_fatal(), without
    noticing that the latter was printf-style variadic and the former
    wasn't. On the plus side, that means now I can remove the explicit
    dupprintf/free around the error message.

 unix/gtkwin.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

commit 031537092643956d4e1dd7b061fe920086fff7b7
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=031537092643956d4e1dd7b061fe920086fff7b7;hp=921613ff088a40b639c34918f383f274af3f04f1
Author: Simon Tatham <anakin at pobox.com>
Date:   Fri Jun 28 19:23:33 2019 +0100

    Fix integer underflow in SSH-1 BPP.

    If the packet length field was in the range 0 <= x < 5, then it would
    pass the initial range check, but underflow to something in the region
    of 0xFFFFFFFF when the BPP code subtracted 5 from it, leading to an
    overlarge memory allocation, and/or allocation failure, and perhaps
    worse.

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

commit c191ff129cd3415af08143fb40c461de5730655d
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=c191ff129cd3415af08143fb40c461de5730655d;hp=031537092643956d4e1dd7b061fe920086fff7b7
Author: Simon Tatham <anakin at pobox.com>
Date:   Fri Jun 28 19:24:55 2019 +0100

    Fix too-short buffer in SSH-1 key exchange.

    If _both_ the host key and the server key were less than 32 bytes
    long, then less than 32 bytes would be allocated for the buffer
    s->rsabuf, into which the 32-byte session id is then copied.

 ssh1login.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

commit 81609be0524a4f38e1187fd6a0a0c8bcd4e17d4c
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=81609be0524a4f38e1187fd6a0a0c8bcd4e17d4c;hp=c191ff129cd3415af08143fb40c461de5730655d
Author: Simon Tatham <anakin at pobox.com>
Date:   Fri Jun 28 19:38:45 2019 +0100

    Check for overflow in the addition in snew_plus().

    We were carefully checking for overflow inside safemalloc() before
    multiplying together the two factors of the desired allocation size.
    But snew_plus() did an addition _outside_ safemalloc, without the same
    guard. Now that addition also happens inside safemalloc.

 memory.c   | 33 +++++++++++++++++++++------------
 puttymem.h | 10 +++++-----
 2 files changed, 26 insertions(+), 17 deletions(-)

commit 453cf99357d1da9ec9b8270ddbf7b6167468efbc
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=453cf99357d1da9ec9b8270ddbf7b6167468efbc;hp=81609be0524a4f38e1187fd6a0a0c8bcd4e17d4c
Author: Jacob Nevins <jacobn at chiark.greenend.org.uk>
Date:   Sun Jun 23 11:38:55 2019 +0100

    Fix minor server-triggered DoS in get_fxp_attrs.

    If a server sent a very large number as extended_count, and didn't
    actually send any extended attributes, we could loop around and around
    calling get_string, which would carefully not overrun any buffer or
    leak any memory, and we weren't paying attention to extended
    attributes anyway, but it would still pointlessly consume CPU.

    Now we bail as soon as the BinarySource flags an error. Current
    callers will then spot the error and complain that the packet was
    malformed.

 sftpcommon.c | 6 ++++++
 1 file changed, 6 insertions(+)

commit 721650bcb149664ef688e0cacfddd67918de2f04
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=721650bcb149664ef688e0cacfddd67918de2f04;hp=453cf99357d1da9ec9b8270ddbf7b6167468efbc
Author: Simon Tatham <anakin at pobox.com>
Date:   Sun Jun 30 15:21:08 2019 +0100

    Fix dodgy strcats in access_random_seed().

    Looking over this function today, I spotted several questionable uses
    of strcat to concatenate "\PUTTY.RND" to the end of a pathname,
    without having checked whether the pathname had filled up the static
    fixed-size buffer already.

    I don't think this is exploitable (because you'd have to be in control
    of the local account already to control any of the data sources used
    to fill those buffers). But it's horrible anyway, of course. Now all
    of those are replaced with sensible dupcats.

    (This patch re-indents a lot of the function, to give variables
    tighter scopes. So the diff is best viewed with whitespace ignored.)

 windows/winstore.c | 95 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 55 insertions(+), 40 deletions(-)

commit b38d47e94cf6848dddeaa7aa50301456130715a9
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=b38d47e94cf6848dddeaa7aa50301456130715a9;hp=721650bcb149664ef688e0cacfddd67918de2f04
Author: Simon Tatham <anakin at pobox.com>
Date:   Sat Jul 6 19:11:56 2019 +0100

    winpgntc: check the length field in agent responses.

    If the agent sent a response whose length field describes an interval
    of memory larger than the file-mapping object the message is supposed
    to be stored in, we shouldn't return that message to the client as if
    nothing is wrong. Treat that the same as a failure to receive any
    response at all.

 windows/winpgntc.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

commit 75cd6c8b2703137e574223d90d2f3ead9ca34acc
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=75cd6c8b2703137e574223d90d2f3ead9ca34acc;hp=b38d47e94cf6848dddeaa7aa50301456130715a9
Author: Simon Tatham <anakin at pobox.com>
Date:   Sun Jul 14 09:51:06 2019 +0100

    Update version number for 0.72 release.

 Buildscr      | 2 +-
 LATEST.VER    | 2 +-
 doc/plink.but | 2 +-
 doc/pscp.but  | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)