Just happened to jump out at me in an eyeball inspection just now. I
carefully moved all the protocol byte-value constants into a header
file with mnemonic names, but I still hard-coded SOCKS4_REPLY_VERSION
in the text of one diagnostic, and I got the wrong one of
SOCKS5_REQUEST_VERSION and SOCKS5_REPLY_VERSION at one point in the
code. Both benign (the right value was there, juste called by the
wrong name).
Also fixed some missing whitespace, in passing. (Probably the line it
was missing from had once been squashed up closer to the right margin.)
(cherry picked from commit 1cd0f1787f)
Recently I encountered a CLI tool that took tens of seconds to run,
and produced no _visible_ output, but wrote ESC[0m to the terminal a
few times during its operation. (Probably by mistake. In other modes
it does print colourful messages, so I expect a 'reset colour' call
was accidentally outside the 'if' statement containing the rest of the
diagnostic it followed. Or something along those lines.)
I noticed this because every ESC[0m reset my pterm scrollback to the
bottom, which wasn't very helpful, and was unintentional on pterm's
part (as _well_ as on the part of the tool). But I can fix pterm!
At first glance the code _looked_ sensible: terminal.c contains calls
to seen_disp_event(term) whenever terminal output does something that
requires a redraw of the terminal window. Those are also the updates
that should count as 'reset scrollback on display activity'. And
ESC[0m, along with the rest of the SGR handler, correctly contained no
such call. So how did a display update happen at all?
The code was confusingly tangled up with the code that responds to
terminal activity by resetting the phase of the blinking cursor (if
any). term_reset_cblink() was calling seen_disp_event() (when surely
it should be the other way round!), and also, term_reset_cblink() was
called whenever _any_ terminal output data arrived. That combination
meant that any byte output to the terminal at all turned out to count
as display activity, whether or not it changed the screen contents.
Additionally, the other scrollback-reset flag, 'reset scrollback on
keypress', was handled by calling seen_disp_event() from the keyboard
handler. But display events and keyboard events are supposed to be
_independent_ potential causes of scrollback resets - it doesn't make
any sense to handle one by treating it as the other!
So I've reorganised the code completely:
- the seen_disp_event *flag* is now gone. Instead, the
seen_disp_event function tests the scroll_on_disp flag, and if set,
resets the scroll position immediately and sets the general
'scrollbar needs updating' flag.
- keyboard input is handled by doing exactly the same thing except
testing the scroll_on_key flag, so the two systems are properly
independent. That code calls term_schedule_update so that the
terminal will be redrawn as a result of the scroll, but doesn't
also call seen_disp_event() for the rest of the full treatment.
- the term_update code that does the scrollbar update is much
simpler, since now it only needs to test that one flag.
- I also had to set that flag explicitly in scroll() so that the
scrollbar would still be updated as a result of the scrollback size
changing. I think that must have been happening entirely by
accident before.
- term_reset_cblink is subsumed into seen_disp_event, so that only
_substantive_ display updates cause the cursor blink phase to reset
to the start of the solid period.
Result: if programs output no-op sequences like ESC[0m, or if you
press keys that don't echo, then the cursor will carry on blinking
normally, and (if you don't also have scroll_on_key set) the
scrollback won't be reset. And the code is slightly shorter than it
was before, and hopefully more sensible too.
(However, other classes of no-op activity _will_ still cause a cursor
blink phase change and a scrollback reset, such as sending a
cursor-positioning sequence that puts the cursor in the same place it
was already - even something as simple as ^M when already at the start
of the line. It might be nice to fix that, but it's much more
difficult: you'd have to either put a complicated and error-prone test
at every seen_disp_event call site, or else expensively diff the
entire visible terminal state against how it was before. And to avoid
a nondeterministic dependency on the terminal update cooldown, that
diff would have to be done at the granularity of individual control
sequences rather than a bounded number of times a second. I'd rather
not!)
(cherry picked from commit bdbd5f429c)
(cherry-picker's note: I also had to remove the initialisation of
the removed field term->seen_disp_event from term_init(), which didn't
need doing on main because commit 74aa3cb7fb had already replaced
all the boring parts of term_init with a big memset)
A user just reported that 0.79 doesn't build out of the box on Ubuntu
14.04 (trusty), because although its gcc (4.8.4) does _support_ C99,
it doesn't enable it without a non-default -std option. The user was
able to work around the problem by defining CMAKE_C_FLAGS=-std=gnu99,
but it would have been nicer if we'd done that automatically. Setting
CMAKE_C_STANDARD causes cmake to do so.
(This isn't a regression of 0.79 over 0.78 as far as I know; the user
in question said they had last built 0.76.)
I was surprised to find Ubuntu 14.04 still in use at all, but a quick
web search revealed that its support has been extended until next
year, so fair enough, I suppose. It's also running a cmake way older
than we support, but apparently that can be worked around via
Kitware's binary tarball downloads (which do still run on 14.04).
This is a bit unsatisfactory: I'd prefer to ask for C standards
support of _at least_ C99 level, and C11 if possible. Then I could
test for the presence of C11 features via check_c_source_compiles, and
use them opportunistically (e.g. in macro definitions). But as far as
I can see, cmake has no built-in support for asking for a standards
level of 'as new as you can get, but no older than 99'. Oh well.
(In any case, the thing I'd find most useful from C11 is _Generic, and
since that's in implementation namespace, compilers can - and do -
support it in C99 mode anyway. So it's probably fine, at least for now.)
(cherry picked from commit bd27962cd9)
Experimenting with different compile flags pointed out two instances
of typedefing the same name twice (though benignly, with the same
definition as well). PsocksDataSink was typedefed a couple of lines
above its struct definition and then again _with_ its struct
definition; cliloop_continue_t was typedefed in unix/platform.h and
didn't need defining again in unix/psocks.c.
(cherry picked from commit 3d34007889)
That's two releases running I've got most of the way through the
mechanical upload processes and suddenly realised I still have a piece
of creative writing to do. A small one, but even so, there's no reason
it couldn't have been prepared a week in advance like the rest of the
announcements and changelogs. The only reason I didn't is that the
checklist didn't remind me to. Now it does.
(cherry picked from commit da550c3158)
A user reports, _just_ in time to make the 0.79 release, that changes
in the Windows port of OpenSSH from 8.9.x have made it unhappy with
the use of \ as a path separator in the 'IdentityAgent' config
directive. Switch to /, which is also accepted by earlier versions, so
it should work everywhere.
Spotted by Coverity. If PuTTY is functioning as a sharing upstream,
and a new downstream mishandles the version string exchange in any way
that provokes an error message from share_receive() (such as failing
to start the greeting with the expected protocol-name string), we were
calling share_disconnect() and then going to crFinish. But
share_disconnect is capable of actually freeing the entire
ssh_sharing_connstate which contains the coroutine state - in which
case, crFinish's zeroing out of crLine is a use-after-free.
The usual pattern elsewhere in this code is to exit a coroutine with
an ordinary 'return' when you've destroyed its state structure. Switch
to doing that here.
An irate user complained today that they wished we'd documented
firewalls as a possible cause of WSAECONNREFUSED, because it took them
ages to think of checking that. Fair enough.
Someone just asked us a question which suggests they might have thought
they need to supply both files in the 'Public-key authentication' box in
the config dialog, to use public-key authentication at all. I can see
why someone might think that, anyway.
For serial connections, &H generally expanded to the empty string.
This seems more useful.
(It so happens that &H _could_ expand to the serial line if it came from
the command-line, but that's accidental.)
The pathname of Pageant's named pipe includes the name of the user
running it. And Windows usernames are allowed to have spaces in! So
the pipe pathname may also have a space, in which case Windows OpenSSH
will interpret the spacey pathname as an invalid first half followed
by a trailing garbage word.
A user reports that quoting the filename makes this work. Since double
quotes are an illegal Windows filename character, I think it should
therefore do no harm to quote it unconditionally, which is the easiest
fix.
Index the older format as 'PEM-style', since PEM is how it's referred to
in OpenSSH's own docs; and justify why you might want to use the newer
format.
Testing the script described in the previous commit message, Leak
Sanitiser pointed out that we didn't free the LogContext from the
first connection, and overwrote the pointer variable with the one from
the second.
A user points out that it always returned failure, even if it
succeeded. As a result, a 'psftp -b' script of the form
open this.host
do stuff
close
open that.host
do stuff
close
would terminate at the first 'close', believing it to have failed, and
PSFTP would exit with a failure status.
(Not only that, but there would be no error message indicating _why_
PSFTP had closed, because when a command returns failure it's expected
to have printed an error message already.)
When you send a "publickey" USERAUTH_REQUEST containing a certified
RSA key, and you want to use a SHA-2 based RSA algorithm, modern
OpenSSH expects you to send the algorithm string as
rsa-sha2-NNN-cert-v01@openssh.com. But 7.7 and earlier didn't
recognise those names, and expected the algorithm string in the
userauth request packet to be ssh-rsa-cert-v01@... and would then
follow it with an rsa-sha2-NNN signature.
OpenSSH itself has a bug workaround for its own older versions. Follow
suit.
If you specify a detached certificate, it's supposed to completely
replace any certificate that might have been embedded in the input PPK
file. But one thing wasn't working: if the key was RSA, and the server
was using new SHA-2 based RSA, and the user provided both an embedded
_and_ detached certificate, then the initial call to
ssh2_userauth_signflags would upgrade the ssh-rsa-cert-... key type to
rsa-sha2-NNN-cert-..., which ssh2_userauth_add_alg_and_publickey's
call to ssh_keyalg_related_alg would not recognise as any of the base
RSA types while trying to decide on the key algorithm string _after_
replacing the certificate.
Fixed by reverting to the the uncertified base algorithm before
calling ssh_keyalg_related_alg.
Introduced in the previous commit. The new ssh_ppl_final_output method
shouldn't be called in any of the error cleanup functions if
ssh->base_layer is NULL, which it can be if we haven't got far enough
through the connection to set up any packet protocol layers at
all. (For example, ECONNREFUSED would do it.)
This should fix the bug mentioned three commits ago: if an SSH server
sends a userauth banner and then immediately slams the connection
shut (with or without SSH_MSG_DISCONNECT), the banner message should
now be reliably printed to the user, which is important if that's
where the server put its explanation for the disconnection (e.g. "Your
account has expired").
(cherry picked from commit e8becb45b5)
No functional change: I've just pulled out into separate subroutines
the piece of code that process a USERAUTH_BANNER message and append
it to our banner bufchain, and the piece that prints the contents of
the bufchain as user output. This will enable them to be called from
additional places easily.
(cherry picked from commit 99bbbd8d32)
This is called just before closing the connection, and gives every PPL
one last chance to output anything to the user that it might have
buffered.
No functional change: all implementations so far are trivial, except
that the transport layer passes the call on to its higher
layer (because otherwise nothing would do so).
(cherry picked from commit d6e6919f69)
During the transition to cmake, commit b00e5fb129 renamed
unix/unix.h to unix/platform.h, and for visual consistency, also
renamed the guard macro PUTTY_UNIX_H to PUTTY_UNIX_PLATFORM_H.
But I had failed to notice that that guard macro is re-tested in
settings.c, as a convenient method of knowing whether we're building
the Windows or Unix version of PuTTY in order to store some settings
differently. So all those '#ifdef PUTTY_UNIX_H' statements silently
became equivalent to '#if 0', because PUTTY_UNIX_H is _never_ defined
any more.
Specifically, these ifdefs were causing the time intervals relating to
bell overloads to be off by a factor of 1000, because for some reason
I can't remember, we were storing those intervals using a different
time unit on Unix and Windows. In my own configuration, for example,
~/.putty/sessions/Default%20Settings contains "BellOverloadT=2000000"
and "BellOverloadS=5000000", which originally meant that too many
bells within 2 seconds would silence the bell until there were 5
seconds of silence - but current PuTTY shows it in the configurer as
2000 and 5000 seconds!
This commit belatedly rewrites the ifdefs in settings.c, so that saved
sessions from before 0.77 will now be interpreted correctly. Saved
sessions from after that may need a rewrite. (But you have to have one
or the other.)
(cherry picked from commit 62b69a4f16)
For the same reason that diffie-hellman-group18 goes below group16:
it's useful to _have_ it there, in case a server demands it, but under
normal circumstances it seems like overkill and a waste of CPU.
SHA-256 is not only intrinsically faster, it's also more likely to be
hardware-accelerated, so PuTTY's preference is to use that if possible
and SHA-512 only if necessary.
(cherry picked from commit 289d123fb8)
I saw a post on comp.security.ssh just now where someone had
encountered an SSH server that would _only_ speak that, which makes it
worth bothering to implement.
The totally obvious implementation works, and passes the test cases
from RFC 6234.
(cherry picked from commit b77e985513)
ssh->base_layer is NULL when the connection is still in its early
stages, before greetings are exchanged. If the user invokes the Change
Settings dialog in this situation, ssh_reconfig would call
ssh_ppl_reconfigure() on ssh->base_layer without checking if it was
NULL first.
(cherry picked from commit d67c13eeb8)
In commit d07d7d66f6 I rewrote the code that constructs RTF paste
data so that it uses a strbuf, in place of the previous ad-hoc code
that counted up the lengths of pieces of RTF in advance in order to
realloc the buffer.
But apparently I left in an entire loop whose job was to count up one
of those lengths, failing to notice that it's now completely pointless
because its output value is never needed!
Happily a clang upgrade has just improved the 'variable set but not
used' warning to the point where it can spot that. I expect previously
the variable still counted as 'used' because each increment of it used
the previous value.
(cherry picked from commit 6a27ae772c)
When the standalone version of a binary, with its help file included
as a resource, extracts that resource to write it to a disk, it could
have accidentally skipped a byte in the middle if the WriteFile call
in this loop had not managed to write the whole file in one go.
(cherry picked from commit 775d969ca8)
While writing the previous patch, I realise that walking along a
decrypted string and stopping to complain about the first mismatch you
find is an anti-pattern. If we're going to deliberately give the same
error message for various mismatches, so as not to give away which
part failed first, then we should also avoid giving away the same
information via a timing leak!
I don't think this is serious enough to warrant the full-on advisory
protocol, because XDM-AUTHORIZATION-1 is rarely used these days and
also DES-based, so there are bigger problems with it. (Plus, why on
earth is it based on encryption anyway, not a MAC?) But since I
spotted it in passing, might as well fix it.
(cherry picked from commit 8e7e3c5944)
Now the return value is a dynamically allocated string instead of a
static one, which means that the error message can include details
taken from the specific failing connection. In particular, if someone
requests an X11 authorisation protocol we don't support, we can print
its name as part of the message, which may help users debug the
problem.
One particularly important special case of this is that if the client
connection presents _no_ authorisation - which is surely by far the
most likely thing to happen by accident, e.g. if the auth file has
gone missing, or the hostname doesn't match for some reason - then we
now give a specific message "No authorisation provided", which I think
is considerably more helpful than just lumping that very common case
in with "Unsupported authorisation protocol". Even changing the latter
to "Unsupported authorisation protocol ''" is still not very sensible.
The problem in that case is not that the user has tried an exotic auth
protocol we've never heard of - it's that they've forgotten, or
failed, to provide one at all.
The error message for "XDM-AUTHORIZATION-1 data was wrong length" is
the other modified one: it now says what the wrong length _was_.
However, all other failures of X-A-1 are still kept deliberately
vague, because saying which part of the decrypted string didn't match
is an obvious information leak.
(cherry picked from commit dff4bd4d14)
This reverts commit 0615767224
("Windows installer: remove explicit InstallScope setting"), albeit
with different comments.
The original change worked around a Windows security vulnerability
(CVE-2023-21800), but also resulted in a rather broken installer.
(cherry picked from commit cedeb75d59)
I checked exhaustively today and found that the only characters (even
in Unicode) that Windows's default argv splitter will recognise as
word separators are the space and tab characters. So I think it's a
mistake to use <ctype.h> functions to identify word separators; we
should use that fixed character pair, and then we know we're getting
the right ones only.
(cherry picked from commit 9adfa79767)
I thought I'd found all of these before, but perhaps a few managed to
slip in since I last looked. The character argument to the <ctype.h>
functions must have the value of an unsigned char or EOF; passing an
ordinary char (unless you know char is unsigned on every platform the
code will ever go near) risks mistaking '\xFF' for EOF, and causing
outright undefined behaviour on byte values in the range 80-FE. Never
do it.
(cherry picked from commit a76109c586)
Another bug turned up by writing tests. The code that spots that the
character won't fit, and wraps it to the next line setting
LATTR_WRAPPED2, was not checking that wrap mode was _enabled_ before
doing that. So if you printed a DW character in the rightmost column
while the terminal was in non-auto-wrap mode, you'd get an unwanted
wrap.
Other terminals disagree on what to do here. xterm leaves the cursor
in the same place and doesn't print any character at all.
gnome-terminal, on the other hand, backspaces by a character so that
it _can_ print the requested DW character, in the rightmost _two_
columns.
I think I don't much like either of those, so instead I'm using the
same fallback we use for displaying a DW character when the whole
terminal is only one column wide: if there is physically no room to
print the requested character, turn it into U+FFFD REPLACEMENT
CHARACTER.
(cherry picked from commit ed5bf9b3b8)
This is the first bug found as a direct result of writing that
terminal test program - I added some tests for things I expected to
work already, and some of them didn't, proving immediately that it was
a good idea!
If the terminal is one column wide, and you've printed a
character (hence, set the wrapnext flag), what should backspace do?
Surely it should behave like any other backspace with wrapnext set,
i.e. clear the wrapnext flag, returning the cursor's _logical_
position to the location of the most recently printed character. But
in fact it was anti-wrapping to the previous line, because I'd got the
cases in the wrong order in the if-else chain that forms the backspace
handler. So the handler for 'we're in column 0, wrapping time' was
coming before 'wrapnext is set, just clear it'.
Now wrapnext is checked _first_, before checking anything at all. Any
time we can just clear that, we should.
(cherry picked from commit 069f7c8b21)
Suppose an application tries to print a double-width character
starting in the rightmost column of the screen, so that we apply our
emergency fix of wrapping to the next line immediately and printing
the character in the first two columns. Suppose they then backspace
twice, taking the cursor to the RHS and then the LHS of that
character. What should happen if they backspace a third time?
Our previous behaviour was to completely ignore the unusual situation,
and do the same thing we'd do in any other backspace from column 0:
anti-wrap the cursor to the last column of the previous line, leaving
it in the empty character cell that was skipped when the DW char
couldn't be printed in it.
But I think this isn't the best response, because it breaks the
invariant that printing N columns' worth of graphic characters and
then backspacing N times should leave the cursor on the first of those
characters. If I print "a가" (for example) and then backspace three
times, I want the cursor on the a, _even_ if weird line wrapping
behaviour happened somewhere in that sequence.
(Rationale: this helps naïve terminal applications which don't even
know what the terminal width is, and aren't tracking their absolute x
position. In particular, the simplistic line-based input systems that
appear in OS kernels and our own lineedit.c will want to emit a fixed
number of backspace-space-backspace sequences to delete characters
previously entered on to the line by the user. They still need to
check the wcwidth of the characters they're emitting, so that they can
BSB twice for a DW character or 0 times for a combining one, but it
would be *hugely* more awkward for them to ask the terminal where the
cursor is so that they can take account of difficult line wraps!)
We already have the ability to _recognise_ this situation: on a line
that was wrapped in this unusual way, we set the LATTR_WRAPPED2 line
attribute flag, to prevent the empty rightmost column from injecting
an unwanted space into copy-pastes from the terminal. Now we also use
the same flag to cause the backspace control character to do something
interesting.
This was the fix that inspired me to start writing test_terminal,
because I knew it was touching a delicate area. However, in the course
of writing this fix and its tests, I encountered two (!) further bugs,
which I'll fix in followup commits!
(cherry picked from commit 9ba742ad9f)
For years I've been following the principle that before I'll add
auto-detection of an SSH server bug, I want the server maintainer to
have fixed the bug, so that the list of affected version numbers
triggering the workaround is complete, and to provide an incentive for
implementations to gradually converge on rightness.
*Finally*, I've got round to documenting that policy in public, for
the Feedback page!
(cherry picked from commit b5645f79dd)
A user just reported that it hasn't been there since 0.76. This turns
out to be because I put the wrong pathname on the 'zip' commands in
Buildscr (miscounted the number of ../ segments).
I would have noticed immediately, if Info-Zip had failed with an error
when it found I'd given it a nonexistent filename to add to the zip
file. But in fact it just prints a warning and proceeds to add all the
other files I specified. It looks as if it will only return a nonzero
exit status if _all_ the filenames you specified were nonexistent.
Therefore, I've rewritten the zip-creation commands so that they run
zip once per file. That way if any file is unreadable we _will_ get a
build error.
(Also, while I'm here, I took the opportunity to get rid of that ugly
ls|grep.)
(cherry picked from commit 9d308b39da)
cmake's configure-time #defines (at least the way I use them) are
defined to 0 or 1, rather than sometimes not defined at all, so you
have to test them with plain #if rather than #ifdef or #if defined.
I _thought_ I'd caught all of those in previous fixes, but apparently
there were a couple still lurking. Oops.
(cherry picked from commit e289265e37)
You can't call sk_write_eof() twice on the same socket, because the
second one will fail an assertion. Instead, you're supposed to know
you've already sent EOF, and not try to send it again.
The call to sk_write_eof() in raw_special (triggered by pressing ^D in
GUI PuTTY, for example) sets the flag raw->sent_socket_eof in an
attempt to prevent this. But it doesn't _check_ that flag, so a second
press of ^D can reach that assertion failure.
(cherry picked from commit 9f2e1e6e03)
In ldisc's line editing mode, pressing ^U is supposed to erase the
current unsent line rather than inserting a literal ^U into the
buffer. In fact, when using a non-Telnet backend, it erases the
line *and* inserts ^U into the buffer!
This happens because it shares a case handler with three other
disruptive control characters (^C, ^\, ^Z), which all also clear the
line-editing buffer before doing their various actions. But in
non-Telnet mode, their actions become literal insertion of themselves,
so the combined effect is to erase the line and them self-insert.
I'm not 100% convinced that was what I actually meant to do with those
characters. But it _certainly_ wasn't what I meant to do with ^U, so
that one at least I should fix right now!
(cherry picked from commit 5ade8c0047)
