Skip to content
Snippets Groups Projects
  1. Jun 21, 2020
  2. Feb 16, 2020
    • Simon Tatham's avatar
      Formatting change to braces around one case of a switch. · 8d186c3c
      Simon Tatham authored
      Sometimes, within a switch statement, you want to declare local
      variables specific to the handler for one particular case. Until now
      I've mostly been writing this in the form
      
          switch (discriminant) {
            case SIMPLE:
              do stuff;
              break;
            case COMPLICATED:
              {
                  declare variables;
                  do stuff;
              }
              break;
          }
      
      which is ugly because the two pieces of essentially similar code
      appear at different indent levels, and also inconvenient because you
      have less horizontal space available to write the complicated case
      handler in - particuarly undesirable because _complicated_ case
      handlers are the ones most likely to need all the space they can get!
      
      After encountering a rather nicer idiom in the LLVM source code, and
      after a bit of hackery this morning figuring out how to persuade
      Emacs's auto-indent to do what I wanted with it, I've decided to move
      to an idiom in which the open brace comes right after the case
      statement, and the code within it is indented the same as it would
      have been without the brace. Then the whole case handler (including
      the break) lives inside those braces, and you get something that looks
      more like this:
      
          switch (discriminant) {
            case SIMPLE:
              do stuff;
              break;
            case COMPLICATED: {
              declare variables;
              do stuff;
              break;
            }
          }
      
      This commit is a big-bang change that reformats all the complicated
      case handlers I could find into the new layout. This is particularly
      nice in the Pageant main function, in which almost _every_ case
      handler had a bundle of variables and was long and complicated. (In
      fact that's what motivated me to get round to this.) Some of the
      innermost parts of the terminal escape-sequence handling are also
      breathing a bit easier now the horizontal pressure on them is
      relieved.
      
      (Also, in a few cases, I was able to remove the extra braces
      completely, because the only variable local to the case handler was a
      loop variable which our new C99 policy allows me to move into the
      initialiser clause of its for statement.)
      
      Viewed with whitespace ignored, this is not too disruptive a change.
      Downstream patches that conflict with it may need to be reapplied
      using --ignore-whitespace or similar.
      8d186c3c
  3. Feb 02, 2020
    • Simon Tatham's avatar
      Allow import.c to read from a BinarySource. · 36d214c5
      Simon Tatham authored
      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.
      36d214c5
  4. Dec 15, 2019
    • Simon Tatham's avatar
      Adopt the new hash API functions where they're useful. · 1344d4d1
      Simon Tatham authored
      This commit switches as many ssh_hash_free / ssh_hash_new pairs as
      possible to reuse the previous hash object via ssh_hash_reset. Also a
      few other cleanups: use the wrapper function hash_simple() where
      possible, and I've also introduced ssh_hash_digest_nondestructive()
      and switched to that where possible as well.
      1344d4d1
  5. Sep 08, 2019
    • Simon Tatham's avatar
      Whitespace rationalisation of entire code base. · 5d718ef6
      Simon Tatham authored
      The number of people has been steadily increasing who read our source
      code with an editor that thinks tab stops are 4 spaces apart, as
      opposed to the traditional tty-derived 8 that the PuTTY code expects.
      
      So I've been wondering for ages about just fixing it, and switching to
      a spaces-only policy throughout the code. And I recently found out
      about 'git blame -w', which should make this change not too disruptive
      for the purposes of source-control archaeology; so perhaps now is the
      time.
      
      While I'm at it, I've also taken the opportunity to remove all the
      trailing spaces from source lines (on the basis that git dislikes
      them, and is the only thing that seems to have a strong opinion one
      way or the other).
          
      Apologies to anyone downstream of this code who has complicated patch
      sets to rebase past this change. I don't intend it to be needed again.
      5d718ef6
  6. May 05, 2019
    • Simon Tatham's avatar
      openssh_new_read: fix misaimed null pointer check. · f1fe0c7d
      Simon Tatham authored
      If the input key_wanted field were set to an out-of-range value, the
      parent structure retkey would not become NULL as a whole: instead, its
      field 'key' would never be set to a non-null pointer. So I was testing
      the wrong pointer.
      
      Fortunately, this couldn't have come up, because we don't actually
      have any support yet for loading the nth key from an OpenSSH new-style
      key file containing more than one. So key_wanted was always set to 0
      by load_openssh_new_key(), which also checked that the file's key
      count was exactly 1 (guarding against the possibility that even 0
      might have been an out-of-range index).
      f1fe0c7d
    • Simon Tatham's avatar
      Remove some spurious null pointer tests. · 0f6ce9bd
      Simon Tatham authored
      In load_openssh_new_key, ret->keyblob is never null any more: now that
      it's a strbuf instead of a bare realloc()ed string, it's at worst an
      _empty_ strbuf. Secondly, as Coverity pointed out, the null pointer
      check was too late to do any good in the first place - the previous
      clause of the same if condition would already have dereferenced it!
      
      In test_mac in the auxiliary testsc program, there's no actual reason
      I would ever have called it with null ssh_mac pointer - it would mean
      'don't test anything'! Looks as if I just copy-pasted the MAC parts of
      the cipher+MAC setup code in test_cipher.
      0f6ce9bd
  7. Mar 02, 2019
  8. Feb 28, 2019
    • Simon Tatham's avatar
      Replace more ad-hoc growing char buffers with strbuf. · d07d7d66
      Simon Tatham authored
      I've fixed a handful of these where I found them in passing, but when
      I went systematically looking, there were a lot more that I hadn't
      found!
      
      A particular highlight of this collection is the code that formats
      Windows clipboard data in RTF, which was absolutely crying out for
      strbuf_catf, and now it's got it.
      d07d7d66
  9. Feb 06, 2019
    • Simon Tatham's avatar
      Assorted further migration to ptrlen. · 5b17a2ce
      Simon Tatham authored
      The local put_mp_*_from_string functions in import.c now take ptrlen
      (which simplifies essentially all their call sites); so does the local
      function logwrite() in logging.c, and so does ssh2_fingerprint_blob.
      5b17a2ce
    • Simon Tatham's avatar
      Add and use BinarySource_*INIT_PL. · 751a9890
      Simon Tatham authored
      A great many BinarySource_BARE_INIT calls are passing the two halves
      of a ptrlen as separate arguments. It saves a lot of call-site faff to
      have a variant of the init function that just takes the whole ptrlen
      in one go.
      751a9890
  10. Feb 04, 2019
    • Simon Tatham's avatar
      Stop using unqualified {GET,PUT}_32BIT. · acc21c4c
      Simon Tatham authored
      Those were a reasonable abbreviation when the code almost never had to
      deal with little-endian numbers, but they've crept into enough places
      now (e.g. the ECC formatting) that I think I'd now prefer that every
      use of the integer read/write macros was clearly marked with its
      endianness.
      
      So all uses of GET_??BIT and PUT_??BIT are now qualified. The special
      versions in x11fwd.c, which used variable endianness because so does
      the X11 protocol, are suffixed _X11 to make that clear, and where that
      pushed line lengths over 80 characters I've taken the opportunity to
      name a local variable to remind me of what that extra parameter
      actually does.
      acc21c4c
  11. Jan 23, 2019
    • Simon Tatham's avatar
      Replace random_byte() with random_read(). · 628e7948
      Simon Tatham authored
      This is in preparation for a PRNG revamp which will want to have a
      well defined boundary for any given request-for-randomness, so that it
      can destroy the evidence afterwards. So no more looping round calling
      random_byte() and then stopping when we feel like it: now you say up
      front how many random bytes you want, and call random_read() which
      gives you that many in one go.
      
      Most of the call sites that had to be fixed are fairly mechanical, and
      quite a few ended up more concise afterwards. A few became more
      cumbersome, such as mp_random_bits, in which the new API doesn't let
      me load the random bytes directly into the target integer without
      triggering undefined behaviour, so instead I have to allocate a
      separate temporary buffer.
      
      The _most_ interesting call site was in the PKCS#1 v1.5 padding code
      in sshrsa.c (used in SSH-1), in which you need a stream of _nonzero_
      random bytes. The previous code just looped on random_byte, retrying
      if it got a zero. Now I'm doing a much more interesting thing with an
      mpint, essentially scaling a binary fraction repeatedly to extract a
      number in the range [0,255) and then adding 1 to it.
      628e7948
  12. Jan 20, 2019
    • Simon Tatham's avatar
      Access all hashes and MACs through the standard API. · 0d2d20aa
      Simon Tatham authored
      All the hash-specific state structures, and the functions that
      directly accessed them, are now local to the source files implementing
      the hashes themselves. Everywhere we previously used those types or
      functions, we're now using the standard ssh_hash or ssh2_mac API.
      
      The 'simple' functions (hmacmd5_simple, SHA_Simple etc) are now a pair
      of wrappers in sshauxcrypt.c, each of which takes an algorithm
      structure and can do the same conceptual thing regardless of what it
      is.
      0d2d20aa
  13. Jan 18, 2019
    • Simon Tatham's avatar
      Merge the ssh1_cipher type into ssh2_cipher. · 986508a5
      Simon Tatham authored
      The aim of this reorganisation is to make it easier to test all the
      ciphers in PuTTY in a uniform way. It was inconvenient that there were
      two separate vtable systems for the ciphers used in SSH-1 and SSH-2
      with different functionality.
      
      Now there's only one type, called ssh_cipher. But really it's the old
      ssh2_cipher, just renamed: I haven't made any changes to the API on
      the SSH-2 side. Instead, I've removed ssh1_cipher completely, and
      adapted the SSH-1 BPP to use the SSH-2 style API.
      
      (The relevant differences are that ssh1_cipher encapsulated both the
      sending and receiving directions in one object - so now ssh1bpp has to
      make a separate cipher instance per direction - and that ssh1_cipher
      automatically initialised the IV to all zeroes, which ssh1bpp now has
      to do by hand.)
      
      The previous ssh1_cipher vtable for single-DES has been removed
      completely, because when converted into the new API it became
      identical to the SSH-2 single-DES vtable; so now there's just one
      vtable for DES-CBC which works in both protocols. The other two SSH-1
      ciphers each had to stay separate, because 3DES is completely
      different between SSH-1 and SSH-2 (three layers of CBC structure
      versus one), and Blowfish varies in endianness and key length between
      the two.
      
      (Actually, while I'm here, I've only just noticed that the SSH-1
      Blowfish cipher mis-describes itself in log messages as Blowfish-128.
      In fact it passes the whole of the input key buffer, which has length
      SSH1_SESSION_KEY_LENGTH == 32 bytes == 256 bits. So it's actually
      Blowfish-256, and has been all along!)
      986508a5
  14. Jan 13, 2019
    • Simon Tatham's avatar
      Rename the AES vtables. · be5c0e63
      Simon Tatham authored
      The old names like ssh_aes128 and ssh_aes128_ctr reflect the SSH
      protocol IDs, which is all very well, but I think a more important
      principle is that it should be easy for me to remember which cipher
      mode each one refers to. So I've renamed them so that they all end in
      _cbc and _sdctr.
      
      (I've left alone the string identifiers used by testcrypt, for the
      moment. Perhaps I'll go back and change those later.)
      be5c0e63
    • Simon Tatham's avatar
      Localise AESContext into sshaes.c. · 91284547
      Simon Tatham authored
      All access to AES throughout the code is now done via the ssh2_cipher
      vtable interface. All code that previously made direct calls to the
      underlying functions (for encrypting and decrypting private key files)
      now does it by instantiating an ssh2_cipher.
      
      This removes constraints on the AES module's internal structure, and
      allows me to reorganise it as much as I like.
      91284547
  15. Jan 04, 2019
    • Simon Tatham's avatar
      Remove a lot of pointless 'struct' keywords. · 35690040
      Simon Tatham authored
      This is the commit that f3295e0f _should_ have been. Yesterday I just
      added some typedefs so that I didn't have to wear out my fingers
      typing 'struct' in new code, but what I ought to have done is to move
      all the typedefs into defs.h with the rest, and then go through
      cleaning up the legacy 'struct's all through the existing code.
      
      But I was mostly trying to concentrate on getting the test suite
      finished, so I just did the minimum. Now it's time to come back and do
      it better.
      35690040
  16. Jan 03, 2019
    • Simon Tatham's avatar
      New marshalling function put_datapl(). · c02031ff
      Simon Tatham authored
      Just like put_data(), but takes a ptrlen rather than separate ptr and
      len arguments, so it saves a bit of repetition at call sites. I
      probably should have written this ages ago, but better late than
      never; I've also converted every call site I can find that needed it.
      c02031ff
    • Simon Tatham's avatar
      Replace assert(false) with an unreachable() macro. · 0112936e
      Simon Tatham authored
      Taking a leaf out of the LLVM code base: this macro still includes an
      assert(false) so that the message will show up in a typical build, but
      it follows it up with a call to a function explicitly marked as no-
      return.
      
      So this ought to do a better job of convincing compilers that once a
      code path hits this function it _really doesn't_ have to still faff
      about with making up a bogus return value or filling in a variable
      that 'might be used uninitialised' in the following code that won't be
      reached anyway.
      
      I've gone through the existing code looking for the assert(false) /
      assert(0) idiom and replaced all the ones I found with the new macro,
      which also meant I could remove a few pointless return statements and
      variable initialisations that I'd already had to put in to placate
      compiler front ends.
      0112936e
  17. Dec 31, 2018
    • Simon Tatham's avatar
      Complete rewrite of PuTTY's bignum library. · 25b034ee
      Simon Tatham authored
      The old 'Bignum' data type is gone completely, and so is sshbn.c. In
      its place is a new thing called 'mp_int', handled by an entirely new
      library module mpint.c, with API differences both large and small.
      
      The main aim of this change is that the new library should be free of
      timing- and cache-related side channels. I've written the code so that
      it _should_ - assuming I haven't made any mistakes - do all of its
      work without either control flow or memory addressing depending on the
      data words of the input numbers. (Though, being an _arbitrary_
      precision library, it does have to at least depend on the sizes of the
      numbers - but there's a 'formal' size that can vary separately from
      the actual magnitude of the represented integer, so if you want to
      keep it secret that your number is actually small, it should work fine
      to have a very long mp_int and just happen to store 23 in it.) So I've
      done all my conditionalisation by means of computing both answers and
      doing bit-masking to swap the right one into place, and all loops over
      the words of an mp_int go up to the formal size rather than the actual
      size.
      
      I haven't actually tested the constant-time property in any rigorous
      way yet (I'm still considering the best way to do it). But this code
      is surely at the very least a big improvement on the old version, even
      if I later find a few more things to fix.
      
      I've also completely rewritten the low-level elliptic curve arithmetic
      from sshecc.c; the new ecc.c is closer to being an adjunct of mpint.c
      than it is to the SSH end of the code. The new elliptic curve code
      keeps all coordinates in Montgomery-multiplication transformed form to
      speed up all the multiplications mod the same prime, and only converts
      them back when you ask for the affine coordinates. Also, I adopted
      extended coordinates for the Edwards curve implementation.
      
      sshecc.c has also had a near-total rewrite in the course of switching
      it over to the new system. While I was there, I've separated ECDSA and
      EdDSA more completely - they now have separate vtables, instead of a
      single vtable in which nearly every function had a big if statement in
      it - and also made the externally exposed types for an ECDSA key and
      an ECDH context different.
      
      A minor new feature: since the new arithmetic code includes a modular
      square root function, we can now support the compressed point
      representation for the NIST curves. We seem to have been getting along
      fine without that so far, but it seemed a shame not to put it in,
      since it was suddenly easy.
      
      In sshrsa.c, one major change is that I've removed the RSA blinding
      step in rsa_privkey_op, in which we randomise the ciphertext before
      doing the decryption. The purpose of that was to avoid timing leaks
      giving away the plaintext - but the new arithmetic code should take
      that in its stride in the course of also being careful enough to avoid
      leaking the _private key_, which RSA blinding had no way to do
      anything about in any case.
      
      Apart from those specific points, most of the rest of the changes are
      more or less mechanical, just changing type names and translating code
      into the new API.
      25b034ee
  18. Nov 03, 2018
    • Simon Tatham's avatar
      Add missing 'static' on file-internal declarations. · 91d16881
      Simon Tatham authored
      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.
      91d16881
    • Simon Tatham's avatar
      Convert a lot of 'int' variables to 'bool'. · 3214563d
      Simon Tatham authored
      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'!
      3214563d
    • Simon Tatham's avatar
      Adopt C99 <stdbool.h>'s true/false. · a6f1709c
      Simon Tatham authored
      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.
      a6f1709c
  19. Oct 06, 2018
    • Simon Tatham's avatar
      Rename FROMFIELD to 'container_of'. · 9396fcc9
      Simon Tatham authored
      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 :-)
      9396fcc9
  20. Sep 20, 2018
    • Simon Tatham's avatar
      Turn SSH-2 ciphers into a classoid. · 229af2b5
      Simon Tatham authored
      This is more or less the same job as the SSH-1 case, only more
      extensive, because we have a wider range of ciphers.
      
      I'm a bit disappointed about the AES case, in particular, because I
      feel as if it ought to have been possible to arrange to combine this
      layer of vtable dispatch with the subsidiary one that selects between
      hardware and software implementations of the underlying cipher. I may
      come back later and have another try at that, in fact.
      229af2b5
  21. Jun 09, 2018
    • Simon Tatham's avatar
      New BinarySink function 'put_padding'. · 8b98fea4
      Simon Tatham authored
      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.
      8b98fea4
  22. Jun 03, 2018
    • Simon Tatham's avatar
      Fix assertion failure on ssh.com export of ECDSA. · 40580029
      Simon Tatham authored
      It's a key type that format doesn't know how to handle, but that's no
      excuse to fail an assertion - we have a perfectly good failure code we
      can return from the export function, so we should use it.
      40580029
    • Simon Tatham's avatar
      Reorganise ssh_keyalg and use it as a vtable. · 06a14fe8
      Simon Tatham authored
      After Pavel Kryukov pointed out that I have to put _something_ in the
      'ssh_key' structure, I thought of an actually useful thing to put
      there: why not make it store a pointer to the ssh_keyalg structure?
      Then ssh_key becomes a classoid - or perhaps 'traitoid' is a closer
      analogy - in the same style as Socket and Plug. And just like Socket
      and Plug, I've also arranged a system of wrapper macros that avoid the
      need to mention the 'object' whose method you're invoking twice at
      each call site.
      
      The new vtable pointer directly replaces an existing field of struct
      ec_key (which was usable by several different ssh_keyalgs, so it
      already had to store a pointer to the currently active one), and also
      replaces the 'alg' field of the ssh2_userkey structure that wraps up a
      cryptographic key with its comment field.
      
      I've also taken the opportunity to clean things up a bit in general:
      most of the methods now have new and clearer names (e.g. you'd never
      know that 'newkey' made a public-only key while 'createkey' made a
      public+private key pair unless you went and looked it up, but now
      they're called 'new_pub' and 'new_priv' you might be in with a
      chance), and I've completely removed the openssh_private_npieces field
      after realising that it was duplicating information that is actually
      _more_ conveniently obtained by calling the new_priv_openssh method
      (formerly openssh_createkey) and throwing away the result.
      06a14fe8
  23. Jun 02, 2018
    • Simon Tatham's avatar
      Clean up ssh_keyalg APIs and implementations. · ae3edcdf
      Simon Tatham authored
      Quite a few of the function pointers in the ssh_keyalg vtable now take
      ptrlen arguments in place of separate pointer and length pairs.
      Meanwhile, the various key types' implementations of those functions
      now work by initialising a BinarySource with the input ptrlen and
      using the new decode functions to walk along it.
      
      One exception is the openssh_createkey method which reads a private
      key in the wire format used by OpenSSH's SSH-2 agent protocol, which
      has to consume a prefix of a larger data stream, and tell the caller
      how much of that data was the private key. That function now takes an
      actual BinarySource, and passes that directly to the decode functions,
      so that on return the caller finds that the BinarySource's read
      pointer has been advanced exactly past the private key.
      
      This let me throw away _several_ reimplementations of mpint-reading
      functions, one in each of sshrsa, sshdss.c and sshecc.c. Worse still,
      they didn't all have exactly the SSH-2 semantics, because the thing in
      sshrsa.c whose name suggested it was an mpint-reading function
      actually tolerated the wrong number of leading zero bytes, which it
      had to be able to do to cope with the "ssh-rsa" signature format which
      contains a thing that isn't quite an SSH-2 mpint. Now that deviation
      is clearly commented!
      ae3edcdf
    • Simon Tatham's avatar
      Rewrite key import functions using BinarySource. · 59e83a8c
      Simon Tatham authored
      The OpenSSH PEM reader is the most interesting conversion out of
      these: it was using a standalone function called get_ber_id_len(),
      which only skipped over the header of an ASN.1 BER data item and left
      the current position at the start of the payload. That's been replaced
      by a get_ber() function more in the spirit of the new API, which
      consumes the entire BER element, returning its header details and also
      a ptrlen pointing at its payload.
      
      (That function could easily be promoted out of import.c to somewhere
      more central, if we ever had a need to handle ASN.1 on a larger scale
      - e.g. X.509 certificates would find the same function useful. For the
      moment, though, it can stay where it is.)
      
      Other than that, this is a fairly mechanical API translation.
      59e83a8c
  24. May 27, 2018
    • Simon Tatham's avatar
      Invent a struct type for polymorphic SSH key data. · 0fc2d3b4
      Simon Tatham authored
      During last week's work, I made a mistake in which I got the arguments
      backwards in one of the key-blob-generating functions - mistakenly
      swapped the 'void *' key instance with the 'BinarySink *' output
      destination - and I didn't spot the mistake until run time, because in
      C you can implicitly convert both to and from void * and so there was
      no compile-time failure of type checking.
      
      Now that I've introduced the FROMFIELD macro that downcasts a pointer
      to one field of a structure to retrieve a pointer to the whole
      structure, I think I might start using that more widely to indicate
      this kind of polymorphic subtyping. So now all the public-key
      functions in the struct ssh_signkey vtable handle their data instance
      in the form of a pointer to a subfield of a new zero-sized structure
      type 'ssh_key', which outside the key implementations indicates 'this
      is some kind of key instance but it could be of any type'; they
      downcast that pointer internally using FROMFIELD in place of the
      previous ordinary C cast, and return one by returning &foo->sshk for
      whatever foo they've just made up.
      
      The sshk member is not at the beginning of the structure, which means
      all those FROMFIELDs and &key->sshk are actually adding and
      subtracting an offset. Of course I could have put the member at the
      start anyway, but I had the idea that it's actually a feature _not_ to
      have the two types start at the same address, because it means you
      should notice earlier rather than later if you absentmindedly cast
      from one to the other directly rather than by the approved method (in
      particular, if you accidentally assign one through a void * and back
      without even _noticing_ you perpetrated a cast). In particular, this
      enforces that you can't sfree() the thing even once without realising
      you should instead of called the right freekey function. (I found
      several bugs by this method during initial testing, so I think it's
      already proved its worth!)
      
      While I'm here, I've also renamed the vtable structure ssh_signkey to
      ssh_keyalg, because it was a confusing name anyway - it describes the
      _algorithm_ for handling all keys of that type, not a specific key. So
      ssh_keyalg is the collection of code, and ssh_key is one instance of
      the data it handles.
      0fc2d3b4
  25. May 26, 2018
    • Simon Tatham's avatar
      Make lots of generic data parameters into 'void *'. · 7babe66a
      Simon Tatham authored
      This is a cleanup I started to notice a need for during the BinarySink
      work. It removes a lot of faffing about casting things to char * or
      unsigned char * so that some API will accept them, even though lots of
      such APIs really take a plain 'block of raw binary data' argument and
      don't care what C thinks the signedness of that data might be - they
      may well reinterpret it back and forth internally.
      
      So I've tried to arrange for all the function call APIs that ought to
      have a void * (or const void *) to have one, and those that need to do
      pointer arithmetic on the parameter internally can cast it back at the
      top of the function. That saves endless ad-hoc casts at the call
      sites.
      7babe66a
    • Simon Tatham's avatar
      Remove a redundant failure check after an snew. · 2bfbf15c
      Simon Tatham authored
      I spotted this at some point during this week's BinarySink
      refactoring, but only just remembered to come back and fix it. snew
      aborts the whole program rather than return NULL, so there's no need
      to check its return value against NULL.
      2bfbf15c
  26. May 25, 2018
    • Simon Tatham's avatar
      Use BinarySink to tidy up key export code. · 8ce0a670
      Simon Tatham authored
      The output routines in import.c and sshpubk.c were further horrifying
      hotbeds of manual length-counting. Reworked it all so that it builds
      up key file components in strbufs, and uses the now boringly standard
      put_* functions to write into those strbufs.
      
      This removes the write_* functions in import.c, which I had to hastily
      rename a few commits ago when I introduced the new marshalling system
      in the first place.
      
      However, I wasn't quite able to get rid of _all_ of import.c's local
      formatting functions; there are a couple still there (but now with new
      BinarySink-style API) which output multiprecision integers in a couple
      of different formats starting from an existing big-endian binary
      representation, as opposed to starting from an internal Bignum.
      8ce0a670
    • Simon Tatham's avatar
      Change ssh.h crypto APIs to output to BinarySink. · 67de463c
      Simon Tatham authored
      This affects all the functions that generate public and private key
      and signature blobs of all kinds, plus ssh_ecdhkex_getpublic. Instead
      of returning a bare block of memory and taking an extra 'int *length'
      parameter, all these functions now write to a BinarySink, and it's the
      caller's job to have prepared an appropriate one where they want the
      output to go (usually a strbuf).
      
      The main value of this change is that those blob-generation functions
      were chock full of ad-hoc length-counting and data marshalling. You
      have only to look at rsa2_{public,private}_blob, for example, to see
      the kind of thing I was keen to get rid of!
      67de463c
    • Simon Tatham's avatar
      Replace all uses of SHA*_Bytes / MD5Update. · 4988fd41
      Simon Tatham authored
      In fact, those functions don't even exist any more. The only way to
      get data into a primitive hash state is via the new put_* system. Of
      course, that means put_data() is a viable replacement for every
      previous call to one of the per-hash update functions - but just
      mechanically doing that would have missed the opportunity to simplify
      a lot of the call sites.
      4988fd41
    • Simon Tatham's avatar
      New centralised binary-data marshalling system. · 0e3082ee
      Simon Tatham authored
      I've finally got tired of all the code throughout PuTTY that repeats
      the same logic about how to format the SSH binary primitives like
      uint32, string, mpint. We've got reasonably organised code in ssh.c
      that appends things like that to 'struct Packet'; something similar in
      sftp.c which repeats a lot of the work; utility functions in various
      places to format an mpint to feed to one or another hash function; and
      no end of totally ad-hoc stuff in functions like public key blob
      formatters which actually have to _count up_ the size of data
      painstakingly, then malloc exactly that much and mess about with
      PUT_32BIT.
      
      It's time to bring all of that into one place, and stop repeating
      myself in error-prone ways everywhere. The new marshal.h defines a
      system in which I centralise all the actual marshalling functions, and
      then layer a touch of C macro trickery on top to allow me to (look as
      if I) pass a wide range of different types to those functions, as long
      as the target type has been set up in the right way to have a write()
      function.
      
      This commit adds the new header and source file, and sets up some
      general centralised types (strbuf and the various hash-function
      contexts like SHA_State), but doesn't use the new calls for anything
      yet.
      
      (I've also renamed some internal functions in import.c which were
      using the same names that I've just defined macros over. That won't
      last long - those functions are going to go away soon, so the changed
      names are strictly temporary.)
      0e3082ee
  27. Apr 11, 2018
Loading