Not that we ever actually _support_ trying to authenticate for any SSH
subprotocol other than "ssh-connection", or any plans to add such
support. But it's inelegant to hardcode it at all when we have it
right there in the successor layer's vtable.
s->done_service_req was set but never read; it's not needed at all in
the current code structure, where the service request has already
happened in an entirely different source file and userauth never has
to track it at all.
If the file is empty, or otherwise fails to start with a recognised
'PuTTY-User-Key-File' header line, we forgot to fill in the error
message before returning failure, leading to a null pointer
dereference.
Commit 733fcca2c introduced named types settings_r and settings_w and
made the per-platform storage abstraction use those in place of 'void
*'. But a lot of the intermediate helper functions in the centralised
settings.c, like gpps() and wmap(), were still taking 'void *'. It
still worked, because of C's implicit casting, but it was avoiding the
type-checking advantages from making that change in the first place.
This is the structure that stores the truncated version of the Event
Log data to be displayed by the GTK Event Log dialog. It persists for
the lifetime of the parent SSH window, so it was deliberate that it
wasn't freed on destruction of the dialog itself, but I also forgot to
free it on destruction of the SSH window. (This will be more important
in multi-connection process architectures like the OS X port, of
course.)
While I'm at it, I'll follow my recent practice by exposing the
structure tag outside gtkdlg.c so that callers can more easily not
confuse it with some other kind of void *.
The variable s->e in ssh2_transport_state should never be freed by
ssh2transport itself, because it's owned by the dh_ctx, so it will be
freed by dh_cleanup.
If we disconnect because the two ends' SSH protocol versions don't
match, ssh_initiate_connection_close triggers a call to the BPP's
handle_output method, and sshverstring's one of those unconditionally
fails an assertion on the basis that nobody should be trying to send
SSH packets at that stage of the connection. In fact this call to
handle_output is only precautionary, and it's unproblematic because
there aren't any packets on the output queue. So the assertion is now
conditional on there actually being an erroneous packet.
If you start up two sharing-enabled PuTTYs to the same host
simultaneously, the one that ends up being the downstream can connect
to the upstream before the upstream has provided a ConnectionLayer to
the sharestate, which means that log_downstream() will dereference
cs->parent->cl->frontend to find its Frontend and fail because cl is
NULL.
Fixed by providing a dummy initial ConnectionLayer containing nothing
but a frontend pointer, which is then replaced by the real one later.
Almost all the call sites were doing a cumbersome dupprintf-use-free
cycle to get a formatted message into an ErrorSocket anyway, so it
seems more sensible to give them an easier way of doing so.
The few call sites that were passing a constant string literal
_shouldn't_ have been - they'll be all the better for adding a
strerror suffix to the message they were previously giving!
Looks as if I introduced this in commit 733fcca2c, where the pointer
returned from enum_settings_start() stopped being the same thing as
the underlying 'DIR *' - I needed to retain a check for the outer
containing structure not being NULL but the DIR * being NULL inside
it.
I ran 'git push' too soon on the last commit; after a bit more thought
I realise that I didn't get the logic quite right in the case where
one direction of the connection negotiates delayed compression and the
other negotiates ordinary or no compression.
For a start, we only need to worry about temporarily delaying outgoing
packets to avoid the race condition if delayed compression applies to
_outgoing_ packets - that can be disabled in the case where delayed
compression is inbound only (though that is admittedly unlikely).
Secondly, that means that detecting USERAUTH_SUCCESS to enable
compression has to happen even if the output blockage wasn't in place.
Thirdly, if we're independently enabling delayed compression in the
two directions, we should only print an Event Log entry for the one we
actually did!
This revised version is probably more robust, although for the moment
all of this is theoretical - I haven't tested against a server
implementing unidirectional delayed compression.
The problem with OpenSSH delayed compression is that the spec has a
race condition. Compression is enabled when the server sends
USERAUTH_SUCCESS. In the server->client direction, that's fine: the
USERAUTH_SUCCESS packet is not itself compressed, and the next packet
in the same direction is. But in the client->server direction, this
specification relies on there being a moment of half-duplex in the
connection: the client can't send any outgoing packet _after_ whatever
userauth packet the USERAUTH_SUCCESS was a response to, and _before_
finding out whether the response is USERAUTH_SUCCESS or something
else. If it emitted, say, an SSH_MSG_IGNORE or initiated a rekey
(perhaps due to a timeout), then that might cross in the network with
USERAUTH_SUCCESS and the server wouldn't be able to know whether to
treat it as compressed.
My previous solution was to note the presence of delayed compression
options in the server KEXINIT, but not to negotiate them in the
initial key exchange. Instead, we conduct the userauth exchange with
compression="none", and then once userauth has concluded, we trigger
an immediate rekey in which we do accept delayed compression methods -
because of course by that time they're no different from the non-
delayed versions. And that means compression is enabled by the
bidirectional NEWKEYS exchange, which lacks that race condition.
I think OpenSSH itself gets away with this because its layer structure
is structure so as to never send any such asynchronous transport-layer
message in the middle of userauth. Ours is not. But my cunning plan is
that now that my BPP abstraction includes a queue of packets to be
sent and a callback that processes that queue on to the output raw
data bufchain, it's possible to make that callback terminate early, to
leave any dangerous transport-layer messages unsent while we wait for
a userauth response.
Specifically: if we've negotiated a delayed compression method and not
yet seen USERAUTH_SUCCESS, then ssh2_bpp_handle_output will emit all
packets from its queue up to and including the last one in the
userauth type-code range, and keep back any further ones. The idea is
that _if_ that last userauth message was one that might provoke
USERAUTH_SUCCESS, we don't want to send any difficult things after it;
if it's not (e.g. it's in the middle of some ongoing userauth process
like k-i or GSS) then the userauth layer will know that, and will emit
some further userauth packet on its own initiative which will clue us
in that it's OK to release everything up to and including that one.
(So in particular it wasn't even necessary to forbid _all_ transport-
layer packets during userauth. I could have done that by reordering
the output queue - packets in that queue haven't been assigned their
sequence numbers yet, so that would have been safe - but it's more
elegant not to have to.)
One particular case we do have to be careful about is not trying to
initiate a _rekey_ during userauth, if delayed compression is in the
offing. That's because when we start rekeying, ssh2transport stops
sending any higher-layer packets at all, to discourage servers from
trying to ignore the KEXINIT and press on regardless - you don't get
your higher-layer replies until you actually respond to the
lower-layer interrupt. But in this case, if ssh2transport sent a
KEXINIT, which ssh2bpp kept back in the queue to avoid a delayed
compression race and would only send if another userauth packet
followed it, which ssh2transport would never pass on to ssh2bpp's
output queue, there'd be a complete protocol deadlock. So instead I
defer any attempt to start a rekey until after userauth finishes
(using the existing system for starting a deferred rekey at that
moment, which was previously used for the _old_ delayed-compression
strategy, and still has to be here anyway for GSSAPI purposes).
The sshverstring quasi-frontend is passed a Frontend pointer at setup
time, so that it can generate Event Log entries containing the local
and remote version strings and the results of remote bug detection.
I'm promoting that field of sshverstring to a field of the public BPP
structure, so now all BPPs have the right to talk directly to the
frontend if they want to. This means I can move all the log messages
of the form 'Initialised so-and-so cipher/MAC/compression' down into
the BPPs themselves, where they can live exactly alongside the actual
initialisation of those primitives.
It also means BPPs will be able to log interesting things they detect
at any point in the packet stream, which is about to come in useful
for another purpose.
I haven't needed these until now, but I'm about to need to inspect the
entire contents of a packet queue before deciding whether to process
the first item on it.
I've changed the single 'vtable method' in packet queues from get(),
which returned the head of the queue and optionally popped it, to
after() which does the same bug returns the item after a specified
tree node. So if you pass the special end node to after(), then it
behaves like get(), but now you can also use it to retrieve the
successor of a packet.
(Orthogonality says that you can also _pop_ the successor of a packet
by calling after() with prev != pq.end and pop == TRUE. I don't have a
use for that one yet.)
These are things where no fix was actually necessary in the code, but
the FIXME indicated that the comment itself was either in need of a
rewrite or removal.
There was a while when I hadn't decided what the name of the program
was going to be, and apparently once I did I never got round to
substituting it back in everywhere.
It's not used anywhere, but this would make it one step easier to add
a mode argument to PSFTP's mkdir command, if anyone needs it. Mostly
the point is to get rid of the FIXME comment in fxp_mkdir_send itself.
While grepping for FIXME comments I could get rid of easily, I came
across a completely unexplained one in puttytel.rc, and after a moment
of thought, realised that it was there because PuTTYtel sharing
PuTTY's manifest file means the manifest has the wrong application
name.
Of course I could do something a bit more clever involving having one
copy of the manifest file and templating it to multiple applications,
but I think it would be more pain than it's worth given that the
templating system would have to be compatible with all the makefiles
and run on Windows systems where no sensible scripting was available.
So I just do it the trivial way.
It's never set to anything but NULL at any call site, and there's been
a FIXME comment in uxucs.c for ages saying it should be removed. I
think it only existed in the first place because it was a facility
supported by the underlying Windows API function and we couldn't see a
reason _not_ to pass it through. But I'm cleaning up FIXMEs, so we
should get rid of it.
(It stood for 'default used', incidentally - as in 'did the function
at any point have to make use of the parameter providing a default
fallback character?'. Nothing to do with _defusing_ things :-)
Ian Jackson points out that the Linux kernel has a macro of this name
with the same purpose, and suggests that it's a good idea to use the
same name as they do, so that at least some people reading one code
base might recognise it from the other.
I never really thought very hard about what order FROMFIELD's
parameters should go in, and therefore I'm pleasantly surprised to
find that my order agrees with the kernel's, so I don't have to
permute every call site as part of making this change :-)
I don't actually know why this was ever here; it appeared in the very
first commit that invented Plug in the first place (7b0e08270) without
explanation. Perhaps Dave's original idea was that sometimes you'd
need those macros _not_ to be defined so that the same names could be
reused as the methods for a particular Plug instance? But I don't
think that ever actually happened, and the code base builds just fine
with those macros defined unconditionally just like all the other sets
of method macros we now have, so let's get rid of this piece of cruft
that was apparently unnecessary all along.
I think that means that _every_ one of my traitoids is now a struct
containing a vtable pointer as one of its fields (albeit sometimes the
only field), and never just a bare pointer.
Now that I'm doing that in so many of the new classes as a more
type-safe alternative to ordinary C casts, I should make sure all the
old code is also reaping the benefits. This commit converts the system
of unifont vtables in the GTK front end, and also the 'unifontsel'
structure that exposes only a few of its fields outside gtkfont.c.
All the main backend structures - Ssh, Telnet, Pty, Serial etc - now
describe structure types themselves rather than pointers to them. The
same goes for the codebase-wide trait types Socket and Plug, and the
supporting types SockAddr and Pinger.
All those things that were typedefed as pointers are older types; the
newer ones have the explicit * at the point of use, because that's
what I now seem to be preferring. But whichever one of those is
better, inconsistently using a mixture of the two styles is worse, so
let's make everything consistent.
A few types are still implicitly pointers, such as Bignum and some of
the GSSAPI types; generally this is either because they have to be
void *, or because they're typedefed differently on different
platforms and aren't always pointers at all. Can't be helped. But I've
got rid of the main ones, at least.
In mainchan_send_eof, which is the Channel method that gets called
when EOF has been received from the SSH server and is now being passed
on to the local endpoint, we decide whether or not to respond to the
server-side EOF with a client-side EOF based on application
preference. But I was doing the followup admin _outside_ that if
statement, so if the server sent EOF and we _didn't_ want to send EOF
in response, we still set the flag that said we'd sent it, and stopped
reading from standard input. Result: if you use 'plink -nc' to talk to
a remote network socket, and the server sends EOF first, Plink will
never send EOF in the other direction, because it'll stop reading from
standard input and never actually see the EOF that needs to be sent.
s->want_user_input is set and unset in response to fluctuations of the
main channel's available SSH window size. But that means it can become
TRUE before a command has been successfully started, which we don't
want, because pscp.c uses backend_sendok() to determine when it's safe
to check the flag that tells it whether to speak the SFTP or SCP1
protocol. So we want to ensure we never return true from that backend
method until we know which command we're running.
GUI feedback mode was last seen in 2006 (removed in commit 33b7caa59),
so quite what a conditioned-out piece of online help text for it was
doing still around here 12 years later, I have no idea.
(Especially since it had been under #if 0 since 2001, and also since
then its containing source file had ceased to be Windows-only so it
would have been extra-wrong to reinstate it.)
Another mistake in commit 54b300f15 was to introduce a new flag
'progress_bar_displayed', when in fact we were already storing an
indication of whether a set of live transfer statistics were currently
on the display, in the form of prev_stats_len (which is also used to
make sure each stats display overwrites all of the previous one).
Removed that redundancy, and while I'm at it, renamed the new
abandon_progress_bar() to match the rest of the code's general
convention of calling that status display 'statistics' or 'transfer
statistics' rather than a 'progress bar'.
In commit 54b300f15, I managed to set the progress_bar_displayed flag
just _after_, rather than before, the call to abandon_progress_bar
that moves to the new line once the file has finished copying. So in
the case where a file is so small that the very first displaying of
the transfer statistics is already at 100% completion, the flag
wouldn't be set when abandon_progress_bar checked for it, and a
newline still wouldn't be printed.
When I made the 'overwrite or append log file?' dialog box into a
non-modal one, it exposed a bug in logging.c's handling of an
asynchronous response to askappend(): we queued all the pending log
data and wrote it out to the log file, but forgot the final fflush
that would have made sure it all actually _went_ to the log file. So
one stdio buffer's worth could still be held in the C library, to be
released the next time log data shows up.
Added the missing logflush().
A second bug in the area of clean SSH-connection closure: I was
setting the pending_close flag (formerly send_outgoing_eof) and
expecting that once the outgoing backlog was cleared it would cause a
socket closure. But of course the function that does that -
ssh_bpp_output_raw_data_callback() - will only get called if there
_is_ any outgoing backlog to be cleared! So if there was already no
backlog, I would set the pending_close flag and nothing would ever
check it again.
Fixed by manually re-queuing the callback that will check the backlog
and the pending_close flag.
When PuTTY wants to cleanly close an SSH connection, my policy has
been to use shutdown(SHUT_WR) (or rather, sk_write_eof, which ends up
translating into that) to close just the outgoing side of the TCP
connection, and then wait for the server to acknowledge that by
closing its own end.
Mainly the purpose of doing this rather than just immediately closing
the whole socket was that I wanted to make sure any remaining outgoing
packets of ours got sent before the connection was wound up. In
particular, when we send SSH_MSG_DISCONNECT immediately before the
close, we do want that to get through.
But I now think this was a mistake, because it puts us at the mercy of
the server remembering to respond by closing the other direction of
the connection. It might absent-mindedly just continue to sit there
holding the connection open, which would be silly, but if it did
happen, we wouldn't want to sit around waiting in order to close the
client application - we'd rather abandon a socket in that state and
leave it to the OS's network layer to tell the server how silly it was
being.
So now I'm using an in-between strategy: I still wait for outgoing
data to be sent before closing the socket (so the DISCONNECT should
still go out), but once it's gone, I _do_ just close the whole thing
instead of just sending outgoing EOF.
I left this message type code out of the list in the outer switch in
ssh2_connection_filter_queue for messages with the standard handling
of an initial recipient channel id. The inner switch had a perfectly
good handler for extended data, but the outer one didn't pass the
message on to that handler, so it went back to the main coroutine and
triggered a sw_abort for an unexpected packet.
The check_termination function in ssh2connection is supposed to be
called whenever it's possible that we've run out of (a) channels, and
(b) sharing downstreams. I've been calling it on every channel close,
but apparently completely forgot to add a callback from sshshare.c
that also arranges to call it when we run out of downstreams.
Otherwise we loop round repeatedly with the event loop continuing to
report the same EOF condition on them over and over again, consuming
CPU pointlessly and probably causing other knock-on trouble too.
Without this, we don't receive EOF notifications on pipes, because gtk
uses poll rather than select, which separates those out into distinct
event types.
If you call plug_closing directly from localproxy_try_send, which can
in turn be called directly from sk_write, then the plug's
implementation of plug_closing may well free things that the caller of
sk_write expected not to have vanished.
The corresponding routine in uxnet.c pushes that call to plug_closing
into a toplevel callback, so let's do that here too.
When any BPP calls ssh_remote_error or ssh_remote_eof, it triggers an
immediate cleanup of the BPP itself - so on return from one of those
functions we should avoid going straight to the crFinish macro,
because that will write to s->crState, which no longer exists.
I carefully put a flag in the new Ssh structure so that I could tell
the difference between ssh->base_layer being NULL because it hasn't
been set up yet, and being NULL because it's been and gone and the
session is terminated. And did I check that flag in all the error
routines? I did not. Result: an early socket error, while we're still
in the verstring BPP, doesn't get reported as an error message and
doesn't cause the socket to be cleaned up.
It is useful to be able to exclude the header so that the log file
can be used for realtime input to other programs such as Kst for
plotting live data from sensors.
The call to ssh2_censor_packet for incoming packets in ssh2bpp was
passing the wrong starting position in the packet data - in
particular, not the same starting position as the adjacent call to
log_packet - so the censor couldn't parse SSH2_MSG_CHANNEL_DATA to
identify the string of session data that it should be bleeping out.
In commit 8cb68390e I managed to copy the packet contexts inaccurately
from the old implementation of ssh2_pkt_type, and listed the ECDH KEX
packets against SSH2_PKTCTX_DHGEX instead of SSH2_PKTCTX_ECDHKEX,
which led to them appearing as "unknown" in packet log files.
