This was pointed out by another compiler warning. The 'name' parameter
of inquire_cred_by_mech is not a gss_name_t (which is the type of
GSS_C_NO_NAME); it's a gss_name_t *, because it's an _output_
parameter. We're not telling the library that we aren't _passing_ a
name: we're telling it that we don't need it to _return_ us a name. So
the appropriate null pointer representation is just NULL.
(This was harmless apart from a compiler warning, because gss_name_t
is a pointer type in turn and GSS_C_NO_NAME expands to a null pointer
anyway. It was just a wrongly-typed null pointer.)
Heimdal provides its own definitions of OIDs like GSS_C_NT_USER_NAME
in the form of macros, which conflict with our attempt to redefine
them as variables - the macro gets expanded into the middle of the
variable declaration, leaving the poor C compiler trying to parse a
non-declaration along the lines of
const_gss_OID (&__gss_c_nt_anonymous_oid_desc) = oids+5;
Easily fixed by just not redefining these at all if they're already
defined as macros. To make that easier, I've broken up the oids[]
array into individual gss_OID_desc declarations, so I can put each one
inside the appropriate ifdef.
In the process, I've removed the 'const' from the gss_OID_desc
declarations. That's on purpose! The problem is that not all
implementations of the GSSAPI headers make const_gss_OID a pointer to
a *const* gss_OID_desc; sometimes it's just a plain one and the
'const' prefix is just a comment to the user. So removing that const
prevents compiler warnings (or worse) about address-taking a const
thing and assigning it into a non-const pointer.
I mentioned recently (in commit 9e7d4c53d8) message that I'm no
longer fond of the variable name 'ret', because it's used in two quite
different contexts: it's the return value from a subroutine you just
called (e.g. 'int ret = read(fd, buf, len);' and then check for error
or EOF), or it's the value you're preparing to return from the
_containing_ routine (maybe by assigning it a default value and then
conditionally modifying it, or by starting at NULL and reallocating,
or setting it just before using the 'goto out' cleanup idiom). In the
past I've occasionally made mistakes by forgetting which meaning the
variable had, or accidentally conflating both uses.
If all else fails, I now prefer 'retd' (short for 'returned') in the
former situation, and 'toret' (obviously, the value 'to return') in
the latter case. But even better is to pick a name that actually says
something more specific about what the thing actually is.
One particular bad habit throughout this codebase is to have a set of
functions that deal with some object type (say 'Foo'), all *but one*
of which take a 'Foo *foo' parameter, but the foo_new() function
starts with 'Foo *ret = snew(Foo)'. If all the rest of them think the
canonical name for the ambient Foo is 'foo', so should foo_new()!
So here's a no-brainer start on cutting down on the uses of 'ret': I
looked for all the cases where it was being assigned the result of an
allocation, and renamed the variable to be a description of the thing
being allocated. In the case of a new() function belonging to a
family, I picked the same name as the rest of the functions in its own
family, for consistency. In other cases I picked something sensible.
One case where it _does_ make sense not to use your usual name for the
variable type is when you're cloning an existing object. In that case,
_neither_ of the Foo objects involved should be called 'foo', because
it's ambiguous! They should be named so you can see which is which. In
the two cases I found here, I've called them 'orig' and 'copy'.
As in the previous refactoring, many thanks to clang-rename for the
help.
In the course of recent refactorings I noticed a couple of cases where
we were doing old-fashioned preallocation of a char array with some
conservative maximum size, then writing into it via *p++ or similar
and hoping we got the calculation right.
Now we have strbuf and dupcat, so we shouldn't ever have to do that.
Fixed as many cases as I could find by searching for allocations of
the form 'snewn(foo, char)'.
Particularly worth a mention was the Windows GSSAPI setup code, which
was directly using the Win32 Registry API, and looks much more legible
using the windows/utils/registry.c wrappers. (But that was why I had
to enhance them in the previous commit so as to be able to open
registry keys read-only: without that, the open operation would
actually fail on this key, which is not user-writable.)
Also unix/askpass.c, which was doing a careful reallocation of its
buffer to avoid secrets being left behind in the vacated memory -
which is now just a matter of ensuring we called strbuf_new_nm().
dh_is_gex() expects to find a 'struct dh_extra' in the 'extra' field
of the kex_alg you pass in, and won't look kindly on finding an
instance of some totally different structure type. We were being
careful about that everywhere in the GSSAPI kex code except for the
final free step.
This is like the seat-independent nonfatal(), but specifies a Seat,
which allows the GUI dialog box to have the right terminal window as
its parent (if there are multiple ones).
Changed over all the nonfatal() calls in the code base that could be
localised to a Seat, which means all the ones that come up if
something goes horribly wrong in host key storage. To make that
possible, I've added a 'seat' parameter to store_host_key(); it turns
out that all its call sites had one available already.
Just spotted this in eyeball review: we're about to construct our new
outgoing KEXINIT and write it into the strbuf s->outgoing_kexinit. So
we should clear that strbuf first. But in fact we were clearing
s->client_kexinit, which aliases s->outgoing_kexinit in an SSH client,
but in a server, aliases s->incoming_kexinit.
This was harmless in PuTTY (since the strbuf we cleared was the right
one anyway). And it was harmless in Uppity's initial kex (since the
strbuf we _meant_ to clear was empty anyway). But if Uppity had ever
initiated a rekey, this would have exploded messily.
I only realised this bug while writing up the feature for the
wishlist:
It's one thing _at connection startup_ to delay sending your KEXINIT
until the server has sent its: the server is very likely to send it
anyway (unless it's attempting the same workaround against us), so
probably nothing goes wrong.
But if we want to initiate a rekey, we do that _by_ sending a KEXINIT.
In that situation we can't just wait until the server sends one,
because it has no idea it's supposed to be doing so!
Happily, in that situation, we already have a KEXINIT from the server,
left over from the previous key exchange. So we can filter against
that, and still have the intended effect of not spending KEXINIT space
on algorithms the server doesn't know about.
My correspondent on the new authentication-plugin feature reports that
their plugin is not reliably receiving the final PLUGIN_AUTH_SUCCESS
message on Windows. I _think_ this is because the whole userauth layer
is being dismissed, leading to sk_close() of the Socket talking to the
plugin, before the data has actually been written to the outgoing
pipe.
This should fix it: track the Socket's backlog, and immediately after
sending that message, wait until we receive a notification that the
backlog has decreased to size 0. That stops us from terminating the
userauth layer until the message has left our process.
In recent months I've had two requests from different people to build
support into PuTTY for automatically handling complicated third-party
auth protocols layered on top of keyboard-interactive - the kind of
thing where you're asked to enter some auth response, and you have to
refer to some external source like a web server to find out what the
right response _is_, which is a pain to do by hand, so you'd prefer it
to be automated in the SSH client.
That seems like a reasonable thing for an end user to want, but I
didn't think it was a good idea to build support for specific
protocols of that kind directly into PuTTY, where there would no doubt
be an ever-lengthening list, and maintenance needed on all of them.
So instead, in collaboration with one of my correspondents, I've
designed and implemented a protocol to be spoken between PuTTY and a
plugin running as a subprocess. The plugin can opt to handle the
keyboard-interactive authentication loop on behalf of the user, in
which case PuTTY passes on all the INFO_REQUEST packets to it, and
lets it make up responses. It can also ask questions of the user if
necessary.
The protocol spec is provided in a documentation appendix. The entire
configuration for the end user consists of providing a full command
line to use as the subprocess.
In the contrib directory I've provided an example plugin written in
Python. It gives a set of fixed responses suitable for getting through
Uppity's made-up k-i system, because that was a reasonable thing I
already had lying around to test against. But it also provides example
code that someone else could pick up and insert their own live
response-provider into the middle of, assuming they were happy with it
being in Python.
No functional change, but I've pulled the bulk of the k-i setup and
prompting code out of ssh2_userauth_process_queue and into
subroutines, in preparation for wanting to do the same work in more
than one place in the main coroutine's control flow.
I'm going to want to use it in an upcoming commit, because together
with 'savedhost', it forms the identification of an SSH server (at
least as far as the host key cache is concerned, and therefore it's
appropriate for other uses too).
We were already passing the hostname through for use in user-facing
prompts (not to mention the FQDN version for use in GSSAPI).
We've occasionally had reports of SSH servers disconnecting as soon as
they receive PuTTY's KEXINIT. I think all such reports have involved
the kind of simple ROM-based SSH server software you find in small
embedded devices.
I've never been able to prove it, but I've always suspected that one
possible cause of this is simply that PuTTY's KEXINIT is _too long_,
either in number of algorithms listed or in total length (especially
given all the ones that end in @very.long.domain.name suffixes).
If I'm right about either of those being the cause, then it's just
become even more likely to happen, because of all the extra
Diffie-Hellman groups and GSSAPI algorithms we just threw into our
already-long list in the previous few commits.
A workaround I've had in mind for ages is to wait for the server's
KEXINIT, and then filter our own down to just the algorithms the
server also mentioned. Then our KEXINIT is no longer than that of the
server, and hence, presumably fits in whatever buffer it has. So I've
implemented that workaround, in anticipation of it being needed in the
near future.
(Well ... it's not _quite_ true that our KEXINIT is at most the same
length as the server. In fact I had to leave in one KEXINIT item that
won't match anything in the server's list, namely "ext-info-c" which
gates access to SHA-2 based RSA. So if we turn out to support
absolutely everything on all the server's lists, then our KEXINIT
would be a few bytes longer than the server's, even with this
workaround. But that would only cause trouble if the server's outgoing
KEXINIT was skating very close to whatever buffer size it has for the
incoming one, and I'm guessing that's not very likely.)
((Another possible cause of this kind of disconnection would be a
server that simply objects to seeing any KEXINIT string it doesn't
know how to speak. But _surely_ no such server would have survived
initial testing against any full-featured client at all!))
This is surprisingly simple, because it wasn't necessary to touch the
GSS parts at all. Nothing changes about the message formats between
integer DH and ECDH in GSS KEX, except that the mpints sent back and
forth as part of integer DH are replaced by the opaque strings used in
ECDH. So I've invented a new KEXTYPE and made it control a bunch of
small conditionals in the middle of the GSS KEX code, leaving the rest
unchanged.
None of the constructors can fail and return NULL. I think this must
have come in with a lot of other unnecessary null-pointer checks when
ECC support was first added. I've got rid of most of them since then,
but that one apparently escaped my notice.
Introduced recently by commit 42740a5455, in which I decided to
call ssh_key_cache_str() even on certified host keys. But that call
was conditional on s->hkey being non-NULL (which happens in GSS KEX)
as well as on it not being certified, and I managed to absentmindedly
remove _both_ conditions. As a result we got a null-pointer
dereference on any GSS kex.
I ran 'ls /usr/share/doc' in a psusan session the other day and the
output hung part way through the long directory listing. This turned
out to be because the ssh-connection channel window had run out
temporarily, and PuTTY had sent a WINDOW_ADJUST extending it again,
which the connection layer acted on by calling chan_set_input_wanted
... which sesschan was ignoring with a comment saying /* I don't think
we need to do anything here */.
Well, turns out we do need to. Implemented the simplest possible
unblocking action.
I only recently found out that OpenSSH defined their own protocol IDs
for AES-GCM, defined to work the same as the standard ones except that
they fixed the semantics for how you select the linked cipher+MAC pair
during key exchange.
(RFC 5647 defines protocol ids for AES-GCM in both the cipher and MAC
namespaces, and requires that you MUST select both or neither - but
this contradicts the selection policy set out in the base SSH RFCs,
and there's no discussion of how you resolve a conflict between them!
OpenSSH's answer is to do it the same way ChaCha20-Poly1305 works,
because that will ensure the two suites don't fight.)
People do occasionally ask us for this linked cipher/MAC pair, and now
I know it's actually feasible, I've implemented it, including a pair
of vector implementations for x86 and Arm using their respective
architecture extensions for multiplying polynomials over GF(2).
Unlike ChaCha20-Poly1305, I've kept the cipher and MAC implementations
in separate objects, with an arm's-length link between them that the
MAC uses when it needs to encrypt single cipher blocks to use as the
inputs to the MAC algorithm. That enables the cipher and the MAC to be
independently selected from their hardware-accelerated versions, just
in case someone runs on a system that has polynomial multiplication
instructions but not AES acceleration, or vice versa.
There's a fourth implementation of the GCM MAC, which is a pure
software implementation of the same algorithm used in the vectorised
versions. It's too slow to use live, but I've kept it in the code for
future testing needs, and because it's a convenient place to dump my
design comments.
The vectorised implementations are fairly crude as far as optimisation
goes. I'm sure serious x86 _or_ Arm optimisation engineers would look
at them and laugh. But GCM is a fast MAC compared to HMAC-SHA-256
(indeed compared to HMAC-anything-at-all), so it should at least be
good enough to use. And we've got a working version with some tests
now, so if someone else wants to improve them, they can.
This provides a convenient hook to be called between SSH messages, for
the crypto components to do any per-message processing like
incrementing a sequence number.
In the situation where a MAC and cipher implementation are tied
together by being facets of the same underlying object (used by the
inseparable ChaCha20 + Poly1305 pair), previously we freed them by
having the cipher_free function actually do the freeing, having the
mac_free function do nothing, and taking great care to call those in
the right order. (Otherwise, the mac_free function dereferences a
no-longer-valid vtable pointer and doesn't get as far as _finding out_
that it doesn't have to do anything.)
That's a time bomb in general, and especially awkward in situations
like testcrypt where we don't get precise control over freeing order
in any case. So I've replaced that system with one in which there are
two flags in the ChaCha20-Poly1305 structure, saying whether each of
the cipher and MAC facets is currently considered to be allocated.
When the last of those flags is cleared, the object is actually freed.
So now they can be freed in either order.
OpenSSH, when called on to give the fingerprint of a certified public
key, will in many circumstances generate the hash of the public blob
of the _underlying_ key, rather than the hash of the full certificate.
I think the hash of the certificate is also potentially useful (if
nothing else, it provides a way to tell apart multiple certificates on
the same key). But I can also see that it's useful to be able to
recognise a key as the same one 'really' (since all certificates on
the same key share a private key, so they're unavoidably related).
So I've dealt with this by introducing an extra pair of fingerprint
types, giving the cross product of {MD5, SHA-256} x {base key only,
full certificate}. You can manually select which one you want to see
in some circumstances (notably PuTTYgen), and in others (such as
diagnostics) both fingerprints will be emitted side by side via the
new functions ssh2_double_fingerprint[_blob].
The default, following OpenSSH, is to just fingerprint the base key.
This patch fixes a few other whitespace and formatting issues which
were pointed out by the bulk-reindent or which I spotted in passing,
some involving manual editing to break lines more nicely.
I think the weirdest hunk in here is the one in windows/window.c
TranslateKey() where _half_ of an assignment statement inside an 'if'
was on the same line as the trailing paren of the if condition. No
idea at all how that one managed to happen!
I think a lot of these were inserted by a prior run through GNU indent
many years ago. I noticed in a more recent experiment that that tool
doesn't always correctly distinguish which instances of 'id * id' are
pointer variable declarations and which are multiplications, so it
spaces some of the former as if they were the latter.
If the function name (or expression) in a function call or declaration
is itself so long that even the first argument doesn't fit after it on
the same line, or if that would leave so little space that it would be
silly to try to wrap all the run-on lines into a tall thin column,
then I used to do this
ludicrously_long_function_name
(arg1, arg2, arg3);
and now prefer this
ludicrously_long_function_name(
arg1, arg2, arg3);
I picked up the habit from Python, where the latter idiom is required
by Python's syntactic significance of newlines (you can write the
former if you use a backslash-continuation, but pretty much everyone
seems to agree that that's much uglier). But I've found it works well
in C as well: it makes it more obvious that the previous line is
incomplete, it gives you a tiny bit more space to wrap the following
lines into (the old idiom indents the _third_ line one space beyond
the second), and I generally turn out to agree with the knock-on
indentation decisions made by at least Emacs if you do it in the
middle of a complex expression. Plus, of course, using the _same_
idiom between C and Python means less state-switching.
So, while I'm making annoying indentation changes in general, this
seems like a good time to dig out all the cases of the old idiom in
this code, and switch them over to the new.
My bulk indentation check also turned up a lot of cases where a run-on
function call or if statement didn't have its later lines aligned
correctly relative to the open paren.
I think this is quite easy to do by getting things out of
sync (editing the first line of the function call and forgetting to
update the rest, perhaps even because you never _saw_ the rest during
a search-replace). But a few didn't quite fit into that pattern, in
particular an outright misleading case in unix/askpass.c where the
second line of a call was aligned neatly below the _wrong_ one of the
open parens on the opening line.
Restored as many alignments as I could easily find.
In several pieces of development recently I've run across the
occasional code block in the middle of a function which suddenly
switched to 2-space indent from this code base's usual 4. I decided I
was tired of it, so I ran the whole code base through a re-indenter,
which made a huge mess, and then manually sifted out the changes that
actually made sense from that pass.
Indeed, this caught quite a few large sections with 2-space indent
level, a couple with 8, and a handful of even weirder things like 3
spaces or 12. This commit fixes them all.
In the case where a server presents a host key signed by a different
certificate from the one you've configured, it need not _always_ be
evidence of wrongdoing. I can imagine situations in which two CAs
cover overlapping sets of things, and you don't want to blanket-trust
one of them, but you do want to connect to a specific host signed by
that one.
Accordingly, PuTTY's previous policy of unconditionally aborting the
connection if certificate validation fails (which was always intended
as a stopgap until I thought through what I wanted to replace it with)
is now replaced by fallback handling: we present the host key
fingerprint to the user and give them the option to accept and/or
cache it based on the public key itself.
This means that the certified key types have to have a representation
in the host key cache. So I've assigned each one a type id, and
generate the cache string itself by simply falling back to the base
key.
(Rationale for the latter: re-signing a public key with a different
certificate doesn't change the _private_ key, or the set of valid
signatures generated with it. So if you've been convinced for reasons
other than the certificate that a particular private key is in the
possession of $host, then proof of ownership of that private key
should be enough to convince you you're talking to $host no matter
what CA has signed the public half this week.)
We now offer to receive a given certified host key type if _either_ we
have at least one CA configured to trust that host, _or_ we have a
certified key of that type cached. (So once you've decided manually
that you trust a particular key, we can still receive that key and
authenticate the host with it, even if you later delete the CA record
that it didn't match anyway.)
One change from normal (uncertified) host key handling is that for
certified key types _all_ the host key prompts use the stronger
language, with "WARNING - POTENTIAL SECURITY BREACH!" rather than the
mild 'hmm, we haven't seen this host before'. Rationale: if you
expected this CA key and got that one, it _could_ be a bold-as-brass
MITM attempt in which someone hoped you'd accept their entire CA key.
The mild wording is only for the case where we had no previous
expectations _at all_ for the host to violate: not a CA _or_ a cached
key.
In the macro automation for ssh2_bpp_check_unimplemented, I #defined
SSH2_BITMAP_WORD, and 20 lines later, tried to #undef it by the wrong
spelling. Of course this gave no error, so I didn't notice! But I
spotted it just now, so let's fix it.
The text of the host key warnings was replicated in three places: the
Windows rc file, the GTK dialog setup function, and the console.c
shared between both platforms' CLI tools. Now it lives in just one
place, namely ssh/common.c where the rest of the centralised host-key
checking is done, so it'll be easier to adjust the wording in future.
This comes with some extra automation. Paragraph wrapping is no longer
done by hand in any version of these prompts. (Previously we let GTK
do the wrapping on GTK, but on Windows the resource file contained a
bunch of pre-wrapped LTEXT lines, and console.c had pre-wrapped
terminal messages.) And the dialog heights in Windows are determined
automatically based on the amount of stuff in the window.
The main idea of all this is that it'll be easier to set up more
elaborate kinds of host key prompt that deal with certificates (if,
e.g., a server sends us a certified host key which we don't trust the
CA for). But there are side benefits of this refactoring too: each
tool now reliably inserts its own appname in the prompts, and also, on
Windows the entire prompt text is copy-pastable.
Details of implementation: there's a new type SeatDialogText which
holds a set of (type, string) pairs describing the contents of a
prompt. Type codes distinguish ordinary text paragraphs, paragraphs to
be displayed prominently (like key fingerprints), the extra-bold scary
title at the top of the 'host key changed' version of the dialog, and
the various information that lives in the subsidiary 'more info' box.
ssh/common.c constructs this, and passes it to the Seat to present the
actual prompt.
In order to deal with the different UI for answering the prompt, I've
added an extra Seat method 'prompt_descriptions' which returns some
snippets of text to interpolate into the messages. ssh/common.c calls
that while it's still constructing the text, and incorporates the
resulting snippets into the SeatDialogText.
For the moment, this refactoring only affects the host key prompts.
The warnings about outmoded crypto are still done the old-fashioned
way; they probably ought to be similarly refactored to use this new
SeatDialogText system, but it's not immediately critical for the
purpose I have right now.
This replaces the previous placeholder scheme of having a list of
hostname wildcards with implicit logical-OR semantics (if any wildcard
matched then the certificate would be trusted to sign for that host).
That scheme didn't allow for exceptions within a domain ('everything
in example.com except extra-high-security-machine.example.com'), and
also had no way to specify port numbers.
In the new system, you can still write a hostname wildcard by itself
in the simple case, but now those are just atomic subexpressions in a
boolean-logic domain-specific language I've made up. So if you want
multiple wildcards, you can separate them with || in a single longer
expression, and also you can use && and ! to impose exceptions on top
of that.
Full details of the expression language are in the comment at the top
of utils/cert-expr.c. It'll need documenting properly before release,
of course.
For the sake of backwards compatibility for early adopters who've
already set up configuration in the old system, I've put in some code
that will read the old MatchHosts configuration and automatically
translate it into the equivalent boolean expression (by simply
stringing together the list of wildcards with || between them).
Now we check that we can actually make an ssh_key out of it, and
moreover, that the key is of a sensible kind (i.e. not a certificate
in turn). If that's not true, we report something about the problem in
a new CTRL_TEXT below the public key input box. If the key _is_ valid,
that same text control is used to show its type, length and
fingerprint.
On Windows, I've widened the dialog box a little to make fingerprints
fit sensibly in it.
Now the RSA signature-type checkboxes should be aligned with their
label; the 'Add' and 'Remove' buttons for wildcards should align with
the edit box for entering a wildcard; and the 'Load from file' button
for a public key aligns with the edit box for that.
When we linked a new entry on to the global request queue, we forgot
to set its next pointer to NULL, so that when it was removed again,
s->globreq_head could end up pointing to nonsense.
In addition, even if the next pointer happened to be NULL by luck, we
also did not notice that s->globreq_head had become NULL and respond
by nulling out s->globreq_tail, which would leave s->globreq_tail as a
stale pointer to the just-freed list element, causing a memory access
error on the next attempt to link something on to the list.
This could come up in the situation where you open Change Settings and
configure a remote port forwarding, close it (so that the global
request is sent, queued, replied to, and unqueued again), and then
reopen Change Settings and configure a second one (so that the linked
list in the confused state actually gets used).
Introduced in dc7ba12253 earlier today. On GTK these caused no
problems worse than a GTK warning, but I'd better fix them before they
(potentially) do worse on Windows!
As distinct from the type of signature generated by the SSH server
itself from the host key, this lets you exclude (and by default does
exclude) the old "ssh-rsa" SHA-1 signature type from the signature of
the CA on the certificate.
This means that, on the one hand, an absentminded press of Return
doesn't dismiss the entire CA config box, which would be pretty
annoying if you were half way through entering a load of fiddly stuff.
And on the other hand, you _can_ press Escape to dismiss the box,
which is less likely to happen by accident.
This allows you to load a CA public key from a disk file (in any
format acceptable to ppk_load_pub, which means OpenSSH one-line public
keys and also RFC4716 ones).
In the course of polishing up this dialog box, I'm going to want it to
actually do cryptographic things (such as checking validity of a
public key blob and printing its fingerprint), which means it will
need to link against SSH utility functions.
So I've moved the dialog-box setup and handling code out of config.c
into a new file in the ssh subdirectory and in the ssh library, where
those facilities will be conveniently available.
This also means that dialog-box setup code _won't_ be linked into
PuTTYtel or pterm (on either platform), so I've added a stub source
file to provide its entry-point function in those tools. Also,
provided a const bool to indicate whether that dialog is available,
which we use to decide whether to recognise that command-line option.
In a rekey, we expect to see the same host key again, which we enforce
by comparing its cache string, which we happened to have handy. But
certified host keys don't have cache strings, so this no longer works
reliably - the 'assert(s->keystr)' fails.
(This is what I get for making a zillion short-lived test connections
and not leaving any of them running for more than 2 minutes!)
Instead, we now keep the official public blob of the host key from the
first key exchange, and compare that to the public blob of the one in
the rekey.
It's a thoroughly pedantic point, but I just spotted that the
comparison of wire data against theoretically platform-dependent char
escapes is a violation of \k{udp-portability}.
In commit 7d44e35bb3 I introduced a bug: we were providing an
array of MAXKEXLIST ints to ssh2_scan_kexinits() to write a list of
server-supplied host keys into, and when MAXKEXLIST stopped being a
thing, I mindlessly replaced it with an array dynamically allocated to
the number of host key types we'd offered the server.
But we return a list of host key types the _server_ offered _us_ (and
that we can speak at all), which isn't necessarily the same thing. In
particular, if you deliberately ask to cache a new host key type from
the specials menu, we send a KEXINIT offering just _one_ host key
type, namely the one you've asked for. But that loop still writes down
all the key types it gets back from the server, which is (almost
certainly) more than one. So the array overflows.
In that situation we don't really need the returned array of key types
at all, but it's easier to just make it work than to add conditionals.
Replaced it with a dynamically grown array in the usual sort of way.
Now we offer the OpenSSH certificate key types in our KEXINIT host key
algorithm list, so that if the server has a certificate, they can send
it to us.
There's a new storage.h abstraction for representing a list of trusted
host CAs, and which ones are trusted to certify hosts for what
domains. This is stored outside the normal saved session data, because
the whole point of host certificates is to avoid per-host faffing.
Configuring this set of trusted CAs is done via a new GUI dialog box,
separate from the main PuTTY config box (because it modifies a single
set of settings across all saved sessions), which you can launch by
clicking a button in the 'Host keys' pane. The GUI is pretty crude for
the moment, and very much at a 'just about usable' stage right now. It
will want some polishing.
If we have no CA configured that matches the hostname, we don't offer
to receive certified host keys in the first place. So for existing
users who haven't set any of this up yet, nothing will immediately
change.
Currently, if we do offer to receive certified host keys and the
server presents one signed by a CA we don't trust, PuTTY will bomb out
unconditionally with an error, instead of offering a confirmation box.
That's an unfinished part which I plan to fix before this goes into a
release.
This is triggered by a new config option, or alternatively a -cert
command-line option. You provide a certificate file (i.e. a public key
containing one of the cert key formats), and then, whenever you
authenticate with a private key that matches the public key inside
that certificate, the certificate will be sent to the server in place
of whatever public key it would have used before.
I expect this to be more convenient for some users than the approach
of baking the certificate into a modified version of the PPK file -
especially users who want to use different certificates on the same
key, either in sequence (if a CA continually reissues certificates
with short lifetimes) or in parallel (if different hosts trust
different CAs).
In particular, this substitution is applied consistently, even when
doing authentication via an agent. So if your bare private key is held
in Pageant, you can _still_ specify a detached certificate, and PuTTY
will spot that the key it's picked from Pageant matches that
certificate, and do the same substitution.
The detached certificate also overrides an existing certificate, if
there was one on the public key already.
Previously, the fact that "ssh-rsa" sometimes comes with two subtypes
"rsa-sha2-256" and "rsa-sha2-512" was known to three different parts
of the code - two in userauth and one in transport. Now the knowledge
of what those ids are, which one goes with which signing flags, and
which key types have subtypes at all, is centralised into a method of
the key algorithm, and all those locations just query it.
This will enable the introduction of further key algorithms that have
a parallel upgrade system.
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.
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.
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.
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.
Correcting a source file name in the docs just now reminded me that
I've seen a lot of outdated source file names elsewhere in the code,
due to all the reorganisation since we moved to cmake. Here's a giant
pass of trying to make them all accurate again.
All the seat functions that request an interactive prompt of some kind
to the user - both the main seat_get_userpass_input and the various
confirmation dialogs for things like host keys - were using a simple
int return value, with the general semantics of 0 = "fail", 1 =
"proceed" (and in the case of seat_get_userpass_input, answers to the
prompts were provided), and -1 = "request in progress, wait for a
callback".
In this commit I change all those functions' return types to a new
struct called SeatPromptResult, whose primary field is an enum
replacing those simple integer values.
The main purpose is that the enum has not three but _four_ values: the
"fail" result has been split into 'user abort' and 'software abort'.
The distinction is that a user abort occurs as a result of an
interactive UI action, such as the user clicking 'cancel' in a dialog
box or hitting ^D or ^C at a terminal password prompt - and therefore,
there's no need to display an error message telling the user that the
interactive operation has failed, because the user already knows,
because they _did_ it. 'Software abort' is from any other cause, where
PuTTY is the first to know there was a problem, and has to tell the
user.
We already had this 'user abort' vs 'software abort' distinction in
other parts of the code - the SSH backend has separate termination
functions which protocol layers can call. But we assumed that any
failure from an interactive prompt request fell into the 'user abort'
category, which is not true. A couple of examples: if you configure a
host key fingerprint in your saved session via the SSH > Host keys
pane, and the server presents a host key that doesn't match it, then
verify_ssh_host_key would report that the user had aborted the
connection, and feel no need to tell the user what had gone wrong!
Similarly, if a password provided on the command line was not
accepted, then (after I fixed the semantics of that in the previous
commit) the same wrong handling would occur.
So now, those Seat prompt functions too can communicate whether the
user or the software originated a connection abort. And in the latter
case, we also provide an error message to present to the user. Result:
in those two example cases (and others), error messages should no
longer go missing.
Implementation note: to avoid the hassle of having the error message
in a SeatPromptResult being a dynamically allocated string (and hence,
every recipient of one must always check whether it's non-NULL and
free it on every exit path, plus being careful about copying the
struct around), I've instead arranged that the structure contains a
function pointer and a couple of parameters, so that the string form
of the message can be constructed on demand. That way, the only users
who need to free it are the ones who actually _asked_ for it in the
first place, which is a much smaller set.
(This is one of the rare occasions that I regret not having C++'s
extra features available in this code base - a unique_ptr or
shared_ptr to a string would have been just the thing here, and the
compiler would have done all the hard work for me of remembering where
to insert the frees!)
In SSH-1 there's a function that takes a void * that it casts to the
state of the login layer. The corresponding function in SSH-2 casts it
to the state of a differently named layer, but I had still called the
parameter 'loginv'.
I recently encountered a paper [1] which catalogues all kinds of
things that can go wrong when one party in a discrete-log system
invents a prime and the other party chooses an exponent. In
particular, some choices of prime make it reasonable to use a short
exponent to save time, but others make that strategy very bad.
That paper is about the ElGamal encryption scheme used in OpenPGP,
which is basically integer Diffie-Hellman with one side's key being
persistent: a shared-secret integer is derived exactly as in DH, and
then it's used to communicate a message integer by simply multiplying
the shared secret by the message, mod p.
I don't _know_ that any problem of this kind arises in the SSH usage
of Diffie-Hellman: the standard integer DH groups in SSH are safe
primes, and as far as I know, the usual generation of prime moduli for
DH group exchange also picks safe primes. So the short exponents PuTTY
has been using _should_ be OK.
However, the range of imaginative other possibilities shown in that
paper make me nervous, even so! So I think I'm going to retire the
short exponent strategy, on general principles of overcaution.
This slows down 4096-bit integer DH by about a factor of 3-4 (which
would be worse if it weren't for the modpow speedup in the previous
commit). I think that's OK, because, firstly, computers are a lot
faster these days than when I originally chose to use short exponents,
and secondly, more and more implementations are now switching to
elliptic-curve DH, which is unaffected by this change (and with which
we've always been using maximum-length exponents).
[1] On the (in)security of ElGamal in OpenPGP. Luca De Feo, Bertram
Poettering, Alessandro Sorniotti. https://eprint.iacr.org/2021/923
This seemed like a worthwhile cleanup to do while I was working on
this code anyway. Now all the magic numbers are defined in a header
file by macro names indicating their meaning, and used by both the
SOCKS client code in the proxy subdirectory and the SOCKS server code
in portfwd.c.
marshal.h now provides a macro put_fmt() which allows you to write
arbitrary printf-formatted data to an arbitrary BinarySink.
We already had this facility for strbufs in particular, in the form of
strbuf_catf(). That was able to take advantage of knowing the inner
structure of a strbuf to minimise memory allocation (it would snprintf
directly into the strbuf's existing buffer if possible). For a general
black-box BinarySink we can't do that, so instead we dupvprintf into a
temporary buffer.
For consistency, I've removed strbuf_catf, and converted all uses of
it into the new put_fmt - and I've also added an extra vtable method
in the BinarySink API, so that put_fmt can still use strbuf_catf's
more efficient memory management when talking to a strbuf, and fall
back to the simpler strategy when that's not available.
(TL;DR: to suppress redundant 'Press Return to begin session' prompts
in between hops of a jump-host configuration, in Plink.)
This new query method directly asks the Seat the question: is the same
stream of input used to provide responses to interactive login
prompts, and the session input provided after login concludes?
It's used to suppress the last-ditch anti-spoofing defence in Plink of
interactively asking 'Access granted. Press Return to begin session',
on the basis that any such spoofing attack works by confusing the user
about what's a legit login prompt before the session begins and what's
sent by the server after the main session begins - so if those two
things take input from different places, the user can't be confused.
This doesn't change the existing behaviour of Plink, which was already
suppressing the antispoof prompt in cases where its standard input was
redirected from something other than a terminal. But previously it was
doing it within the can_set_trust_status() seat query, and I've now
moved it out into a separate query function.
The reason why these need to be separate is for SshProxy, which needs
to give an unusual combination of answers when run inside Plink. For
can_set_trust_status(), it needs to return whatever the parent Seat
returns, so that all the login prompts for a string of proxy
connections in session will be antispoofed the same way. But you only
want that final 'Access granted' prompt to happen _once_, after all
the proxy connection setup phases are done, because up until then
you're still in the safe hands of PuTTY itself presenting an unbroken
sequence of legit login prompts (even if they come from a succession
of different servers). Hence, SshProxy unconditionally returns 'no' to
the query of whether it has a single mixed input stream, because
indeed, it never does - for purposes of session input it behaves like
an always-redirected Plink, no matter what kind of real Seat it ends
up sending its pre-session login prompts to.
This is generated when setup of a network connection is cancelled by
deliberate user action, namely, pressing ^C or ^D or the like at a
get_userpass_input prompt presented during proxy setup.
It's handled just like normal socket setup errors, except that it
omits the call to seat_connection_fatal, on the grounds that in this
one case of connection-setup failure, the user doesn't need to be
_informed_ that the connection failed - they already know, because
they failed it themself on purpose.
Passing an operating-system-specific error code to plug_closing(),
such as errno or GetLastError(), was always a bit weird, given that it
generally had to be handled by cross-platform receiving code in
backends. I had the platform.h implementations #define any error
values that the cross-platform code would have to handle specially,
but that's still not a great system, because it also doesn't leave
freedom to invent error representations of my own that don't
correspond to any OS code. (For example, the ones I just removed from
proxy.h.)
So now, the OS error code is gone from the plug_closing API, and in
its place is a custom enumeration of closure types: normal, error, and
the special case BROKEN_PIPE which is the only OS error code we have
so far needed to handle specially. (All others just mean 'abandon the
connection and print the textual message'.)
Having already centralised the handling of OS error codes in the
previous commit, we've now got a convenient place to add any further
type codes for errors needing special handling: each of Unix
plug_closing_errno(), Windows plug_closing_system_error(), and Windows
plug_closing_winsock_error() can easily grow extra special cases if
need be, and each one will only have to live in one place.
Previously, SshProxy dealt with creating a TempSeat to wrap the one it
was borrowing from its client, and then each client in turn dealt with
detecting when it had had its seat borrowed and finishing up with the
TempSeat. The latter involved a lot of code duplication; the former
didn't involve code duplication _yet_ (since SshProxy was the only
thing doing this job), but would have once we started wanting to do
interactive password prompting for other types of network proxy.
Now all of that functionality is centralised into two new Interactor
helper functions: interactor_borrow_seat and interactor_return_seat.
All this Interactor business has been gradually working towards being
able to inform the user _which_ network connection is currently
presenting them with a password prompt (or whatever), in situations
where more than one of them might be, such as an SSH connection being
used as a proxy for another SSH connection when neither one has
one-touch login configured.
At some point, we have to arrange that any attempt to do a user
interaction during connection setup - be it a password prompt, a host
key confirmation dialog, or just displaying an SSH login banner -
makes it clear which host it's come from. That's going to mean calling
some kind of announcement function before doing any of those things.
But there are several of those functions in the Seat API, and calls to
them are scattered far and wide across the SSH backend. (And not even
just there - the Rlogin backend also uses seat_get_userpass_input).
How can we possibly make sure we don't forget a vital call site on
some obscure little-tested code path, and leave the user confused in
just that one case which nobody might notice for years?
Today I thought of a trick to solve that problem. We can use the C
type system to enforce it for us!
The plan is: we invent a new struct type which contains nothing but a
'Seat *'. Then, for every Seat method which does a thing that ought to
be clearly identified as relating to a particular Interactor, we
adjust the API for that function to take the new struct type where it
previously took a plain 'Seat *'. Or rather - doing less violence to
the existing code - we only need to adjust the API of the dispatch
functions inline in putty.h.
How does that help? Because the way you _get_ one of these
struct-wrapped Seat pointers is by calling interactor_announce() on
your Interactor, which will in turn call interactor_get_seat(), and
wrap the returned pointer into one of these structs.
The effect is that whenever the SSH (or Rlogin) code wants to call one
of those particular Seat methods, it _has_ to call
interactor_announce() just beforehand, which (once I finish all of
this) will make sure the user is aware of who is presenting the prompt
or banner or whatever. And you can't forget to call it, because if you
don't call it, then you just don't have a struct of the right type to
give to the Seat method you wanted to call!
(Of course, there's nothing stopping code from _deliberately_ taking a
Seat * it already has and wrapping it into the new struct. In fact
SshProxy has to do that, in order to forward these requests up the
chain of Seats. But the point is that you can't do it _by accident_,
just by forgetting to make a vital function call - when you do that,
you _know_ you're doing it on purpose.)
No functional change: the new interactor_announce() function exists,
and the type-system trick ensures it's called in all the right places,
but it doesn't actually _do_ anything yet.
Thanks to the previous commit, this new parameter can replace two of
the existing ones: instead of passing a LogPolicy and a Seat, we now
pass just an Interactor, from which any proxy implementation can
extract the LogPolicy and the Seat anyway if they need it.
This trait will be implemented by anything that wants to display
interactive prompts or notifications to the user in the course of
setting up a network connection, _or_ anything that wants to make a
network connection whose proxy setup might in turn need to do that.
To begin with, that means every Backend that makes network connections
at all must be an Interactor, because any of those network connections
might be proxied via an SSH jump host which might need to interact
with the user.
I'll fill in the contents of this trait over the next few commits, to
keep the patches comprehensible. For the moment, I've just introduced
the trait, set up implementations of it in the five network backends,
and given it a single 'description' method.
The previous 'description' methods of Backend and Plug are now
removed, and their work is done by the new Interactor method instead.
(I changed my mind since last week about where that should best live.)
This isn't too much of an upheaval, fortunately, because I hadn't got
round yet to committing anything that used those methods!
Previously, SSH authentication banners were displayed by calling the
ordinary seat_output function, and passing it a special value in the
SeatOutputType enumeration indicating an auth banner.
The awkwardness of this was already showing a little in SshProxy's
implementation of seat_output, where it had to check for that special
value and do totally different things for SEAT_OUTPUT_AUTH_BANNER and
everything else. Further work in that area is going to make it more
and more awkward if I keep the two output systems unified.
So let's split them up. Now, Seat has separate output() and banner()
methods, which each implementation can override differently if it
wants to.
All the 'end user' Seat implementations use the centralised
implementation function nullseat_banner_to_stderr(), which turns
banner text straight back into SEAT_OUTPUT_STDERR and passes it on to
seat_output. So I didn't have to tediously implement a boring version
of this function in GTK, Windows GUI, consoles, file transfer etc.
Previously, checking the host key against the persistent cache managed
by the storage.h API was done as part of the seat_verify_ssh_host_key
method, i.e. separately by each Seat.
Now that check is done by verify_ssh_host_key(), which is a new
function in ssh/common.c that centralises all the parts of host key
checking that don't need an interactive prompt. It subsumes the
previous verify_ssh_manual_host_key() that checked against the Conf,
and it does the check against the storage API that each Seat was
previously doing separately. If it can't confirm or definitively
reject the host key by itself, _then_ it calls out to the Seat, once
an interactive prompt is definitely needed.
The main point of doing this is so that when SshProxy forwards a Seat
call from the proxy SSH connection to the primary Seat, it won't print
an announcement of which connection is involved unless it's actually
going to do something interactive. (Not that we're printing those
announcements _yet_ anyway, but this is a piece of groundwork that
works towards doing so.)
But while I'm at it, I've also taken the opportunity to clean things
up a bit by renaming functions sensibly. Previously we had three very
similarly named functions verify_ssh_manual_host_key(), SeatVtable's
'verify_ssh_host_key' method, and verify_host_key() in storage.h. Now
the Seat method is called 'confirm' rather than 'verify' (since its
job is now always to print an interactive prompt, so it looks more
like the other confirm_foo methods), and the storage.h function is
called check_stored_host_key(), which goes better with store_host_key
and avoids having too many functions with similar names. And the
'manual' function is subsumed into the new centralised code, so
there's now just *one* host key function with 'verify' in the name.
Several functions are reindented in this commit. Best viewed with
whitespace changes ignored.
These will typically be implemented by objects that are both a Backend
*and* a Plug, and the two methods will deliver the same results to any
caller, regardless of which facet of the object is known to that
caller.
Their purpose is to deliver a user-oriented natural-language
description of what network connection the object is handling, so that
it can appear in diagnostic messages.
The messages I specifically have in mind are going to appear in cases
where proxies require interactive authentication: when PuTTY prompts
interactively for a password, it will need to explain which *thing*
it's asking for the password for, and these descriptions are what it
will use to describe the thing in question.
Each backend is allowed to compose these messages however it thinks
best. In all cases at present, the description string is constructed
by the new centralised default_description() function, which takes a
host name and port number and combines them with the backend's display
name. But the SSH backend does things a bit differently, because it
uses the _logical_ host name (the one that goes with the SSH host key)
rather than the physical destination of the network connection. That
seems more appropriate when the question it's really helping the user
to answer is "What host am I supposed to be entering the password for?"
In this commit, no clients of the new methods are introduced. I have a
draft implementation of actually using it for the purpose I describe
above, but it needs polishing.
The current 'displayname' field is designed for presenting in the
config UI, so it starts with a capital letter even when it's not a
proper noun. If I want to name the backend in the middle of a
sentence, I'll need a version that starts with lowercase where
appropriate.
The old field is renamed displayname_tc, to avoid ambiguity.
It was totally unused. No implementation of the 'closing' method in a
Plug vtable was checking it for any reason at all, except for
ProxySocket which captured it from its client in order to pass on to
its server (which, perhaps after further iterations of ProxySocket,
would have ended up ignoring it similarly). And every caller of
plug_closing set it to 0 (aka false), except for the one in sshproxy.c
which passed true (but it would have made no difference to anyone).
The comment in network.h refers to a FIXME comment which was in
try_send() when that code was written (see winnet.c in commit
7b0e082700). That FIXME is long gone, replaced by a use of a
toplevel callback. So I think the aim must have been to avoid
re-entrancy when sk_write called try_send which encountered a socket
error and called back to plug_closing - but that's long since fixed by
other means now.
Now you can turn various authentication methods on and off, so that
the server won't even offer (say) k-i or publickey at all.
This subsumes the previous -allow-none-auth option; there's now a
general -{allow,deny}-auth=foo option schema, so -allow-auth=none is
the new spelling of -allow-none-auth. The former spelling is kept for
backwards compatibility, just in case.
It doesn't actually do anything specific to the userauth layer; it's
just a helper function that deals with the mechanics of printing an
unspoofable message on various kinds of front end, and the only
parameters it needs are a Seat and a message.
Currently, it's used for 'here is the start/end of the server banner'
only. But it's also got all the right functionality to be used for the
(still missing) messages about which proxy SSH server the next set of
login prompts are going to refer to.
The jump host system ought really to be treating SSH authentication
banners as a distinct thing from the standard-error session output, so
that the former can be presented to the user in the same way as the
auth banner for the main session.
This change converts the 'bool is_stderr' parameter of seat_output()
into an enumerated type with three values. For the moment, stderr and
banners are treated the same, but the plan is for that to change.
If a userauth layer is destroyed while userpass input is still
ongoing, ssh2_userauth_free forgot to free the active prompts_t,
leaking memory.
But adding the missing free_prompts call to ssh2_userauth_free results
in a double-free, because another thing I forgot was to null out that
pointer field everywhere _else_ it's freed. Fixed that too.
Commit 8f5e9a4f8d introduced a segfault into both Windows Pageant
and Windows connection sharing upstreams when they receive an incoming
named-pipe connection. This occurs because the PlugVtables for those
incoming connections had a null pointer in the 'log' field, because
hitherto, only sockets involved with an outgoing connection expected
to receive plug_log() notifications. But I added such a notification
in make_handle_socket, forgetting that that function is used for both
outgoing and incoming named-pipe connections (among other things). So
now a Plug implementation that expects to be set up by the
plug_accepting() method on a listener may still receive
PLUGLOG_CONNECT_SUCCESS.
I could fix that by adding a parameter to make_handle_socket telling
it whether to send a notification, but that seems like more faff than
is really needed. Simpler to make a rule that says _all_ Socket types
must implement the log() method, even if only with a no-op function.
We already have a no-op implementation of log(), in nullplug.c. So
I've exposed that outside its module (in the same style as all the
nullseat functions and so on), to make it really easy to add log()
methods to PlugVtables that don't need one.
Now that the SSH backend's user_input bufchain is no longer needed for
handling userpass input, it doesn't have to be awkwardly shared
between all the packet protocol layers any more. So we can turn the
want_user_input and got_user_input methods of PacketProtocolLayer into
methods of ConnectionLayer, and then only the two connection layers
have to bother implementing them, or store a pointer to the bufchain
they read from.
The system for handling seat_get_userpass_input has always been
structured differently between GUI PuTTY and CLI tools like Plink.
In the CLI tools, password input is read directly from the OS
terminal/console device by console_get_userpass_input; this means that
you need to ensure the same terminal input data _hasn't_ already been
consumed by the main event loop and sent on to the backend. This is
achieved by the backend_sendok() method, which tells the event loop
when the backend has finished issuing password prompts, and hence,
when it's safe to start passing standard input to backend_send().
But in the GUI tools, input generated by the terminal window has
always been sent straight to backend_send(), regardless of whether
backend_sendok() says it wants it. So the terminal-based
implementation of username and password prompts has to work by
consuming input data that had _already_ been passed to the backend -
hence, any backend that needs to do that must keep its input on a
bufchain, and pass that bufchain to seat_get_userpass_input.
It's awkward that these two totally different systems coexist in the
first place. And now that SSH proxying needs to present interactive
prompts of its own, it's clear which one should win: the CLI style is
the Right Thing. So this change reworks the GUI side of the mechanism
to be more similar: terminal data now goes into a queue in the Ldisc,
and is not sent on to the backend until the backend says it's ready
for it via backend_sendok(). So terminal-based userpass prompts can
now consume data directly from that queue during the connection setup
stage.
As a result, the 'bufchain *' parameter has vanished from all the
userpass_input functions (both the official implementations of the
Seat trait method, and term_get_userpass_input() to which some of
those implementations delegate). The only function that actually used
that bufchain, namely term_get_userpass_input(), now instead reads
from the ldisc's input queue via a couple of new Ldisc functions.
(Not _trivial_ functions, since input buffered by Ldisc can be a
mixture of raw bytes and session specials like SS_EOL! The input queue
inside Ldisc is a bufchain containing a fiddly binary encoding that
can represent an arbitrary interleaving of those things.)
This greatly simplifies the calls to seat_get_userpass_input in
backends, which now don't have to mess about with passing their own
user_input bufchain around, or toggling their want_user_input flag
back and forth to request data to put on to that bufchain.
But the flip side is that now there has to be some _other_ method for
notifying the terminal when there's more input to be consumed during
an interactive prompt, and for notifying the backend when prompt input
has finished so that it can proceed to the next stage of the protocol.
This is done by a pair of extra callbacks: when more data is put on to
Ldisc's input queue, it triggers a call to term_get_userpass_input,
and when term_get_userpass_input finishes, it calls a callback
function provided in the prompts_t.
Therefore, any use of a prompts_t which *might* be asynchronous must
fill in the latter callback when setting up the prompts_t. In SSH, the
callback is centralised into a common PPL helper function, which
reinvokes the same PPL's process_queue coroutine; in rlogin we have to
set it up ourselves.
I'm sorry for this large and sprawling patch: I tried fairly hard to
break it up into individually comprehensible sub-patches, but I just
couldn't tease out any part of it that would stand sensibly alone.
I've introduced a function ldisc_notify_sendok(), which backends
should call on their ldisc (if they have one) when anything changes
that might cause backend_sendok() to start returning true.
At the moment, the function does nothing. But in future, I'm going to
make ldisc start buffering typed-ahead input data not yet sent to the
backend, and then the effect of this function will be to trigger
flushing all that data into the backend.
Backends only have to call this function if sendok was previously
false: backends requiring no network connection stage (like pty and
serial) can safely return true from sendok, and in that case, they
don't also have to immediately call this function.
I did a horrible thing with a list macro which builds up a 256-bit
bitmap of known SSH-2 message types at compile time, by means of
evaluating a conditional expression per known message type and per
bitmap word which boils down to (in pseudocode)
(shift count in range ? 1 << shift count : 0)
I think this is perfectly valid C. If the shift count is out of range,
then the use of the << operator in the true branch of the ?: would
have undefined behaviour if it were executed - but that's OK, because
in that situation, the safe false branch is executed instead.
But when the whole thing is a compile-time evaluated constant
expression, the compiler can prove statically that the << in the true
branch is an out-of-range shift, and at least some compilers will warn
about it verbosely. The same compiler *could* also prove statically
that that branch isn't taken, and use that to suppress the warning -
but at least clang does not.
The solution is the same one I used in shift_right_by_one_word and
shift_left_by_one_word in mpint.c: inside the true branch, nest a
second conditional expression which coerces the shift count to always
be in range, by setting it to 0 if it's not. This doesn't affect the
output, because the only cases in which the output of the true branch
is altered by this transformation are the ones in which the true
branch wasn't taken anyway.
So this change should make no difference to the output of this macro
construction, but it suppresses about 350 pointless warnings from
clang.
This is working towards allowing the subsidiary SSH connection in an
SshProxy to share the main user-facing Seat, so as to be able to pass
through interactive prompts.
This is more difficult than the similar change with LogPolicy, because
Seats are stateful. In particular, the trust-sigil status will need to
be controlled by the SshProxy until it's ready to pass over control to
the main SSH (or whatever) connection.
To make this work, I've introduced a thing called a TempSeat, which is
(yet) another Seat implementation. When a backend hands its Seat to
new_connection(), it does it in a way that allows new_connection() to
borrow it completely, and replace it in the main backend structure
with a TempSeat, which acts as a temporary placeholder. If the main
backend tries to do things like changing trust status or sending
output, the TempSeat will buffer them; later on, when the connection
is established, TempSeat will replay the changes into the real Seat.
So, in each backend, I've made the following changes:
- pass &foo->seat to new_connection, which may overwrite it with a
TempSeat.
- if it has done so (which we can tell via the is_tempseat() query
function), then we have to free the TempSeat and reinstate our main
Seat. The signal that we can do so is the PLUGLOG_CONNECT_SUCCESS
notification, which indicates that SshProxy has finished all its
connection setup work.
- we also have to remember to free the TempSeat if our backend is
disposed of without that having happened (e.g. because the
connection _doesn't_ succeed).
- in backends which have no local auth phase to worry about, ensure
we don't call seat_set_trust_status on the main Seat _before_ it
gets potentially replaced with a TempSeat. Moved some calls of
seat_set_trust_status to just after new_connection(), so that now
the initial trust status setup will go into the TempSeat (if
appropriate) and be buffered until that seat is relinquished.
In all other uses of new_connection, where we don't have a Seat
available at all, we just pass NULL.
This is NFC, because neither new_connection() nor any of its delegates
will _actually_ do this replacement yet. We're just setting up the
framework to enable it to do so in the next commit.
Now new_connection() takes an optional LogPolicy * argument, and
passes it on to the SshProxy setup. This means that SshProxy's
implementation of the LogPolicy trait can answer queries like
askappend() and logging_error() by passing them on to the same
LogPolicy used by the main backend.
Not all callers of new_connection have a LogPolicy, so we still have
to fall back to the previous conservative default behaviour if
SshProxy doesn't have a LogPolicy it can ask.
The main backend implementations didn't _quite_ have access to a
LogPolicy already, but they do have a LogContext, which has a
LogPolicy vtable pointer inside it; so I've added a query function
log_get_policy() which allows them to extract that pointer to pass to
new_connection.
This is the first step of fixing the non-interactivity limitations of
SshProxy. But it's also the easiest step: the next ones will be more
involved.
This is called by the backend to notify the Seat that the connection
has progressed to the point where the main session channel (i.e. the
thing that would typically correspond to the client's stdin/stdout)
has been successfully set up.
The only Seat that implements this method nontrivially is the one in
SshProxy, which uses it as an indication that the proxied connection
to the remote host has succeeded, and sends the
PLUGLOG_CONNECT_SUCCESS notification to its own Plug.
Hence, the only backends that need to implement it at the moment are
the two SSH-shaped backends (SSH proper and bare-connection / psusan).
For other backends, it's not always obvious what 'main session
channel' would even mean, or whether it means anything very useful; so
I've also introduced a backend flag indicating whether the backend is
expecting to call that method at all, so as not to have to spend
pointless effort on defining an arbitrary meaning for it in other
contexts.
So a lot of this patch is just introducing the new method and putting
its trivial do-nothing implementation into all the existing Seat
methods. The interesting parts happen in ssh/mainchan.c (which
actually calls it), and sshproxy.c (which does something useful in
response).
