These are better than my previous approach of just assigning to
sb->len, because firstly they check by assertion that the new length
is within range, and secondly they preserve the invariant that the
byte stored in the buffer just after the length runs out is \0.
Switched to using the new functions everywhere a grep could turn up
opportunities.
(cherry picked from commit 5891142aee)
UBsan points out that if the input pointer is NULL, we'll pass it to
memcpy, which is technically illegal by the C standard _even_ if the
length you pass with it is zero.
(cherry picked from commit 88d5948ead)
In setting up the ECC tests for cmdgen, I noticed that OpenSSH and
PuTTYgen disagree on the bit length to put in a key fingerprint for an
ed25519 key: we think 255, they think 256.
On reflection, I think 255 is more accurate, which is why I bodged
get_fp() in the test suite to ignore that difference when checking our
key fingerprint against OpenSSH's. But having done that, it now seems
silly that if you unnecessarily specify a bit count at ed25519
generation time, cmdgen will insist that it be 256!
255 is now permitted everywhere an ed25519 bit count is input. 256 is
also still allowed for backwards compatibility but 255 is preferred by
the error message if you give any other value.
(cherry picked from commit 187cc8bfcc)
A user reports that if the ^E answerback string is configured to be
empty, then causing the answerback to be sent fails the assertion in
ldisc_send introduced in commit c269dd013.
I thought I'd caught all of the remaining cases of this in commit
4634cd47f, but apparently not.
(cherry picked from commit 43a63019f5)
Well, actually, two new test programs. agenttest.py is the actual
test; it depends on agenttestgen.py which generates a collection of
test private keys, using the newly exposed testcrypt interface to our
key generation code.
In this commit I've also factored out some Python SSH marshalling code
from cryptsuite, and moved it into a module ssh.py which the agent
tests can reuse.
(cherry picked from commit 8c7b0a787f)
This doesn't affect what files are _legal_: the spec said we tolerated
three kinds of line ending, and it still says we tolerate the same
three. But I noticed that we're actually outputting \n by preference,
whereas the spec said we prefer \r\n. I'd rather change the docs than
the code.
(cherry picked from commit cbfd7dadac)
If the user is scrolled back in the scrollback when a screen-swap
takes place, and if we're not configured to reset the scrollback
completely on the grounds that the swap is display activity, then we
should do the same thing we do for other kinds of display activity:
strive to keep the scroll position pointing at the same text. In this
case, that means adjusting term->disptop by the number of virtual
lines added to the scrollback to allow the main screen to be viewed
while the alt screen is active.
This improves the quality of behaviour in that corner case, but more
importantly, it should also fix a case of the dreaded line==NULL
assertion failure, which someone just reported against 0.73 when
exiting tmux (hence, switching away from the alt screen) while
scrolled back in a purely virtual scrollback buffer: the virtual
scrollback lines vanished, but disptop was still set to a negative
value, which made it out of range.
(cherry picked from commit 22453b46da)
A long time ago, in commit 09f86ce7e, I introduced a separate copy of
the saved cursor position (used by the ESC 7 / ESC 8 sequences) for
the main and alternate screens. The idea was to fix mishandling of an
input sequence of the form
ESC 7 (save cursor)
ESC [?47h (switch to alternate screen)
...
ESC 7 ESC 8 (save and restore cursor, while in alternate screen)
...
ESC [?47l (switch back from alternate screen)
ESC 8 (restore cursor, expecting it to match the _first_ ESC 7)
in which, before the fix, the second ESC 7 would overwrite the
position saved by the first one. So the final ESC 8 would restore the
cursor position to wherever it happened to have been saved in the
alternate screen, instead of where it was saved before switching _to_
the alternate screen.
I've recently noticed that the same bug still happens if you use the
alternative escape sequences ESC[?1047h and ESC[?1047l to switch to
the alternate screen, instead of ESC[?47h and ESC[?47l. This is
because that version of the escape sequence sets the internal flag
'keep_cur_pos' in the call to swap_screen, whose job is to arrange
that the actual cursor position doesn't change at the instant of the
switch. But the code that swaps the _saved_ cursor position in and out
is also conditioned on keep_cur_pos, so the 1047 variant of the
screen-swap sequence was bypassing that too, and behaving as if there
was just a single saved cursor position inside and outside the
alternate screen.
I don't know why I did it that way in 2006. It could have been
deliberate for some reason, or it could just have been mindless copy
and paste from the existing cursor-related swap code. But checking
with xterm now, it definitely seems to be wrong: the 1047 screen swap
preserves the _actual_ cursor position across the swap, but still has
independent _saved_ cursor positions in the two screens. So now PuTTY
does the same.
(cherry picked from commit 421a8ca5d9)
The do_select function is called with a boolean parameter indicating
whether we're supposed to start or stop paying attention to network
activity on a given socket. So if we freeze and unfreeze the socket in
mid-session because of backlog, we'll call do_select(s, false) to
freeze it, and do_select(s, true) to unfreeze it.
But the implementation of do_select in the Windows SFTP code predated
the rigorous handling of socket backlogs, so it assumed that
do_select(s, true) would only be called at initialisation time, i.e.
only once, and therefore that it was safe to use that flag as a cue to
set up the Windows event object to associate with socket activity.
Hence, every time the socket was frozen and unfrozen, we would create
a new netevent at unfreeze time, leaking the old one.
I think perhaps part of the reason why that was hard to figure out was
that the boolean parameter was called 'startup' rather than 'enable'.
To make it less confusing the next time I read this code, I've also
renamed it, and while I was at it, adjusted another related comment.
(cherry picked from commit bd5c957e5b)
As explained in the comment in the code, this makes it easier to map
addresses in the log files back to addresses in the code, if the
testsc image is built as a position-independent executable.
(cherry picked from commit 2804789be8)
This makes all the new deferred-decryption business actually _useful_
for the first time: you can now load an encrypted key file and then
get a prompt to decrypt it on first use, without Pageant being in the
low-usability debug mode.
Currently, the option to present runtime prompts is enabled if Pageant
is running with an X display detected, regardless of lifetime mode.
I'm not really sure why that's necessary: by my understanding of the C
standard, it shouldn't be. But my observation is that when compiling
with {Address,Leak} Sanitiser enabled, pageant --askpass can somehow
manage to exit without having actually written the passphrase to its
standard output.
This applies to both server modes ('pageant -E key.ppk [lifetime]')
and client mode ('pageant -a -E key.ppk').
I'm not completely confident that the CLI syntax is actually right
yet, but for the moment, it's enough that it _exists_. Now I don't
have to test the encrypted-key loading via manually mocked-up agent
requests.
Until now, all the functions that have to work in both the Pageant
server and a separate client process have been implemented by having
two code paths for every request, one of which marshals an agent
request and passes it to agent_query_synchronous, and the other just
calls one of the internal functions in the Pageant core.
This is already quite ugly, and it'll only get worse when I start
adding more client requests. So here's a simplification: now, there's
only one code path, and we _always_ marshal a wire-format agent
request. When we're the same process as the Pageant server, we pass it
to the actual message handler and let that decode it again, enforcing
by assertion that it's not an asynchronous operation that's going to
delay.
This patch removes a layer of indentation from many functions in the
Pageant client layer, so it's best viewed with whitespace ignored.
On Windows, due to a copy-paste goof, the message that should have
read "Configuring n stop bits" instead ended with "data bits".
While I'm here, I've arranged that the "1 stop bit" case of that
message is in the singular. And then I've done the same thing again on
Unix, because I noticed that message was unconditionally plural too.
Like other 'utils' modules, the point is that sshutils.c has no
external dependencies, so it's safe to include in a tool without
requiring you to bring in a cascade of other modules you didn't really
want.
Right now I'm only planning to use this change in an out-of-tree
experiment, but it's harmless to commit the change itself here.
Now I've got an enum for PlugLogType, it's easier to add things to it.
We were giving a blow-by-blow account of each connection attempt, and
when it failed, saying what went wrong before we moved on to the next
candidate address, but when one finally succeeded, we never logged
_that_. Now we do.
This is a small cleanup that removes a couple of copies of some boring
stubs, in favour of having just one copy that you can link against.
Unix Pageant can't currently use this, because it's in a precarious
state of _nearly_ having a random number generator: it links against
sshprng but not sshrand, and only uses it for the randomised keypress
acknowledgments in the GUI askpass prompt. But that means it does use
uxnoise, unlike the truly randomness-free tools.
There aren't quite as many of these as there are on Unix, but Windows
Plink and PSFTP still share some suspiciously similar-looking code.
Now they're both clients of wincliloop.c.
Unix Plink, Unix Pageant in server mode, Uppity, and the post-
connection form of PSFTP's command-line reading code all had very
similar loops in them, which run a pollwrapper and mediate between
that, timers, and toplevel callbacks. It's long past time the common
code between all of those became a reusable shared routine.
So, this commit introduces uxcliloop.c, and turns all the previous
copies of basically the same loop into a call to cli_main_loop with
various callback functions to configure the parts that differ.
The sets of poll(2) events that we check in order to return SELECT_R
and SELECT_W overlap: to be precise, they have POLLERR in common. So
if an fd signals POLLERR, then pollwrap_get_fd_rwx will respond by
saying that it has both SELECT_R and SELECT_W available on it - even
if the caller had only asked for one of those.
In other words, you can get a spurious SELECT_W notification on an fd
that you never asked for SELECT_W on in the first place. This
definitely isn't what I'd meant that API to do.
In particular, if a socket in the middle of an asynchronous connect()
signals POLLERR, then Unix Plink will call select_result for it with
SELECT_R and then SELECT_W respectively. The former will notice that
it's got an error condition and call plug_closing - and _then_ the
latter will decide that it's writable and set s->connected! The plan
was to only select it for write until it was connected, but this bug
in pollwrap was defeating that plan.
Now pollwrap_get_fd_rwx should only ever return a set of rwx flags
that's a subset of the one that the client asked for via
pollwrap_add_fd_rwx.
Spotted by Leak Sanitiser, while I was investigating the PSFTP /
proftpd issue mentioned in the previous commit (with ASan on as
usual).
The two very similar loops that read PSFTP commands from the
interactive prompt and a batch file differed in one respect: only one
of them remembered to free the command afterwards. Now I've moved the
freeing code out into a subroutine that both loops can use.
Ever since I reworked the SSH code to have multiple internal packet
queues, there's been a long-standing FIXME in ssh_sendbuffer() saying
that we ought to include the data buffered in those queues as part of
reporting how much data is buffered on standard input.
Recently a user reported that 'proftpd', or rather its 'mod_sftp'
add-on that implements an SFTP-only SSH server, exposes a bug related
to that missing piece of code. The xfer_upload system in sftp.c starts
by pushing SFTP write messages into the SSH code for as long as
sftp_sendbuffer() (which ends up at ssh_sendbuffer()) reports that not
too much data is buffered locally. In fact what happens is that all
those messages end up on the packet queues between SSH protocol
layers, so they're not counted by sftp_sendbuffer(), so we just keep
going until there's some other reason to stop.
Usually the reason we stop is because we've filled up the SFTP
channel's SSH-layer window, so we need the server to send us a
WINDOW_ADJUST before we're allowed to send any more data. So we return
to the main event loop and start waiting for reply packets. And when
the window is moderate (e.g. OpenSSH currently seems to present about
2MB), this isn't really noticeable.
But proftpd presents the maximum-size window of 2^32-1 bytes, and as a
result we just keep shovelling more and more packets into the internal
packet queues until PSFTP has grown to 4GB in size, and only then do
we even return to the event loop and start actually sending them down
the network. Moreover, this happens again at rekey time, because while
a rekey is in progress, ssh2transport stops emptying the queue of
outgoing packets sent by its higher layer - so, again, everything just
keeps buffering up somewhere that sftp_sendbuffer can't see it.
But this commit fixes it! Each PacketProtocolLayer now provides a
vtable method for asking how much data it currently has queued. Most
of them share a default implementation which just returns the newly
added total_size field from their pq_out; the exception is
ssh2transport, which also has to account for data queued in its higher
layer. And ssh_sendbuffer() adds that on to the quantity it already
knew about in other locations, to give a more realistic idea of the
currently buffered data.
The queue-node structure shared between PktIn and PktOut now has a
'formal_size' field, which is initialised appropriately by the various
packet constructors. And the PacketQueue structure has a 'total_size'
field which tracks the sum of the formal sizes of all the packets on
the queue, and is automatically updated by the push, pop and
concatenate functions.
No functional change, and nothing uses the new fields yet: this is
infrastructure that will be used in the next commit.
If the agent client code doesn't even manage to read a full response
message at all (for example, because the agent it's talking to is
Pageant running in debug mode and you just ^Ced it or it crashed,
which is what's been happening to me all afternoon), then previously,
the userauth code would loop back round to the top of the main loop
without having actually sent any request, so the client code would
deadlock waiting for a response to nothing.
I've often found it useful that you can make symlinks to Unix-domain
sockets, and then connect() on the symlink path will redirect to the
original socket.
This commit adds an option to Unix Pageant which will make it symlink
its socket path to a link location of your choice. My initial use case
is when running Pageant in debug mode during development: if you run a
new copy of it every few minutes after making a code change, then it's
annoying to have it change its socket path every time so you have to
keep pasting its setup command into your test shell. Not any more! Now
you can run 'pageant --debug --symlink fixed-location', and then your
test shell can point its SSH_AUTH_SOCK at the fixed location all the
time.
There are very likely other use cases too, but that's the one that
motivated me to add the option.
This adds an extension request to the agent protocol (named in our
private namespace, naturally) which allows you to upload a key file in
the form of a string containing an entire .ppk file. If the key is
encrypted, then Pageant stores it in such a way that it will show up
in the key list, and on the first attempt to sign something with it,
prompt for a passphrase (if it can), decrypt the key, and then answer
the request.
There are a lot of rough edges still to deal with, but this is good
enough to have successfully answered one request, so it's a start.
This is the easiest place to implement _something_ that will work as a
runtime passphrase prompt, which means I get to use it to test the
code I'm about to add to the Pageant core to make use of those
prompts. Once that's working, we can think about adding prompts for
the 'proper' usage modes.
The debug-mode passphrase prompts are implemented by simply reading
from standard input, having emitted a log message mentioning that a
prompt is impending. We put standard input into non-echoing mode, but
otherwise don't print any visible prompt (because standard output will
in general receive further log messages, which would break it anyway).
This is only just good enough for initial testing. In particular, it
won't cope if two prompts are in flight at the same time. But good
enough for initial testing is better than nothing!
This begins to head towards the goal of storing a key file encrypted
in Pageant, and decrypting it on demand via a GUI prompt the first
time a client requests a signature from it. That won't be a facility
available in all situations, so we have to be able to return failure
from the prompt.
More precisely, there are two versions of this API, one in
PageantClient and one in PageantListenerClient: the stream
implementation of PageantClient implements the former API and hands it
off to the latter. Windows Pageant has to directly implement both (but
they will end up funnelling to the same function within winpgnt.c).
NFC: for the moment, the new API functions are never called, and every
implementation of them returns failure.
I don't really know why it was still in cmdgen.c at all. There's no
reason it shouldn't live in its own source file, and keep cmdgen.c for
the actual code of the key generation program!
This reworks the cmdgen main program so that it loads the input file
into a LoadedFile right at the start, and then every time it needs to
do something with the contents, it calls one of the API functions
taking a BinarySource instead of one taking a Filename.
The usefulness of this is that now we can read from things that aren't
regular files, and can't be rewound or reopened. In particular, the
filename "-" is now taken (per the usual convention) to mean standard
input.
So now you can pipe a public or private key file into cmdgen's
standard input and have it do something useful. For example, I was
recently experimenting with the SFTP-only SSH server that comes with
'proftpd', which keeps its authorized_keys file in RFC 4716 format
instead of the OpenSSH one-liner format, and I found I wanted to do
grep 'my-key-comment' ~/.ssh/authorized_keys | puttygen -p -
to quickly get hold of my existing public key to put in that file. But
I had to go via a temporary file to make that work, because puttygen
couldn't read from standard input. Next time, it will be able to!
Like sshpubk.c before it, the loading side of import.c now works by
first slurping the whole input file into a LoadedFile structure, and
then using the BinarySource system to parse the file contents entirely
in memory.
The old API is still present and works the same as ever, but now we
also provide a secondary API that can import a foreign key file from a
BinarySource.
This is rather a superficial conversion: I've replaced all calls to
fgetline() with a local static function bsgetline() which presents
more or less the same interface for a BinarySource: that is, it still
returns a dynamically allocated string containing the line of text, so
that the followup code could change as little as possible. It would be
nice to come back in future and modernise this code to use ptrlens
throughout, saving all the unnecessary allocations.
I'm about to use it in cmdgen for a minor UI improvement. Also, I
expect it to be useful in the Pageant client code sooner or later.
While I'm here, I've also tweaked its UI a little so that it reports a
more precise error, and provided a version that can read from an
already open stdio stream.
In the previous trawl of this, I didn't bother with the statics in
main-program modules, on the grounds that my main aim was to avoid
'library' objects (shared between multiple programs) from polluting
the global namespace. But I think it's worth being more strict after
all, so this commit adds 'static' to a lot more file-scope variables
that aren't needed outside their own module.
Now it's no longer used, we can get rid of it, and better still, get
rid of every #define PUTTY_DO_GLOBALS in the many source files that
previously had them.
We now have no remaining things in header files that switch from being
a declaration to a definition depending on an awkward #define at the
point of including that header. There are still a few mutable
variables with external linkage, but at least now each one is defined
in a specific source file file appropriate to its purpose and context.
The remaining globals as of this commit were:
- 'logctx' and 'term', which never needed to be globals in the first
place, because they were never actually shared between source
files. Now 'term' is just a static in window.c, and 'logctx' is a
static in each of that and winplink.c.
- 'hinst', which still has external linkage, but is now defined
separately in each source file that sets it up (i.e. those with a
WinMain)
- osMajorVersion, osMinorVersion and osPlatformId, whose definitions
now live in winmisc.c alongside the code which sets them up.
(Actually they were defined there all along, it turns out, but
every toolchain I've built with has commoned them together with the
version defined by the GLOBAL in the header.)
- 'hwnd', which nothing was actually _using_ any more after previous
commits, so all this commit had to do was delete it.
