When I cut it in half so I could fetch the XCharStruct for a given
character, I forgot that the remaining half should check whether it
had got NULL from the XCharStruct finder. Ahem.
If you run something like 'seq 2000000000' in a GTK3 pterm, the window
never actually updates, because pterm always considers reading more
data from the pty to have higher priority than delivering the "draw"
event. Using g_io_add_watch_full instead of g_io_add_watch allows us
to explicitly lower the priority of the I/O sources, so that window
redraws will take precedence.
This is quite a pain, since it involves inventing an entire new piece
of infrastructure to install a custom Xlib error handler and give it a
queue of things to do. But it fixes a bug in which Unix pterm/PuTTY
crash out at startup if one of the root window's CUT_BUFFERn
properties contains something of a type other than STRING - in
particular, UTF8_STRING is not unheard-of.
For example, run
xprop -root -format CUT_BUFFER3 8u -set CUT_BUFFER3 "thingy"
and then pterm without this fix would have crashed.
I had completely forgotten, when rendering each glyph to a server-side
pixmap and downloading its contents, to only look at the part of the
pixmap that XDrawImageString would have overwritten, as specified by
the metrics in the XCharStruct. Now 'pterm -fn server:variable'
doesn't randomly make up bitmap nonsense outside each character's
bounding rectangle.
It would be nicer if we could also make this show up as the icon for
the .msi file itself when viewed in Explorer, but apparently nothing
can change that. But at least this still gives us _some_ use for the
cardboard-box icon :-)
Patch due to Colin Watson.
Putting the passphrase in a file avoids exposing it to 'ps' which can
print out every process's command line, while at the same time not
being as platform-specific as the approach of providing an fd number
(since cmdgen.c is in principle a potential cross-platform PuTTYgen,
not just a Unix one, which is why it's not in the 'unix' directory).
Of course it introduces its own risks if someone can read the file
from your disk after you delete it; probably the best approach to
avoiding this, if possible, is to point the option at a file on an
in-memory tmpfs type file system. Or better still, use bash-style
/dev/fd options such as
puttygen --new-passphrase <(echo -n "my passphrase") [options]
Failing that, try a secure file-wipe utility, as the man page change
mentions.
(And a use case not to be overlooked, of course, is the one where you
actually want to generate an unprotected key - in which case, just
pass /dev/null as the filename.)
Mostly this is a reaction to the reports of Inno Setup having a DLL
hijacking vulnerability. But also, the new installer has several other
nice features that our Inno Setup one didn't provide: it can put the
PuTTY install directory on PATH automatically, and it supports
completely automatic and silent install/uninstall via 'msiexec /q'
which should make it easier for sysadmins to roll out installation in
large organisations. Also, it just seems like good sense to be using
Windows's own native packaging system (or closest equivalent) rather
than going it alone.
(And on the developer side, I have to say I like the fact that WiX
lets me pass in the version number as a set of command-line #define-
equivalents, whereas for Inno Setup I had to have Buildscr apply Perl
rewriting to the source file.)
For the moment, I'm still building the old Inno Setup installer
alongside this one, but I expect to retire it once the WiX one has
survived in the wild for a while and proven itself more or less
stable.
I've found both MSI and WiX to be confusing and difficult
technologies, so this installer has some noticeable pieces missing
(e.g. retrospective reconfiguration of the installed feature set, and
per-user vs systemwide installation) simply because I couldn't get
them to work. I've commented the new installer source code heavily, in
the hope that a passing WiX expert can give me a hand!
That option does exist, but only on master; it was not in the 0.66
release. It turned up by mistake when I updated the documentation copy
of the Plink online help while preparing the 0.66 release, because I
ran plink from the wrong branch.
The new release automation should stop that kind of mistake from
happening in future.
A user reported in January that locking down our process ACL causes
get_user_sid's call to OpenProcessToken to fail with a permissions
error. This _shouldn't_ be important, because we'll already have found
and cached the user SID before getting that far - but unfortunately
the call to get_user_sid in winnpc.c was bypassing the cache and
trying the whole process again.
This fix changes the memory ownership semantics of get_user_sid():
it's now an error to free the value it gives you, or else the *next*
call to get_user_sid() will return a stale pointer. Hence, also
removed those frees everywhere they appear.
The reporter of vuln-pscp-sink-sscanf asked for a key to encrypt the
vulnerability report with, and having generated one, it seemed like a
good idea to make it part of the official PuTTY GPG key set and
publish it for the next person to use.
ssh_pkt_getstring can return (NULL,0) if the input packet is too short
to contain a valid string.
In quite a few places we were passing the returned pointer,length pair
to a printf function with "%.*s" type format, which seems in practice
to have not been dereferencing the pointer but the C standard doesn't
actually guarantee that. In one place we were doing the same job by
hand with memcpy, and apparently that _can_ dereference the pointer in
practice (so a server could have caused a NULL-dereference crash by
sending an appropriately malformed "x11" type channel open request).
And also I spotted a logging call in the "forwarded-tcpip" channel
open handler which had forgotten the field width completely, so it was
erroneously relying on the string happening to be NUL-terminated in
the received packet.
I've tightened all of this up in general by normalising (NULL,0) to
("",0) before calling printf("%.*s"), and replacing the two even more
broken cases with the corrected version of that same idiom.
There was a rogue sscanf("%s") with no field width limit, targeting a
stack-based buffer, and scanning a string containing untrusted data.
It occurs in the 'sink' side of the protocol, i.e. when downloading
files *from* the server.
Our own bug id for this vulnerability is 'vuln-pscp-sink-sscanf'.
GCC 6 emits strict-aliasing warnings here, so use the existing
sockaddr_union arrangements to avoid those. As a prerequisite for being
able to express sk_tcp_peer_info in terms of sockaddr_union, I fixed up
the union elements to be a bit less odd in the NO_IPV6 case.
(cherry picked from commit c026b48c53)
GCC 6 warns about potentially misleading indentation, such as:
if (condition) stmt1; stmt2;
Chaining multiple ifs on a single line runs into this warning, even if
it's probably not actually misleading to a human eye, so just add a
couple of newlines to pacify the compiler.
(cherry picked from commit d700c33422)
Not that anyone actually needs to use that conditioned-out main(),
since it only generates the table already present in the same source
file, but since @ch3root's unused-variable patch touched it I tried
compiling it and noticed in passing that I'd also got the wrong printf
format directive for an unsigned long.
(cherry picked from commit 9351a5bfe4)
Now we have licence.pl, it seems to me to make very good sense to have
it generate the Halibut form(s) of the licence and copyright year as
well as the source-code forms.
As a result, I believe _no_ copies of the licence text or copyright
date exist any more except for the master one in LICENCE - so I can
completely remove the checklist section about all the places to update
it, because there's only one. Hooray!
(cherry picked from commit 774d37a0dc)
Conflicts:
doc/licence.but
(cherry-picker's note: the conflict was just because the deleted file
didn't have identical contents)
Now all the uses of the licence text or the short copyright notice get
it from a new header "licence.h", which in turn is built by a Perl
script licence.pl invoked by mkfiles.pl, using LICENCE itself as the
source.
Hence, I can completely remove a whole section from the list of
licence locations in CHECKLST.txt :-)
(cherry picked from commit 9ddd071ec2)
Conflicts:
unix/gtkdlg.c
windows/winpgnt.c
(cherry-picker's notes: one conflict was just changed context, the
other was deleting a copy of the licence that wasn't quite the same
between branches)
This makes the About and Licence boxes copy-and-pasteable, similarly
to what I've just done on Unix.
(But unlike on the Unix side, here I haven't touched the host key
prompt dialog, because that's a standard Windows MessageBox and not
easy to mess around with. Plus, in any case, you can already hit ^C to
copy the whole text out of a MessageBox. Same goes for the PGP
fingerprints dialog.)
As a side effect, several copies of the copyright notice and licence
text have moved from .rc files into C source. I've updated
CHECKLST.txt, but they won't stay there for long.
(cherry picked from commit 2eb952ca31)
Conflicts:
windows/pageant.rc
windows/puttygen.rc
windows/win_res.rc2
(cherry-picker's notes: the conflict was just because several copies
of the licence text were deleted, and they weren't quite the same
between branches)
I've made the licence text, the About box, and the host key dialog
into GTK selectable edit controls. (The former because it contains a
lot of text; the About box because pasting version numbers into bug
reports is obviously useful; the host key because of the fingerprint.)
(cherry picked from commit 21101c7397)
Conflicts:
unix/gtkdlg.c
unix/unix.h
(cherry-picker's notes: not a trivial resolution, since I had to apply
the equivalent changes in the pre-GTK3-port version of the code)
Or, at least, potentially do so. The build script now has a slot into
which code-signing can be dropped by setting a variable in the bob
configuration to specify an appropriate command line.
The variable will typically need to point at a script wrapping the
actual signing tool, since there are lots of fiddly details
(timestamping countersignature, certificate, private key, etc) not
given on the command lines in this build script, on the basis that
they're local configuration questions for whoever is _running_ this
build script.
(cherry picked from commit d0e9630e1c)
logevent() doesn't do printf-style formatting (though the logeventf
wrapper in ssh.c does), so if you need to format a message, it has to
be done separately with dupprintf.
(cherry picked from commit 1659cf3f14)
By default Windows processes have wide open ACLs which allow interference
by other processes running as the same user. Adjust our ACL to make this
a bit harder.
Because it's useful to protect PuTTYtel as well, carve winsecur.c into
advapi functions and wincapi.c for crypt32 functions.
(cherry picked from commit 48db456801)
Conflicts:
Recipe
(cherry-picker's note: the conflict was just some context not looking
quite the same)
We set up a pair of bufchains for the standard input and output
exchanged with the proxy process, but forgot to clear them when the
Local_Proxy_Socket is cleaned up.
(cherry picked from commit bb66e9870e)
make_private_security_descriptor and a new function protectprocess().
protectprocess() opens the running PuTTY process and adjusts the
Everyone and user access control entries in its ACL to deny a
selection of permissions which malicious processes running as the same
user could use to hijack PuTTY.
(cherry picked from commit aba7234bc1)
Half the release checklist has changed recently, what with me
completely reworking the website and also writing all this release
automation. I think these are all the checklist changes needed now the
dust has settled, though of course when I do the next actual release I
expect there'll turn out to be something I missed...
(cherry picked from commit 3e811b3dff)
I've added extra modes to release.pl which should automate the more
tedious parts of the deployment phase: uploading the release build to
all the places it needs to go, checking its integrity once it gets
there, verifying that everything can be downloaded again usefully,
checking content-types etc.
The new version should check more thoroughly (it checks the whole FTP
and HTTP download directories, so it will spot errors like failing to
update the FTP 'latest' symlink), and take fewer commands to run.
(cherry picked from commit f08e2de078)
The length coming back from ber_read_id_len might have overflowed, so
treat it as potentially negative. Also, while I'm here, accumulate it
inside ber_read_id_len as an unsigned, so as to avoid undefined
behaviour on integer overflow, and toint() it before return.
Thanks to Hanno Böck for spotting this, with the aid of AFL.
(cherry picked from commit 5b7833cd47)
Conflicts:
import.c
(cherry-picker's note: resolving the conflict involved removing an
entire section of the original commit which fixed ECDSA code not
present on this branch)
Forgot that a zero-length string might have come back from fgets.
Thanks to Hanno Böck for spotting this, with the aid of AFL.
(cherry picked from commit 5815d6a65a)
The initial test for a line ending with "PRIVATE KEY-----" failed to
take into account the possibility that the line might be shorter than
that. Fixed by introducing a new library function strendswith(), and
strstartswith() for good measure, and using that.
Thanks to Hanno Böck for spotting this, with the aid of AFL.
(cherry picked from commit fa7b23ce90)
Conflicts:
misc.c
misc.h
(cherry-picker's note: the conflicts were only due to other functions
introduced on trunk just next to the ones introduced by this commit)
