All the fiddly business where you have to check that a thing exists,
make sure of its type, find its size, allocate some memory, and then
read it again properly (or, alternatively, loop round dealing with
ERROR_MORE_DATA) just doesn't belong at every call site. It's crying
out to be moved out into some separate utility functions that present
a more ergonomic API, so that the code that decides _which_ Registry
entries to read and what to do with them can concentrate on that.
So I've written a fresh set of registry API wrappers in windows/utils,
and simplified windows/storage.c as a result. The jump-list handling
code in particular is almost legible now!
The function will accept a public key file or a PPK, but if it fails
to parse as any of those, the error message says "not a PuTTY SSH-2
private key", which is particularly incongruous in situations where
you're specifically _not_ after the private half of the key.
Now says "not a public key or a PuTTY SSH-2 private key".
Every time I've had to do this before, I've always done the three-line
dance of initialising a BinarySource and calling get_string on it.
It's long past time I wrapped that up into a convenient subroutine.
If you already have a string (of potentially-binary data) in the form
of a ptrlen reference to somewhere else, and you want to keep a copy
somewhere, it's useful to copy it into a strbuf. But it takes a couple
of lines of faff to do that, and it's nicer to wrap that up into a
tiny helper function.
This commit adds that helper function strbuf_dup, and its non-movable
sibling strbuf_dup_nm for secret data. Also, gone through the existing
code and found a bunch of cases where this makes things less verbose.
These days, the base64 module has 'b64decode', which can tolerate a
str or a bytes as input. Switched to using that, and also, imported it
under a nice short name 'b64'.
In the process, removed the obsolete equivocation between
base64.decodebytes and base64.decodestring. That was there to cope
with Python 2 - but the assert statement right next to it has been
enforcing P3 since commit 2ec2b796ed two years ago!
When a subsidiary part of the SSH system wants to abort the whole
connection, it's supposed to call ssh_sw_abort_deferred, on pain of
free-order confusion. Elsewhere in mainchan.c I was getting this
right, but I missed a couple.
We call setlocale() at the start of the function to get the current
LC_CTYPE locale, then set it to what we need during the function, and
then call setlocale() at the end to put it back again. But the middle
call is allowed to invalidate the pointer returned from the first, so
we have to save it in our own allocated storage until the end of the
function.
This bit me during development just now, and I was surprised that it
hadn't come up before! But I suppose this is one of those things
that's only _allowed_ to fail, and need not in all circumstances -
perhaps it depends on what your LC_CTYPE was set to before.
The list of kex methods recently ran out of space due to the addition
of NTRU (at least, if you have GSSAPI enabled). It's time to stop
having an arbitrary limit on those arrays and switch to doing it
properly.
I wasn't really satisfied with the previous version, but it was
easiest to get Stein's algorithm working on polynomials by doing it
exactly how I already knew to do it for integers. But now I've
improved it in two ways.
The first improvement I got from another implementation: instead of
transforming A into A - kB for some k that makes the constant term
zero, you can scale _both_ inputs, replacing A with mA - kB for some
k,m. The advantage is that you can calculate m and k very easily, by
making each one the constant term of the other polynomial, which means
you don't need to invert something mod q in every step. (Rather like
the projective-coordinates optimisations in elliptic curves, where
instead of inverting in every step you accumulate the product of all
the factors that need to be inverted, and invert the whole product
once at the very end.)
The second improvement is to abandon my cumbersome unwinding loop that
builds up the output coefficients by reversing the steps in the
original gcd-finding loop. Instead, I do the thing you do in normal
Euclid's algorithm: keep track of the coefficients as you go through
the original loop. I had wanted to do this before, but hadn't figured
out how you could deal with dividing a coefficient by x when (unlike
the associated real value) the coefficient isn't a multiple of x. But
the answer is very simple: x is invertible in the ring we're working
in (its inverse mod x^p-x-1 is just x^{p-1}-1), so you _can_ just
divide your coefficient by x, and moreover, very easily!
Together, these changes speed up the NTRU key generation by about a
factor of 1.5. And they remove lots of complicated code as well, so
everybody wins.
This consists of DJB's 'Streamlined NTRU Prime' quantum-resistant
cryptosystem, currently in round 3 of the NIST post-quantum key
exchange competition; it's run in parallel with ordinary Curve25519,
and generates a shared secret combining the output of both systems.
(Hence, even if you don't trust this newfangled NTRU Prime thing at
all, it's at least no _less_ secure than the kex you were using
already.)
As the OpenSSH developers point out, key exchange is the most urgent
thing to make quantum-resistant, even before working quantum computers
big enough to break crypto become available, because a break of the
kex algorithm can be applied retroactively to recordings of your past
sessions. By contrast, authentication is a real-time protocol, and can
only be broken by a quantum computer if there's one available to
attack you _already_.
I've implemented both sides of the mechanism, so that PuTTY and Uppity
both support it. In my initial testing, the two sides can both
interoperate with the appropriate half of OpenSSH, and also (of
course, but it would be embarrassing to mess it up) with each other.
This is already slightly nice because it lets me separate the
Weierstrass and Montgomery code more completely, without having to
have a vtable tucked into dh->extra. But more to the point, it will
allow completely different kex methods to fit into the same framework
later.
To that end, I've moved more of the descriptive message generation
into the vtable, and also provided the constructor with a flag that
will let it do different things in client and server.
Also, following on from a previous commit, I've arranged that the new
API returns arbitrary binary data for the exchange hash, rather than
an mp_int. An upcoming implementation of this interface will want to
return an encoded string instead of an encoded mp_int.
Until now, every kex method has represented the output as an mp_int.
So we were storing it in the mp_int field s->K, and adding it to the
exchange hash and key derivation hashes via put_mp_ssh2.
But there's now going to be the first kex method that represents the
output as a string (so that it might have the top bit set, or multiple
leading zero bytes, without its length varying). So we now need to be
more general.
The most general thing it's sensible to do is to replace s->K with a
strbuf containing _already-encoded_ data to become part of the hash,
including length fields if necessary. So every existing kex method
still derives an mp_int, but then immediately puts it into that strbuf
using put_mp_ssh2 and frees it.
This means if I have functions like foo_subfoo_bar and foo_baz that
both operate on a foo, the Python testcrypt system can translate both
into .bar() and .baz() methods on the object, even though they don't
start with the same prefix.
bool is dangerous in a time-safe context, because C compilers might
insert a control flow divergence to implement the implicit
normalisation of nonzero integers to 1 when you assign to a bool.
Everywhere else time-safe, I avoid using it; but smemeq has been an
exception until now, because the response to smemeq returning failure
was to do an obvious protocol-level divergence _anyway_ (like
disconnecting due to MAC mismatch).
But I'm about to want to use smemeq in a context where I use the
result _subtly_ and don't want to give away what it is, so now it's
time to get rid of that bool and have smemeq return unsigned.
This reallocs an existing mp_int to have a different physical size,
e.g. to make sure there's enough space at the top of it.
Trivial, but I'm a little surprised I haven't needed it until now!
In test_primegen, we loop round retrieving random data until we find
some that will permit a successful prime generation, so that we can
log only the successful attempts, and not the failures (which don't
have to be time-safe). But this itself introduces a potential mismatch
between logs, because the simplistic RNG used in testsc will have
different control flow depending on how far through a buffer of hash
data it is at the start of a given run.
random_advance_counter() gives it a fresh buffer, so calling that at
the start of a run should normalise this out. The code to do that was
already in the middle of random_read(); I've just pulled it out into a
separately callable function.
This hasn't _actually_ caused failures in test_primegen, but I'm not
sure why not. (Perhaps just luck.) But it did cause a failure in
another test of a similar nature, so before I commit _that_ test (and
the thing it's testing), I'd better fix this.
They're pretty much self-contained, and don't really need to be in the
same module as sshpubk.c (which has other dependencies). Move them out
into a utils module, where pulling them in won't pull in anything else
unwanted.
Using a new screenshot-taking module I just added in windows/utils,
these new options allow me to start up one of the tools with
demonstration window contents and automatically save a .BMP screenshot
to disk. This will allow me to keep essentially the same set of demo
images and update them easily to keep pace with the current appearance
of the real tools as PuTTY - and Windows itself - both evolve.
Once we've actually loaded a key file, the job of updating the UI
fields is now done by a subroutine update_ui_after_load(), so that I
can call it from a different context in an upcoming commit.
I've been keeping them up to date with API changes as far as making
sure they still _compile_, but today I tried to actually run them, and
found that they were making a couple of segfault-inducing mistakes:
not filling in their vtable pointer, and not returning a 'realhost'
string. Now fixed.
If an SSH proxy socket is frozen for long enough, and the SSH server
continues to send, then sooner or later the proxy SSH connection will
end up having to freeze its underlying physical socket too. When the
proxy socket is later unfrozen, it needs to pass that unfreezing on in
turn.
The way this should happen is that when the SshProxy begins to clear
the backlog of data passed to it from the proxy SSH connection via
seat_stdout, it should call backend_unthrottle to inform that proxy
connection that the backlog is clearing.
But there was no backlog_unthrottle call in the whole of sshproxy.c.
Now there is.
I got a pterm into a stuck state this morning by an accidental mouse
action. I'd intended to press Ctrl + right-click to pop up the context
menu, but I accidentally pressed down the left button first, starting
a selection drag, and then while the left button was still held down,
pressed down the right button as well, triggering the menu.
The effect was that the context menu appeared while term->selstate was
set to DRAGGING, in which state terminal output is suppressed, and
which is only unset by a mouse-button release event. But then that
release event went to the popup menu, and the terminal window never
got it. So the terminal stayed stuck forever - or rather, until I
guessed the cause and did another selection drag to reset it.
This happened to me on GTK, but once I knew how I'd done it, I found I
could reproduce the same misbehaviour on Windows by the same method.
Added a simplistic fix, on both platforms, that cancels a selection
drag if the popup menu is summoned part way through it.
This gets us scalable icons that will go to extremely large sizes
without the problems that arise from scaling up the output of
mkicon.py, in which outlines become too thin because the script was
mostly concerned with trying to squeeze all the desired detail into
_tiny_ sizes.
The SVG icons are generated by mksvg.py, which is a conversion of the
existing mkicon.py. So the SVG files themselves are not committed in
this repo; 'make svg' in the icons subdir will generate them.
(I haven't decided yet whether this state of affairs should be
permanent. Perhaps _having_ generated the SVGs via a similar program
to the bitmap icons, we should regard the script as a discardable
booster stage and redesignate the SVGs themselves as the source format
for future modifications, so that they can be edited in Inkscape or
similar rather than by tinkering with Python. On the other hand,
perhaps keeping the script will make it easier to keep the icon family
consistent, e.g. if changing the style of one of the shared visual
components.)
My plan is that we should stick with the output of the previous
bitmap-generating script for all the _small_ icons, up to and
including 48 pixels, because it does a better job at low resolution.
(That was really what it was for in the first place: you can think of
it as an analogue of a scalable-font hinting system, to tune the
scaling for very low res so that all the important features are still
visible.)
I think probably I want to switch the 128-pixel icons used in the Mac
icon file over to being rendered from the SVG (though in this commit I
haven't gone that far, not least because I'll also need to prepare a
corresponding black and white version). I haven't done extensive
research yet to decide where I think the crossover point in between
is.
While testing the unrelated pile of commits just past, I accidentally
started a Cygwin saved session I hadn't run in ages which used the old
Telnet-based cygtermd as a local proxy command, and found that it
presented the Cygwin prompt with a trust sigil. Oops!
It turns out that this is because interactor_return_seat does two
things that can change the real seat's trust status, and it does them
in the wrong order: it defaults the status back to trusted (as if the
seat was brand new, because that's how they start out), and it calls
tempseat_flush which may have buffered a trust-status reset while the
seat was borrowed. The former should not override the latter!
This is another fallback needed on Win95, where the Win32 API
functions to convert between multibyte and wide strings exist, but
they haven't heard of the UTF-8 code page. PuTTY can't really do
without that these days.
(In particular, if a server sends a remote window-title escape
sequence while the terminal is in UTF-8 mode, then _something_ needs
to translate the UTF-8 data into Unicode for Windows to reconvert into
the character set used in window titles.)
This is a weird enough thing to be doing that I've put it under the
new #ifdef LEGACY_WINDOWS, so behaviour in the standard builds should
be unchanged.
This makes Pageant run on Win95 again. Previously (after fixing the
startup-time failures due to missing security APIs) it would go into
an uninterruptible CPU-consuming spin in the message loop when every
attempt to retrieve its messages failed because PeekMessageW doesn't
work at all on the 95 series.
Now it can be called from places other than Pageant's WinMain(). In
particular, the attempt to make a security descriptor in
lock_interprocess_mutex() is gated on it.
In return, however, I've tightened up the semantics. In normal PuTTY
builds that aren't trying to support pre-NT systems, the function
*unconditionally* returns true, on the grounds that we don't expect to
target any system that doesn't support the security APIs, and if
someone manages to contrive one anyway - or, more likely, if we some
day introduce a bug in our loading of the security API functions -
then this safety catch should make Pageant less likely to accidentally
fall back to 'never mind, just run in insecure mode'.
Turns out that PuTTY hasn't run successfully on legacy Windows since
0.66, in spite of an ongoing intention to keep it working. Among the
reasons for this is that CreateWindowExW simply fails with
ERROR_CALL_NOT_IMPLEMENTED: apparently Win95 and its ilk just didn't
have fully-Unicode windows as an option.
Fixed by resurrecting the previous code from the git history (in
particular, code removed by commit 67e5ceb9a8 was useful), and
including it as a runtime alternative.
One subtlety was that I found I had to name the A and W window classes
differently (by appending ".ansi" to the A one): apparently they
occupy the same namespace even though the names are in different
character sets, so if you somehow manage to register both classes,
they'll collide with each other without that tweak.
These are more functions that don't exist on all our supported legacy
versions of Windows, so we need to follow the same policy as
everywhere else, by trying to acquire them at run time and having a
fallback if they aren't available.
This fixes a load-time failure on versions of Windows too old to have
that function in kernel32.dll.
We use it to determine whether a file was safe to overwrite in the
context of PuTTY session logging: if it's safe, we skip the 'do you
want to overwrite or append?' dialog box.
On earlier Windows you can use FindFirstFile to get a similar effect,
so that's what we fall back to. It's not quite the same, though - if
you pass a wildcard then it will succeed when you'd rather it had
failed. But it's good enough to at least work in normal cases.
This will be used to wrap some of the stranger workarounds we're
keeping in this code base for the purposes of backwards compatibility
to seriously old platforms like Win95.
This way, anyone who needs to use the version data can quickly call
init_winver to make sure it's been set up, and not waste too much faff
redoing the actual work.
By testing on a platform slow enough to show the flicker, I happened
to notice that if your shell prompt resets the window title every time
it's displayed, this was actually resulting in a call to SetWindowText
every time, which caused the GUI to actually do work.
There's certainly no need for that! We can at least avoid bothering if
the new title is identical to the old one.
It turns out that they're still NULL at the first moment that a
SetWindowText call tries to read one of them, because they weren't
initialised at startup! Apparently SetWindowText notices that it's
been passed a null pointer, and does nothing in preference to failing,
but it's still not what I _meant_ to do.
Checking various implementations of these functions against each
other, I noticed by eyeball review that some of the special cases in
mb_to_wc() never check the buffer limit at all. Yikes!
Fortunately, I think there's no vulnerability, because these special
cases are ones that write out at most one wide char per multibyte
char, and at all the call sites (including dup_mb_to_wc) we allocate
that much even for the first attempt. The only exception to that is
the call in key_event() in unix/window.c, which uses a fixed-size
output buffer, but its input will always be the data generated by an X
keystroke event. So that one can only overrun the buffer if an X key
event manages to translate into more than 32 wide characters of text -
and even if that does come up in some exotic edge case, it will at
least not be happening under _enemy_ control.
