Rsa cert2 #1
Open
Rsa cert2 #1
Conversation
This file contains 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
This formalises my occasional habit of using a single malloc to make a block that contains a header structure and a data buffer that a field of the structure will point to, allowing it to be freed in one go later. Previously I had to do this by hand, losing the type-checking advantages of snew; now I've written an snew-style macro to do the job, plus an accessor macro to cleanly get the auxiliary buffer pointer afterwards, and switched existing instances of the pattern over to using that.
It was horrible - even if harmless in practice - that it wrote the NATed channel id over its input buffer, and I think it's worth the extra memory management to avoid doing that.
This is the first stage of massively tidying up this very confused data structure. In this commit, I replace the unified 'struct Packet' with two structures PktIn and PktOut, each of which contains only the fields of struct Packet that are actually used for packets going in that direction - most notably, PktIn doesn't implement BinarySink, and PktOut doesn't implement BinarySource. All uses of the old structure were statically determinable to be one or the other, so I've done that determination and changed all the types of variables and function signatures. Unlike PktIn, PktOut is not reference-counted, so there's a new ssh_pktout_free function. The most immediately pleasing thing about this change is that it lets me finally get rid of the tedious comment explaining how the 'length' field in struct Packet meant something different depending on direction. Now it's two fields of the same name in two different structures, I can comment the same thing much less verbosely! (I've also got rid of the comment claiming the type field was only used for incoming packets. That wasn't even true! It might have been once, because you can write an outgoing packet's type byte straight into its data buffer, but in fact in the current code pktout->type is nonetheless used in various other places, e.g. log_outgoing_packet.) In this commit I've only removed the fields from each structure that were _already_ unused. There are still quite a few we can get rid of by _making_ them unused.
They were duplicating values stored in the BinarySource substructure. Mostly they're not referred to directly any more (instead, we call get_foo to access the BinarySource); and when they are, we can switch to reading the same values back out of the BinarySource anyway.
These were only used in the rdpkt coroutines, during construction of the incoming packet; once it's complete, they're never touched again. So really they should have been fields in the rdpkt coroutines' state - and now they are. The new memory allocation strategy for incoming packets is to defer creation of the returned pktin structure until we know how big its data buffer will really need to be, and then use snew_plus to make the PktIn and the payload block in the same allocation. When we have to read and keep some amount of the packet before allocating the returned structure, we do it by having a persistent buffer in the rdpkt state, which is retained for the whole connection and only freed once in ssh_free.
The body pointer was used after encryption to mark the start of the fully wire-ready packet by ssh2_pkt_construct, and before encryption by the log_outgoing_packet functions. Now the former returns a nice modern ptrlen (it never really needed to store the pointer _in_ the packet structure anyway), and the latter uses an integer 'prefix' field, which isn't very different in concept but saves effort on reallocs.
This saves a malloc and free every time we add or remove a packet from a packet queue - it can now be done by pure pointer-shuffling instead of allocating a separate list node structure.
rsa_ssh1_fingerprint will look at the input key's comment field, which I forgot to initialise to anything, even the NULL it should be.
This makes it easier for me to examine the contents of binary memory buffers, while debugging through code that does crypto or packet marshalling.
It seems especially silly for a structure whose name ends in _t to have to have the 'struct' prefix!
It is to put_data what memset is to memcpy. Several places in the code wanted it already, but not _quite_ enough for me to have written it with the rest of the BinarySink infrastructure originally.
ssh.c has been an unmanageably huge monolith of a source file for too long, and it's finally time I started breaking it up into smaller pieces. The first step is to move some declarations - basic types like packets and packet queues, standard constants, enums, and the coroutine system - into headers where other files can see them.
This mirrors the use of one for incoming wire data: now when we send raw data (be it the initial greeting, or the output of binary packet construction), we put it on ssh->outgoing_data, and schedule a callback to transfer that into the socket. Partly this is in preparation for delegating the task of appending to that bufchain to a separate self-contained module that won't have direct access to the connection's Socket. But also, it has the very nice feature that I get to throw away the ssh_pkt_defer system completely! That was there so that we could construct more than one packet in rapid succession, concatenate them into a single blob, and pass that blob to the socket in one go so that the TCP headers couldn't contain any trace of where the boundary between them was. But now we don't need a separate function to do that: calling the ordinary packet-send routine twice in the same function before returning to the main event loop will have that effect _anyway_.
sshbpp.h now defines a classoid that encapsulates both directions of an SSH binary packet protocol - that is, a system for reading a bufchain of incoming data and turning it into a stream of PktIn, and another system for taking a PktOut and turning it into data on an outgoing bufchain. The state structure in each of those files contains everything that used to be in the 'rdpkt2_state' structure and its friends, and also quite a lot of bits and pieces like cipher and MAC states that used to live in the main Ssh structure. One minor effect of this layer separation is that I've had to extend the packet dispatch table by one, because the BPP layer can no longer directly trigger sending of SSH_MSG_UNIMPLEMENTED for a message too short to have a type byte. Instead, I extend the PktIn type field to use an out-of-range value to encode that, and the easiest way to make that trigger an UNIMPLEMENTED message is to have the dispatch table contain an entry for it. (That's a system that may come in useful again - I was also wondering about inventing a fake type code to indicate network EOF, so that that could be propagated through the layers and be handled by whichever one currently knew best how to respond.) I've also moved the packet-censoring code into its own pair of files, partly because I was going to want to do that anyway sooner or later, and mostly because it's called from the BPP code, and the SSH-2 version in particular has to be called from both the main SSH-2 BPP and the bare unencrypted protocol used for connection sharing. While I was at it, I took the opportunity to merge the outgoing and incoming censor functions, so that the parts that were common between them (e.g. CHANNEL_DATA messages look the same in both directions) didn't need to be repeated.
Now we have the new marshalling system, I think it's outlived its usefulness, because the new system allows us to directly express various things (e.g. uint16 and non-zero-terminated strings) that were actually _more_ awkward to do via the variadic interface. So here's a rewrite that removes send_packet(), and replaces all its call sites with something that matches our SSH-2 packet construction idioms. This diff actually _reduces_ the number of lines of code in ssh.c. Since the variadic system was trying to save code by centralising things, that seems like the best possible evidence that it wasn't pulling its weight!
Thanks to Alex Landau for pointing out that commit 8b98fea introduced two uses of it with the arguments one way round and one with them the other way round. (Plus a fourth use where it doesn't matter, because the padding at the end of the encrypted blob of an OpenSSH PEM private key consists of n bytes with value n. :-) On the basis of majority vote, I've switched the order in the function definition to match the two of the three call sites that expressed the same opinion, and fixed the third.
When the whole SSH connection is throttled and then unthrottled, we need to requeue the callback that transfers data to the Socket from the new outgoing_data queue introduced in commit 9e3522a. The user-visible effect of this missing call was that outgoing SFTP transactions would lock up, because in SFTP mode we enable the "[email protected]" mode and essentially turn off the per-channel window management, so throttling of the whole connection becomes the main source of back-pressure.
The new SSH-2 BPP has two functions ssh2_bpp_new_outgoing_crypto and ssh2_bpp_new_incoming_crypto, which (due to general symmetry) are _almost_ identical, except that the code that sets up the compression context in the two directions has to call compress_init in the former and decompress_init in the latter. Except that it called compress_init in both, so compression in SSH-2 has been completely broken for a week. How embarrassing. I _remember_ thinking that I'd better make sure to change that one call between the two, but apparently it fell out of my head before I committed.
On the post-userauth rekey, when we're specifically rekeying in order to turn on delayed compression, we shouldn't write the Event Log "Server supports delayed compression; will try this later" message that we did in the original key exchange. At this point, it _is_ later, and we're about to turn on compression right now!
Previously, the code to recover and memory-map the file-mapping object Pageant uses for its IPC, and the code to convey its contents to and from the cross-platform agent code, were widely separated, with the former living in the WM_COPYDATA handler in the window procedure, and the latter in answer_msg. Now all of that code lives in answer_filemapping_message; WndProc only handles the _window message_ contents - i.e. ensures the WM_COPYDATA message has the right dwData id and that its lpData contains an ASCIZ string - and answer_filemapping_message goes all the way from that file-mapping object name to calling pageant_handle_msg. While I'm here, I've also tidied up the code so that it uses the 'goto cleanup' idiom rather than nesting everything inconveniently deeply, and arranged that if anything goes wrong then we at least _construct_ an error message (although as yet we don't use that for anything unless we're compiled with DEBUG_IPC enabled).
I heard recently that at least one third-party client of Pageant exists, and that it's used to generate signatures to use with TLS client certificates. Apparently the signature scheme is compatible, but TLS tends to need signatures over more data than will fit in AGENT_MAX_MSGLEN. Before the BinarySink refactor in commit b6cbad8, this was OK because the Windows Pageant IPC didn't check the size of the _input_ message against AGENT_MAX_MSGLEN, only the output one. But then we started checking both, so that third-party TLS client started failing. Now we use VirtualQuery to find out the actual size of the file mapping we've been passed, and our only requirement is that the input and output messages should both fit in _that_. So TLS should work again, and also, other clients should be able to retrieve longer lists of public keys if they pass a larger file mapping. One side effect of this change is that Pageant's reply message is now written directly into the shared-memory region. Previously, it was written into a separate buffer and then memcpy()ed over after pageant_handle_msg returned, but now the buffer is variable-size, it seems to me to make more sense to avoid that extra not-entirely controlled malloc. So I've done one very small reordering of statements in the cross-platform pageant_handle_msg(), which fixes the only case I could find where that function started writing its output before it had finished using the contents of the input buffer.
That's the same value as in the OpenSSH source code, so it should be large enough that anyone needing to sign a larger message will have other problems too.
I just spotted it while I was looking through this module anyway. It was using %.100s to prevent an sprintf buffer overflow, which hasn't been necessary since I switched everything over to dupprintf, and also it was printing the integer value of GetLastError() without using my convenient translation wrapper win_strerror.
I think ever since commit 679fa90 last month, PuTTY has been forgetting to free any of its outgoing packet structures after turning them into their encrypted wire format. And apparently no users of the development snapshots have noticed - including me!
Yesterday's reinstatement of ssh_free_pktout revealed - via valgrind spotting the use-after-free - that the code that prefixed sensible packets with IV-muddling SSH_MSG_IGNOREs was actually sending a second copy of the sensible packet in place of the IGNORE, due to a typo.
The return value wasn't used to indicate failure; it only indicated whether any compression was being done at all or whether the compression method was ssh_comp_none, and we can tell the latter just as well by the fact that its init function returns a null context pointer.
Now when we construct a packet containing sensitive data, we just set a field saying '... and make it take up at least this much space, to disguise its true size', and nothing in the rest of the system worries about that flag until ssh2bpp.c acts on it. Also, I've changed the strategy for doing the padding. Previously, we were following the real packet with an SSH_MSG_IGNORE to make up the size. But that was only a partial defence: it works OK against passive traffic analysis, but an attacker proxying the TCP stream and dribbling it out one byte at a time could still have found out the size of the real packet by noting when the dribbled data provoked a response. Now I put the SSH_MSG_IGNORE _first_, which should defeat that attack. But that in turn doesn't work when we're doing compression, because we can't predict the compressed sizes accurately enough to make that strategy sensible. Fortunately, compression provides an alternative strategy anyway: if we've got zlib turned on when we send one of these sensitive packets, then we can pad out the compressed zlib data as much as we like by adding empty RFC1951 blocks (effectively chaining ZLIB_PARTIAL_FLUSHes). So both strategies should now be dribble-proof.
I had previously left the platform field (in line 7 of the installer database's SummaryInformation table) set at "x86" instead of any value you might expect such as "Arm" or "Arm64", because I found that an MSI file with either of the latter values was rejected by WoA's msiexec as invalid. It turns out this is because I _also_ needed to upgrade the installer database schema version to a higher value than I even knew existed: apparently the problem is that those platform fields aren't present in the older schema. A test confirms that this works. Unfortunately, WiX 3 doesn't actually know _how_ to write MSIs with those platform values. But that's OK, because diffing the x86 and x64 MSIs against each other suggested that there were basically no other changes in the database tables - so I can just generate the installer as if for x64, and then rewrite that one field after installer construction using GNOME msitools to take apart the binary file structure and put it back together. (Those are the same tools I'm using as part of my system for running WiX on Linux in the first place.) This commit introduces a script to do that post-hoc bodging, and calls it from Buildscr. I've also changed over the choice of Program Files folder for the Arm installers so that it's ProgramFiles64Folder instead of ProgramFilesFolder - so now the Windows on Arm installer doesn't incongruously default to installing in C:\Program Files (x86)!
This commit adds the new ids and fingerprints in the keys appendix of the manual, and moves the old ones down into the historic-keys section. I've tweaked a few pieces of wording for ongoing use, so that they don't imply a specific number of past key rollovers. The -pgpfp option in all the tools now shows the new Master Key fingerprint and the previous (2015) one. I've adjusted all the uses of the #defines in putty.h so that future rollovers should only have to modify the #defines themselves. Most importantly, sign.sh bakes in the ids of the current release and snapshot keys, so that snapshots will automatically be signed with the new snapshot key and the -r option will invoke the new release key.
In GTK 2, this function was a new and convenient way to override the order in which the Tab key cycled through the sub-widgets of a container, replacing the more verbose mechanism in GTK 1 where you had to provide a custom implementation of the "focus" method in GtkContainerClass. GTK 3.24 has now deprecated gtk_container_set_focus_chain(), apparently on the grounds that that old system is what they think you _ought_ to be doing. So I've abandoned set_focus_chain completely, and switched back to doing it by a custom focus method for _all_ versions of GTK, with the only slight wrinkle being that between GTK 1 and 2 the method in question moved from GtkContainer to GtkWidget (my guess is so that an individual non-container widget can still have multiple separately focusable UI components).
This is the first time I've _ever_ been able to test that feature of the client userauth code personally, and pleasingly, it seems to work fine.
x11_char_struct returns a pointer or NULL, so while returning FALSE would have ended up doing the right thing after macro expansion and integer->pointer conversion, it wasn't actually how I _meant_ to spell a failure return.
In some compiler modes - notably the one that gtk-config selects when GTK PuTTY is built for GTK 1 - it's an error to typedef the same thing twice. 'mainchan' is defined in defs.h, so it doesn't need defining again where the structure contents are specified.
If values are boolean, it's confusing to use & and | in place of && and ||. In two of these three cases it was simply a typo and I've used the other one; in the third, it was a deliberate avoidance of short- circuit evaluation (and commented as such), but having seen how easy it is to make the same typo by accident, I've decided it's clearer to just move the LHS and RHS evaluations outside the expression.
The annoying int64.h is completely retired, since C99 guarantees a 64-bit integer type that you can actually treat like an ordinary integer. Also, I've replaced the local typedefs uint32 and word32 (scattered through different parts of the crypto code) with the standard uint32_t.
This commit includes <stdbool.h> from defs.h and deletes my traditional definitions of TRUE and FALSE, but other than that, it's a 100% mechanical search-and-replace transforming all uses of TRUE and FALSE into the C99-standardised lowercase spellings. No actual types are changed in this commit; that will come next. This is just getting the noise out of the way, so that subsequent commits can have a higher proportion of signal.
It's not actually used anywhere yet, though. This is just adding the accessor functions, which will enforce a rigorous separation between conf keys typed as int and as bool.
I think this is the full set of things that ought logically to be
boolean.
One annoyance is that quite a few radio-button controls in config.c
address Conf fields that are now bool rather than int, which means
that the shared handler function can't just access them all with
conf_{get,set}_int. Rather than back out the rigorous separation of
int and bool in conf.c itself, I've just added a similar alternative
handler function for the bool-typed ones.
My normal habit these days, in new code, is to treat int and bool as _almost_ completely separate types. I'm still willing to use C's implicit test for zero on an integer (e.g. 'if (!blob.len)' is fine, no need to spell it out as blob.len != 0), but generally, if a variable is going to be conceptually a boolean, I like to declare it bool and assign to it using 'true' or 'false' rather than 0 or 1. PuTTY is an exception, because it predates the C99 bool, and I've stuck to its existing coding style even when adding new code to it. But it's been annoying me more and more, so now that I've decided C99 bool is an acceptable thing to require from our toolchain in the first place, here's a quite thorough trawl through the source doing 'boolification'. Many variables and function parameters are now typed as bool rather than int; many assignments of 0 or 1 to those variables are now spelled 'true' or 'false'. I managed this thorough conversion with the help of a custom clang plugin that I wrote to trawl the AST and apply heuristics to point out where things might want changing. So I've even managed to do a decent job on parts of the code I haven't looked at in years! To make the plugin's work easier, I pushed platform front ends generally in the direction of using standard 'bool' in preference to platform-specific boolean types like Windows BOOL or GTK's gboolean; I've left the platform booleans in places they _have_ to be for the platform APIs to work right, but variables only used by my own code have been converted wherever I found them. In a few places there are int values that look very like booleans in _most_ of the places they're used, but have a rarely-used third value, or a distinction between different nonzero values that most users don't care about. In these cases, I've _removed_ uses of 'true' and 'false' for the return values, to emphasise that there's something more subtle going on than a simple boolean answer: - the 'multisel' field in dialog.h's list box structure, for which the GTK front end in particular recognises a difference between 1 and 2 but nearly everything else treats as boolean - the 'urgent' parameter to plug_receive, where 1 vs 2 tells you something about the specific location of the urgent pointer, but most clients only care about 0 vs 'something nonzero' - the return value of wc_match, where -1 indicates a syntax error in the wildcard. - the return values from SSH-1 RSA-key loading functions, which use -1 for 'wrong passphrase' and 0 for all other failures (so any caller which already knows it's not loading an _encrypted private_ key can treat them as boolean) - term->esc_query, and the 'query' parameter in toggle_mode in terminal.c, which _usually_ hold 0 for ESC[123h or 1 for ESC[?123h, but can also hold -1 for some other intervening character that we don't support. In a few places there's an integer that I haven't turned into a bool even though it really _can_ only take values 0 or 1 (and, as above, tried to make the call sites consistent in not calling those values true and false), on the grounds that I thought it would make it more confusing to imply that the 0 value was in some sense 'negative' or bad and the 1 positive or good: - the return value of plug_accepting uses the POSIXish convention of 0=success and nonzero=error; I think if I made it bool then I'd also want to reverse its sense, and that's a job for a separate piece of work. - the 'screen' parameter to lineptr() in terminal.c, where 0 and 1 represent the default and alternate screens. There's no obvious reason why one of those should be considered 'true' or 'positive' or 'success' - they're just indices - so I've left it as int. ssh_scp_recv had particularly confusing semantics for its previous int return value: its call sites used '<= 0' to check for error, but it never actually returned a negative number, just 0 or 1. Now the function and its call sites agree that it's a bool. In a couple of places I've renamed variables called 'ret', because I don't like that name any more - it's unclear whether it means the return value (in preparation) for the _containing_ function or the return value received from a subroutine call, and occasionally I've accidentally used the same variable for both and introduced a bug. So where one of those got in my way, I've renamed it to 'toret' or 'retd' (the latter short for 'returned') in line with my usual modern practice, but I haven't done a thorough job of finding all of them. Finally, one amusing side effect of doing this is that I've had to separate quite a few chained assignments. It used to be perfectly fine to write 'a = b = c = TRUE' when a,b,c were int and TRUE was just a the 'true' defined by stdbool.h, that idiom provokes a warning from gcc: 'suggest parentheses around assignment used as truth value'!
Notably toint(), which ought to compile down to the identity function in any case so you don't really want to put in a pointless call overhead, and make_ptrlen() (and a couple of its wrappers) which is standing in for what ought to be a struct-literal syntax.
I came across this unexplained static variable in my boolification trawl. It seems clearly unintentional that it has only one instance instead of one per terminal window - the code in question closely resembles the Windows front end, and I think this must just be a variable that I never swept up into 'inst' in the very early days when I was making gtkwin.c out of a cloned-and-hacked window.c in the first place. These days it's even a bug, now that the OS X port actually does run multiple terminal windows in the same process: if one goes into mouse reporting mode, I'm pretty sure this would have done confusing things to the effects of mouse actions in the other.
Jumped out at me in my trawl of the whole code base: set_explicit_app_user_model_id is declared and defined as () rather than (void), but this isn't C++.
There's one of these centralised in win_strerror() in winmisc.c, and it doesn't seem worth keeping an earlier iteration of the same idea entirely separate in winsock_error_string. This removal means that non-network-specific error codes received in a network context will no longer have "Network error:" prefixed to them. But I think that's OK, because it seems unlikely to be critically important that such an error was received from a network function - if anything like that comes up then it's probably some kind of systemwide chaos.
For a start, they now have different names on Windows and Unix, reflecting their different roles: on Windows they apply escaping to any string that's going to be used as a registry key (be it a session name, or a host name for host key storage), whereas on Unix they're for constructing saved-session file names in particular (and also handle the special case of filling in "Default Settings" for NULL). Also, they now produce output by writing to a strbuf, which simplifies a lot of the call sites. In particular, the strbuf output idiom is passed on to enum_settings_next, which is especially nice because its only actual caller was doing an ad-hoc realloc loop that I can now get rid of completely. Thirdly, on Windows they're centralised into winmisc.c instead of living in winstore.c, because that way Pageant can use the unescape function too. (It was spotting the duplication there that made me think of doing this in the first place, but once I'd started, I had to keep unravelling the thread...)
sk_startup and sk_nextaddr are entirely internal to winnet.c; nearly
all of import.c and minibidi.c's internal routines should have been
static and weren't; {read,write}_utf8 are internal to charset/utf8.c
(and didn't even need separate declarations at all); do_sftp_cleanup
is internal to psftp.c, and get_listitemheight to gtkdlg.c.
While I was editing those prototypes anyway, I've also added missing
'const' to the 'char *' passphrase parameters in import,c.
This is another cleanup I felt a need for while I was doing boolification. If you define a function or variable in one .c file and declare it extern in another, then nothing will check you haven't got the types of the two declarations mismatched - so when you're _changing_ the type, it's a pain to make sure you've caught all the copies of it. It's better to put all those extern declarations in header files, so that the declaration in the header is also in scope for the definition. Then the compiler will complain if they don't match, which is what I want.
GCC 5 does not trace control flow graph and claims that the variable may be used uninitialized. GCC 7 does not have this bug though.
Configuration pointer is globally visible from winstuff.h, so it cannot be 'static' any longer.
Unlike other certs, ED25519's cert isn't a simple concatenation of a cert blob with the ed25519 non-cert priv key. for some reason, the public key is duplicated, causing the cert to not parse correctly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Adding support for RSA certificates to pageant