Handle SOS and PM escape sequences like APC and improve OSC parsing#93
Merged
Conversation
SOS (Start of string) and PM (privacy message) are opening delimiters defined in ECMA-48 similar to APC. With this change they are treated exactly like APC, i.e., like fake OSC sequences that are ignored and not printed. Signed-off-by: Stefan Tauner <stefan.tauner@artech.at>
I didn't like the previous patch setting a flag claiming that two kinds of thing were APC which aren't in fact APC. So I've made a little enum to distinguish them in the code. There's an outside chance we might want to handle some case of these in future, in which case this makes it easier, but more likely it's just making me feel less wrong about it. But I've also folded the two existing flags osc_is_apc and osc_w into that single enum field, which I think is an improvement.
The cmake script that determines the current git head commit, in order
to bake it into the binaries for development builds from a git
checkout, wasn't working reliably on Windows: sometimes it reported
that the source directory didn't seem to be a git repository, when in
fact it was.
This occurred because gitcommit.cmake starts by trying to work out
_whether_ the source directory is the top level of a git worktree at
all, by the technique of running `git rev-parse --show-toplevel` (to
print the top-level path of the git worktree _containing_ $PWD, if
any) and comparing it with ${CMAKE_SOURCE_DIR}. But the comparison was
done as a plain string, which leads to problems if more than one
string can represent the same actual directory.
On Windows, this can occur for two reasons that I know of. One reason
is related to Windows itself: if you map a network file server path to
a local drive letter, then the same directory is accessible as a UNC
path (e.g. \\hostname\share\subdir) and via the drive letter (e.g.
N:\subdir). And it can happen that CMAKE_SOURCE_DIR and git's output
disagree on which representation to use, causing the string comparison
to return a false negative.
(This can also affect filesystems in a WSL instance, accessed from
native Windows via \\wsl$\instance\path, because Windows implements
that as a network file share even though the network in question is
purely in the imagination of that one machine.)
The other reason is related more specifically to git, because some
versions of Windows git are built on top of MSYS or MINGW or that kind
of shim layer, and model Windows drive letters as subdirectories of a
Unixlike VFS root. So you might also find that the two strings
disagree on whether you're in C:\Users\alice\src\putty or
/c/Users/alice/src/putty.
I think this commit should work around both of these problems. Reading
the man page for `git rev-parse` more carefully, it has an option
`--show-cdup`, which returns a _relative_ path from $PWD to the
top-level directory of the worktree: that is, it will be a sequence of
`../../..` as long as necessary, including length zero. So you can use
that to directly query whether you're at the top of a git worktree: if
`git rev-parse --show-cdup` returns the empty string and a success
status, then you are. (If you're deeper in a worktree it will return a
non-empty string, and if you're not in a worktree at all it will
return a failure status and print an error message to stderr.)
Introduced in f8e1a2b.
If the user's choice of fonts can't be instantiated during initial terminal-window setup, then we now automatically try two fallback options, "client:Monospace 10" and "server:fixed", and only give a fatal error if _no_ option allows us to open a terminal window. Previously, on Wayland, PuTTY and pterm with default configuration would completely fail to open a terminal window at all, because our default font configuration is still the trad X11 "server:fixed", and on Wayland, X11-style server-side bitmap fonts don't exist at all. Conversely, in the GTK1 build of PuTTY which we're still supporting, _client-side_ fonts aren't supported, so if a user had configured one in their normal PuTTY configuration, then the GTK1 version would similarly fail to launch. Now both of those cases should work, because the fallbacks include a client-side font _and_ a server-side one, and I hope that any usable Pango system will make "Monospace" map to _some_ locally available font, and that any remotely sensible X server has 'fixed' I think it would be even better if there was a mechanism for the Conf to specify a fallback list of fonts. For example, this might be specified via a new multifont prefix along the lines of choices:client:Monospace 10:server:fixed with the semantics that the "choices:" prefix means that the rest of the string is split up at every other colon to find a list of fonts to try to make. Then we could not only set PuTTY's default to a list of possibilities likely to find a usable font everywhere, but also, users could configure their own list of preferred fallbacks. But I haven't thought of a good answer to the design question of what should happen if a Conf font setting looks like that and the user triggers the GUI font selector! (Also, you'd need to figure out what happened if a 'choices:' string had another 'choices' in it...)
I'd forgotten that I'd already chosen a default client-side font, for NOT_X_WINDOWS builds. I should have made the two defaults match! Now both default font names are defined in the header file.
"server:fixed" was a good default when GTK1 was common and non-X11 environments were rare. Now it's the other way round - Wayland is very common and the GTK1 configuration of PuTTY is legacy - so it's time to make the default GTK font a client-side one. Of course, anyone with an existing saved session (including Default Settings) won't be affected by this change; it only helps new users without an existing ~/.putty at all. That's why we _also_ need the fallbacks introduced by the previous couple of commits. But we can at least start making it sensible for new users. (I considered keeping the #if, and switching it round so that it tests GTK_CHECK_VERSION(2,0,0) rather than NOT_X_WINDOWS, i.e. selects the client-side default whenever client-side fonts _are_ available, instead of only when server-side fonts _aren't_. That way, in GTK1 builds, the Conf default font would _still_ be "server:fixed". But I think this is firstly too marginal to worry about, and secondly, it's more futureproof to make the default the same everywhere: if anyone still stuck on a GTK1 environment later manages to update it, then their saved settings are less likely to have had a legacy thing written into them. And the GTK1 build will still run out of the box because of the last-ditch fallback mechanism I've just added.)
This rewrite, due to SATO Kentaro, uses GetWindowTextLength (which I hadn't known about) to find the correct size to allocate the buffer the first time, avoiding the need to keep growing it until a call to GetDlgItemText doesn't have to truncate the result.
This was introduced in commit e7acb9f, as a side effect of also wanting to call wmemchr to find a NUL wide character in the buffer returned from GetDlgItemTextW. But the previous commit has superseded that code, so now we don't use wmemchr in this code base any more. Remove the machinery that provides it, saving a now-useless cmake configure-time check.
After all these years, this checklist is _still_ hard for me to get right. In the 0.83 runup this month, I prepared everything about the RC build in advance, but nothing about the announcements, website updates etc, and had to do all of that on release day. So I've completely removed the section "Preparing to make the release", which was ambiguous about whether it's done in advance or on the day. Now all the text parts (website, wishlist, announcements) are folded into the "make a release candidate" section, in the hope that I'll remember to do them all at the same time, which should mean - people have a few days to review the text _and_ test the RC build - because they go together, I also remember to revise the text if a new RC build is needed (e.g. mention whatever extra fix it has). The "actual release procedure" section is now down to _only_ the things I have to do on the day, which is basically uploading everything, going live, and communicating the release.
These options apply to many PuTTY tools, not just Plink; the indexing was wonky; and -noshare wasn't documented at all.
(That's Halibut's non-breaking hyphen.) Triggered by noticing that the changes in 54f6fef happened to come out badly in the text-only rendering, but I noticed there were many more instances in the main docs where non-breaking hyphens would help.
We used to have a practice of \IM-ing every command-line option for the index, but haven't kept it up. Add these for all existing indexed command-line options, plus some related tidying.
In PuTTYgen and Pageant.
I stumbled over this paragraph on a re-read.
Some of our words weren't updated when the less-frequently used protocols like 'Raw' were moved to a drop-down.
Colin Watson reported that a build failure occurred in the AArch64 Debian build of PuTTY 0.83: gcc now defaults to enabling branch protection using AArch64 pointer authentication, if the target architecture version supports it. Debian's base supported architecture does not, but Armv8.4-A does. So when I changed the compile flags for enable_dit.c to add -march=armv8.4-a, it didn't _just_ allow me to write the 'msr dit, %0' instruction in my asm statement; it also unexpectedly turned on pointer authentication in the containing function, which caused a SIGILL when running on a pre-Armv8.4-A CPU, because although the code correctly skipped the instruction that set DIT, it was already inside enable_dit() at that point and couldn't avoid going through the unsupported 'retaa' instruction which tries to check an auth code on the return address. An obvious approach would be to add -mbranch-protection=none to the compile flags for enable_dit.c. Another approach is to leave the _compiler_ flags alone, and change the architecture in the assembler, either via a fiddly -Wa,... option or by putting a .arch directive inside the asm statement. But both have downsides. Turning off branch protection is fine for the Debian build, but has the unwanted side effect of turning it off (in that one function) even in builds targeting a later architecture which _did_ want branch protection. And changing the assembler's architecture risks changing it _down_ instead of up, again perhaps invalidating other instructions generated by the compiler (like if some later security feature is introduced that gcc also wants to turn on by default). So instead I've taken the much simpler approach of not bothering to change the target architecture at all, and instead generating the move into DIT by hardcoding its actual instruction encoding. This meant I also had to force the input value into a specific register, but I don't think that does any harm (not _even_ wasting an extra instruction in codegen). Now we should avoid interfering with any security features the compiler wants to turn on or off: all of that should be independent of the instruction I really wanted.
A user reported recently that if you connect to a Telnet server via a proxy that requires authentication, and enter the auth details manually in the PuTTY terminal window, then the entire Telnet session is shown with trust sigils to its left. This happens because telnet.c calls seat_set_trust_status(false) as soon as it's called new_connection() to make the Socket. But the interactive proxy authentication dialogue hasn't happened yet, at that point. So the proxy resets the trust status to true and asks for a username and password, and then nothing ever resets it to false, because telnet.c thought it had already done that. The solution is to defer the Telnet backend's change of trust status to when we get the notification that the socket is properly connected, which arrives via plug_log(PLUGLOG_CONNECT_SUCCESS). The same bug occurs in raw.c and supdup.c, but not in rlogin.c, because Rlogin has an initial authentication exchange known to the protocol, and already delays resetting the trust status until after that has concluded.
This is a convenient thing to look at in a web browser, via a file:// URL, which lets me see all the icons together and check they match.
We weren't building _all_ the icons in true-colour mode, because most don't change anyway. The installer ones do, so let's build them. Works better with the preview page.
It looked nasty that the back corner of the monitor didn't line up exactly with the outline of the system box behind it. Now I choose the y offset between the two components to ensure it does. Also adjusted the monitor's depth so that it fits better with the new alignment.
Jacob points out that Unix PuTTY contained no code to create this directory, in which records are stored for each CA you trust to sign host keys. That's embarassing! I must have created the directory manually during initial development, probably because I wrote and tested the code that loads and uses its contents before the GUI configurer. And then I never re-tested from a clean config directory, which would have made me realise that saving didn't work if the containing directory didn't already exist. (I know at least a few people have been _using_ PuTTY's certificate support since it was added – but apparently not many on Unix!)
Every piece of code that sets term->termstate to SEEN_OSC also sets term->osc_type to identify which kind of generally OSC-shaped escape sequence is being processed, such as DCS or APC or OSC proper. But the latter setting was having no effect, because immediately on arriving in the SEEN_OSC case handler, we would unconditionally reset term->osc_type back to OSCLIKE_OSC, treating them all as OSC anyway! Reported by a user, who was finding that vim was sending the DSC sequence ESC P z z ESC \ on startup, and this was resetting the window title to "z". PuTTY's _intended_ handling of DSC sequences is to consume them without messing up the display, and otherwise ignore them. It had failed to ignore this one! Added a small test of window-title setting to test_terminal.c, mostly so that it can contain a regression test for that bug.
The 'utils' version of the function takes the format string as a parameter rather than extracting it from CONF_proxy_telnet_command, so that it can be reused to format things not kept in that slot.
Patch due to Jonathan Senkerik. This adds a new config feature to spawn a subprocess, on both of Windows and Unix, just before making the main outgoing network connection of the session. The idea is that you'd use it to run a tool that temporarily opens the network port you plan to connect to, such as a port knocker or similar. The subprocess is run synchronously: PuTTY waits for it to terminate before continuing with the session. Its output and exit status are logged in the Event Log, although if it fails, PuTTY will proceed with the connection attempt anyway. (You could imagine that a port knocker might leave the port open for long enough that a second connection shortly afterwards might succeed even if the knock command failed for some reason.)
Document %host and %port escapes. Document -preconnectcommand command-line option. Index 'port knocking'.
The integer s in an EdDSA signature is treated as an exponent: the curve's base point is raised to that power. (OK, multiplied by it, if you use the elliptic curve notational convention rather than the general group convention.) Therefore, in principle, it doesn't make any difference if s varies by a multiple of the base point's order (which is around 2^252, therefore a larger s still fits easily within the 256-bit space for it in the signature encoding). However, RFC 8032 requires s to be strictly less than that order, so that there's a single canonical encoding for any given signature. I'm not treating this as a vulnerability because I don't believe there's any situation in SSH where canonicality of signatures is important. But it should be fixed, nonetheless. In the fix, it's OK to use an ordinary if statement to check the bound on s, because they're visible to everybody anyway: the integer s is encoded directly in the signature, and the bound we're checking it against is a well-known public integer, so nothing new is revealed by any timing side channel proving that that was the reason for the rejection. (Not even if the message being signed were secret, which it is in SSH: the validation of s doesn't depend on the message.) Thanks to Yujie Zhu for the report.
Now there's a function new_main_connection() in proxy/newmain.c, which is called in place of new_connection() when the connection you're making is the primary one of the session (as opposed to some sub-thing like a port forwarding). So now running the pre-connect command is done inside there. This is more DRY in general, but more specifically, allows new_main_connection() to be implemented in a more sophisticated way. The new function differs in API from new_connection() in that it requires a LogContext as well as all the other parameters.
There's a new 'SubprocessWaiter' object, which you can optionally ask for as an extra result of platform_start_subprocess, and then tell it how to give you a callback when the subprocess terminates. Not yet used: the existing use of platform_start_subprocess only cared about the output pipe of the subprocess anyway, so it passes NULL as the extra output pointer, indicating that it doesn't need a waiter.
If a HandleSocket is used with a pair of pipes, we can send outgoing EOF by closing the output pipe, without necessarily closing the input pipe. But we were unconditionally closing both anyway. This makes no difference to usages involving named pipes; it only makes a difference with local proxy subprocesses.
This fixes the limitation of the previous execute_command_hook() that it blocked the whole PuTTY process while waiting for the pre-connect command to terminate. Now, if the subcommand unexpectedly takes a long time, PuTTY itself will still be responsive – you can check its Event Log to see what's going on, or close it if you realise the command is never going to succeed, or simply copy-paste stuff from the terminal window while you wait. platform_start_subprocess opens three pipes to run the command in (on both Unix and Windows), for stdin, stdout and stderr. Here, stdin is a liability: we don't have any input to give the command, so if it were to stop to try to read from stdin, it would blockk. Therefore we send an outgoing EOF immediately by closoing the pipe to the command's stdin, so that instead of blocking, it will immediately find out there's no input for it.
pty_real_select_result was a monolithic function which included handling the result of waitpid() (indicated by passing magic -1 values as fd and event), or actually reading data from the pty (in the absence of those special values), and cleanup functionality if either of those things indicated that it was time to finish the session. Now the magic values are gone: waitpid() results are handled in their own function. Also the shared cleanup functionality lives in the new pty_finished() routine, called from both that and the remainder of pty_real_select_result(), which now just handles _actual_ results of select() reporting activity on the pty fd. This patch is completely boring: it just moves existing code around and reindents it. The interesting change will come in the next commit; this one just separates the boring parts of the work, to make the interesting patch easier to read.
The mechanisms in there - SIGCHLD handler, self-pipe with uxsel, wait loop, tree of known subprocesses indexed by pid - are all replicated in the new centralised system, so there's no need to have two copies of them. This also extends the Unix-specific API of SubprocessWaiter to add a couple of extra 'forcibly do a thing you would have got round to later' functions.
Now you can see the end of a long message without having to paste it into another application. As mentioned in the comment, this doesn't seem to be setting exactly the right scrollbar extent, but I don't know why not. Still, it's overestimating rather than underestimating, which is the better kind of error (you can still read all the text!), so this is better than nothing.
A test-build on current Debian sid revealed that in the latest version of GdkPixbuf, gdk_pixbuf_new_from_xpm_data() is marked as deprecated, causing build failures at -Werror. It turns out that ever since the start of GTK 2, GdkPixbuf has had a reasonably nice API call gdk_pixbuf_new_from_data() which takes a buffer of raw 32-bit RGBA pixels plus width/height/stride parameters, and moreover, uses the data in place rather than copying it – it takes ownership, but you provide a function to free it. Or if it's in the static data segment, you just don't _even_ provide a free function. Not only that, but it turns out the XPM parsing was done by a spare shared library that's not embedded in the GdkPixbuf main code, and a warning in the current docs say that that library can't be 100% relied on to be present everywhere! So using raw RGBA data seems better all round in any case, and since it isn't newly introduced, I don't need to make it conditional on a very new version of anything. We can just use that function for all GTK 2.x and 3.x builds, and use the XPM approach only on GTK 1.
While testing the GTK 1 version of the code I touched in the previous commit, I tried to connect to an SSH server which has both IPv6 and v4 addresses but is refusing connections to port 22 on the v6 address. GTK2 and GTK3 PuTTY cheerfully fall back to v4; GTK1 PuTTY bombed out immediately with a 'Connection refused' message box. Turned out this was because I had performed an asynchronous connect(2) operation; called uxsel_set() to ask for only writability notifications on the socket (that being how you find out that your async connect attempt has finished); uxsel_set() dutifully called gdk_input_add() with only the GDK_INPUT_WRITE flag; and GTK1 called me back with GDK_INPUT_READ as well as GDK_INPUT_WRITE set! So network.c tried to process the readability notification first, leading to the wrong branch of the network error handling code. Easily fixed by remembering what I/O flags we actually _asked_ for, and ignoring any others that GTK chooses to set in the callback.
When you test raising a group element to a power, the power only matters mod the order of the original element. So we should be reducing our test powers mod that, rather than mod the prime used in the curve's field. Now in all three test functions we reduce mod curve.G_order. This also involved making the curve.G_order field exist at all in the Python reference implementation of the Montgomery curves. I'd left it out. But that's easy, because those curves correspond closely to the two Edwards ones and share their values of G_order. Also, testMontgomeryMultiply wasn't even reducing mod the _right_ 256-bit prime: it was using p256.p rather than curve25519.p, presumably from a copy-paste error (p256.p is used in the Weierstrass function right next to it). Now all three functions assign the test curve to a variable of the same name, so that they _deliberately_ look alike.
During (NIST-style) ECDSA key generation, we generate a random exponent, and raise the base point to that power. So the exponent ought to be reduced mod the order of the base point. But in fact we were reducing it mod the prime order of the curve's field – the same error as I just fixed in the test suite in the previous commit. Luckily, this mistake is benign. That reduction is only precautionary in any case, and in fact never comes up, because keygen/ecdsa.c has already generated its random exponent using the _correct_ bounds. And G_order is always smaller than p, so the bogus reduction mod p never did anything.
The comment next to the assertion says that it's checking for either
identical inputs or mutually inverse ones: that is, input curve points
with the same affine x-coordinate. But in that case we should have
checked lambda_n, the _numerator_ of the slope of the line through the
input points. Instead we were accidentally checking lambda_d, the
denominator.
ecc_weierstrass_add_general gets this right: it checks lambda_n to
decide whether to use the output of the unequal-point addition formula
or the doubling formula.
The obvious fix is to replace the assertion with one checking
lambda_n. But that doesn't work, because it's possible during
legitimate operation to cause a call to ecc_weierstrass_add that would
fail the fixed version of the assertion! Because of constant-time
considerations, every time ecc_weierstrass_multiply consumes a bit of
the exponent, it calculates both of two possible outputs of that
iteration step, and then selects between them. And sometimes one of
those outputs _does_ involve passing mutually inverse values to
ecc_weierstrass_add. That's harmless if the result of that calculation
is thrown away by the caller – except that it's not harmless if you
fail an assertion and crash before getting back _to_ the caller!
(With the current style of exponentiation loop, the case that would
fail the 'right' assertion involves raising a point P to its own order
minus 1, because we're consuming the exponent from the MSB downwards,
so at every stage we choose between P^(2k) and P^(2k+1). But switching
to the LSB-upwards style of repeatedly squaring to make P^{2^n} and
conditionally multiplying in each of those, you'd still have the same
failure, just in a different place – the failing case would involve
raising P to the power of its own order with the leading 1 bit
removed.)
So that's why we can't replace the bogus assertion with a better one.
Instead, the only thing to do is remove the assertion completely, and
rely on callers to have already checked that exponents are in
range (hence the precautionary fix in ecdsa_public in the previous
commit).
What about triggering the _actual_ wrong assertion? Guido Vranken
reported this issue and included a test case that demonstrates that
it's possible to trigger the bug deliberately. His example case is a
P256 curve point P such that 10P and 11P have the same y-coordinate,
and therefore trying to compute 21P will add those values together and
fail the previous assertion. Worse, it's easy to construct a malicious
P256 public key and signature such that attempting to verify that
signature provokes the same crash – no matter what the message is. So
this is a DoS vector for an on-path attacker, who can make PuTTY crash
by substituting this fixed P256 key and signature in the cleartext
initial key exchange of any SSH connection.
(Only a DoS, though: PuTTY is always compiled with assertions enabled,
so it won't do anything _worse_ than crash with an unhelpful message.)
Ian suggested this to me the other day, and it seems worth adding to the docs. In fact it might actually be a _better_ use case than port knocking, so I've listed it first, relegating port knocking to second place. (Since a wake-up script would need to wait until the target server is sufficiently booted up to be listening for SSH, this use case benefits particularly from my rework to run the hook command asynchronously.) (cherry picked from commit b7b8389999878d19a6685ada944219d7d82a2c38)
(cherry picked from commit df256593e9716a191e9752d926064b32b5cee5a2)
I have a new WoA test machine which has the necessary CPU extension, and also, the docs for IsProcessorFeaturePresent have been updated to include an id you can use to test for it. So now Arm Windows PuTTY can detect support for hardware-accelerated SHA-512, and turn it on if available.
ssh_rsakex_freekey(), given an RSAKey structure, calls freersakey() to free all the pieces dangling off it, and then finally frees the RSAKey itself. So calling ssh_rsakex_freekey() and _then_ sfree() is a double free.
- LICENCE: MIT 许可证翻译为中文 - cmdgen.c: puttygen 帮助信息中文化 - config.c: 配置对话框中文化 - console.c: 控制台信息中文化 - pageant.c: Pageant 界面中文化 - pscp.c/psftp.c: 文件传输工具帮助信息中文化 - putty.h: 通用字符串中文化 - settings.c: 设置相关字符串中文化 - ssh/: SSH 模块中文化 - windows/: Windows 平台界面全面中文化 - conf.h: 默认配置调整 (串口速度115200, 滚动行9999等) - cmake: MSVC UTF-8 编译支持
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #92