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.)
These are all going to be used by a test program I have in the works,
which will need to link against a lot more of the code base than any
so far. So we need a pile of new stubs.
The trickiest of these was stubs/no-network.c, which had to
conditionally define a couple of extra network functions, because
there are Windows-specific plug_closing_system_error and
plug_closing_winsock_error functions.
CONF_bold_style is a pair of bit flags rather than an enum, so its
values aren't just BOLD_STYLE_FONT and BOLD_STYLE_COLOUR but also the
bitwise OR of them. (Hopefully not neither.)
The Windows version of the Filename structure now contains three
versions of the pathname, in UTF-16, UTF-8 and the system code page.
Callers can use whichever is most convenient.
All uses of filenames for actually opening files now use the UTF-16
version, which means they can tolerate 'exotic' filenames, by which I
mean those including Unicode characters outside the host system's
CP_ACP default code page.
Other uses of Filename structures inside the 'windows' subdirectory do
something appropriate, e.g. when printing a filename inside a message
box or a console message, we use the UTF-8 version of the filename
with the UTF-8 version of the appropriate API.
There are three remaining pieces to full Unicode filename support:
One is that the cross-platform code has many calls to
filename_to_str(), embodying the assumption that a file name can be
reliably converted into the unspecified current character set; those
will all need changing in some way.
Another is that write_setting_filename(), in windows/storage.c, still
saves filenames to the Registry as an ordinary REG_SZ in the system
code page. So even if an exotic filename were stored in a Conf, that
Conf couldn't round-trip via the Registry and back without corrupting
that filename by coercing it back to a string that fits in CP_ACP and
therefore doesn't represent the same file. This can't be fixed without
a compatibility break in the storage format, and I don't want to make
a minimal change in that area: if we're going to break compatibility,
then we should break it good and hard (the Nanny Ogg principle), and
devise a completely fresh storage representation that fixes as many
other legacy problems as possible at the same time. So that's my plan,
not yet started.
The final point, much more obviously, is that we're still short of
methods to _construct_ any Filename structures using a Unicode input
string! It should now work to enter one in the GUI configurer (either
by manual text input or via the file selector), but it won't
round-trip through a save and load (as discussed above), and there's
still no way to specify one on the command line (the groundwork is
laid by commit 10e1ac7752 but not yet linked up).
But this is a start.
I think the only reason it currently takes a plain string is because
its interesting caller (in unix/x11.c) has just constructed a string
out of an environment variable, and it seemed like the path of least
effort not to bother wrapping it into a proper Filename. But when
Filename on Windows becomes more interesting, we'll need it to take
the full version.
The values of that field in a Control structure are already
platform-dependent: you're only supposed to set them in cross-platform
code by using #defined names that each platform will define
differently.
Now I need the _type_ as well as the values to be opaque, because I'm
about to make a change on Windows that turns it into a wide character
string instead of a char string.
We already had encode_wide_string_as_utf8, which treats the wide
string as UTF-16 or UTF-32 as appropriate to the size of wchar_t. I'm
about to need the inverse function, and was surprised that it didn't
already exist (even though enough component parts did to make it easy).
message_box() previously differed from the real MessageBox API
function in that it permitted the user to provide a help context to be
used for a Help button in the dialog box.
Now it adds a second unusual ability: you can specify that the text
and caption strings are in UTF-8 rather than the system code page.
Having constructed a conio object with its own 'utf8' flag, we should
be checking that flag at time of use rather than the global
conio_use_utf8 (which was already taken into account at setup time).
Otherwise we miss the whole point, which is that without the override
flag turning off UTF-8, _some_ uses of the system should use the
default code page and not UTF-8.
In the key generation step where we invert 3f in the field
Z_q/<x^p-x-1>, I was carefully checking for failure, on the grounds
that even a field does have _one_ non-invertible element, namely zero.
But I forgot that we'd generated f in such a way that it can't
possibly be zero. So that failure check is pointless.
(However, I've retained it in the form of an assertion.)
In commit f9e572595b I claimed that I'd removed very nearly all
the global and static variables from windows/window.c. It turns out
that this was wildly overoptimistic - I missed quite a few of them!
I'm not quite sure how I managed that; my best guess is that I used an
underpowered 'nm' command that failed to find some classes of
variable.
Some of the remaining function-scope statics were removed completely
by commit afb3dab1e9 just now. In this commit, I've swept up some
more and turn them into fields of WinGuiSeat, where they should have
been moved last September.
The (hopefully complete this time) list of remaining variables,
generated by running this rune in the Windows build directory:
nm windows/CMakeFiles/putty.dir/window.c.obj |
grep -E '^([^ ]+)? *[bBcCdDgGsS] [^\.]'
consists of the following variables which are legitimately global
across the whole process and not related to a particular window:
- 'hinst' and 'hprev', instance handles for Windows loadable modules
- 'classname' in the terminal_window_class_a() and
terminal_window_class_w() functions, which allocate a window class
reusably
- some pointers to Windows API functions retrieved via the
DECL_WINDOWS_FUNCTION / GET_WINDOWS_FUNCTION system, such as
p_AdjustWindowRectExForDpi and p_FlashWindowEx
- some pointers to Windows API functions set up by assigning them at
startup to the right one of the ANSI or Unicode version depending on
the Windows version, e.g. sw_DefWindowProc and sw_DispatchMessage
- 'unicode_window', a boolean flag set at the same time as those
sw_Foo function pointers
- 'sesslist', storing the last-retrieved version of the saved
sessions menu
- 'cursor_visible' in show_mouseptr() and 'forced_visible' in
update_mouse_pointer(), each of which tracks the cumulative number
of times that function has shown or hidden the mouse pointer, so as
to manage its effect on the global state updated by ShowCursor
- 'trust_icon', loaded from the executable's resources
- 'wgslisthead', the list of all active WinGuiSeats
- 'wm_mousewheel', the window-message id we use for mouse wheel
events
and the following which are nothing to do with our code:
- '_OptionsStorage' in __local_stdio_printf_options() and
__local_stdio_scanf_options(), which I'd never noticed before, but
apparently are internal to a standard library header.
lpDx_maybe was a pointer defined to point at either lpDx itself or
NULL, depending on whether the code decided it needed to pass the lpDx
array of per-character pixel offsets to various functions during
drawing (based in turn on whether the font was variable-pitch).
lpDx is reallocated as necessary, which means lpDx_maybe must be kept
up to date. This was achieved by resetting it to lpDx if it was
already non-NULL.
But lpDx starts out as NULL before the first reallocation, so that
this can't work - it'll be initialised to NULL even if we _did_ want
to use it, and then at the first realloc, it won't be updated!
Before the previous commit turned lpDx from a static into an automatic
variable, this would have been a rare bug affecting only the first
call to the function. Now it will happen all the time, which is
better, because we can notice and fix it.
Replaced lpDx_maybe completely with a boolean flag indicating whether
we should pass lpDx to drawing functions.
In windows/window.c, a few variables inside functions were declared as
static, with no particular purpose that I can see: they don't seem to
have any reason to persist between calls to the function. So it makes
more sense to have them be ordinary stack-allocated automatic
variables.
Static variables removed by this commit:
- 'RECT ss' in reset_window.
- 'WORD keys[3]' and 'BYTE keysb[3]' in TranslateKey.
- several (buffer, length) pairs in do_text_internal.
- keys_unicode[] in TranslateKey.
All of these variables were originally introduced in patches credited
to Robert de Bath, which means I can't even try to reconstruct my
original thought processes, because they weren't _my_ thoughts anyway.
The arrays in do_text_internal are the easiest to understand: they're
reallocated larger as necessary, and making them static means the
allocation from a previous call can be reused, saving a malloc (though
I don't think that's a good enough reason to bother, these days).
The fixed-size static arrays and RECT are harder to explain. I suspect
they might originally have been that way because of 1990s attitudes to
performance: in x86-32 it's probably marginally faster to give your
variables constant addresses than sp-relative ones, and in the 1990s
computers were much slower, so there's an argument for making things
static if you have no _need_ to make them automatic. These days, the
difference is negligible, and persistent state is much more widely
recognised as a risk!
But keys_unicode[] is by far the strangest, because there was code
that clearly _did_ expect it to persist between calls, namely three
assignments to keys_unicode[0] near the end of the function after it's
finished being used for any other purpose, and a conditioned-out set
of debug() calls at the top of the function that print its contents
before anything has yet written to it.
But as far as I can see, the persistent data in the array is otherwise
completely unused. In any call to the function, if keys_unicode is
used at all, then it's either written directly by a call to ToAsciiEx,
or else (for pre-NT platforms) converted from ToAsciiEx's output via
MultiByteToWideChar. In both cases, the integer variable 'r' indicates
how many array elements were written, and subsequent accesses only
ever read those elements. So the assignments to keys_unicode[0] at the
end of the previous call will be overwritten before anything at all
can depend on them - with the exception of those debug statements.
I don't really understand what was going on here. It's tempting to
guess that those final assignments must have once done something
useful, and the code that used them was later removed. But the source
control history doesn't bear that out: a static array of three
elements (under its original name 'keys') was introduced in commit
0d5d39064a, and then commits 953b7775b3 and 26f1085038
added the other two assignments. And as far as I can see, even as of
the original commit 0d5d39064a, the code already had the property
that there was a final assignment to keys[0] which would inevitably be
overwritten in the next call before it could affect anything.
So I'm totally confused about what those assignments were _ever_
useful for. But an email thread from the time suggests that some of
those patches were being rebased repeatedly past other work (or
rather, the much less reliable CVS analogue of rebasing), so my best
guess is that that's where the confusion crept in - perhaps in RDB's
original version of the code they did do something useful.
Regardless of that, I'm pretty convinced that persistent array can't
be doing anything useful _now_. So I'm taking it out. But if anyone
reports a bug resulting from this change, then I'll eat my words - and
with any luck the details of the bug report will give us a clue what's
going on, and then we can put back some equivalent functionality with
much better comments!
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)
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").
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.
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).
A user reported yesterday that PuTTY can fail to print a userauth
banner message if the server sends one and then immediately slams the
connection shut. The first step to fixing this is making a convenient
way to reproduce that server behaviour.
(Apparently the real use case has to do with account expiry - the
server in question presumably doesn't have enough layer violations to
be able to put the text "Your account has expired" into an
SSH_MSG_DISCONNECT, so instead it does the next best thing and sends
it as a userauth banner immediately before disconnection.)
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.)
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)
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.
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.
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)
