Fix bug introduced in rev 1.467 which causes
"buffer_get_bignum_ret: incomplete message" errors when built with WITH_SSH1
and run such that no Protocol 1 ephemeral host key is generated (eg "Protocol
2", no SSH1 host key supplied). Reported by rainer.laatsch at t-online.de,
ok deraadt@
Darren Tucker [Tue, 2 Aug 2016 02:16:34 +0000 (12:16 +1000)]
Explicitly test for broken strnvis.
NetBSD added an strnvis and unfortunately made it incompatible with the
existing one in OpenBSD and Linux's libbsd (the former having existed
for over ten years). Despite this incompatibility being reported during
development (see http://gnats.netbsd.org/44977) they still shipped it.
Even more unfortunately FreeBSD and later MacOS picked up this incompatible
implementation. Try to detect this mess, and assume the only safe option
if we're cross compiling.
Skip passwords longer than 1k in length so clients can't
easily DoS sshd by sending very long passwords, causing it to spend CPU
hashing them. feedback djm@, ok markus@.
Brought to our attention by tomas.kuthan at oracle.com, shilei-c at
360.cn and coredump at autistici.org
Reduce timing attack against obsolete CBC modes by always
computing the MAC over a fixed size of data. Reported by Jean Paul
Degabriele, Kenny Paterson, Torben Hansen and Martin Albrecht. ok djm@
Darren Tucker [Thu, 21 Jul 2016 04:17:31 +0000 (14:17 +1000)]
Search users for one with a valid salt.
If the root account is locked (eg password "!!" or "*LK*") keep looking
until we find a user with a valid salt to use for crypting passwords of
invalid users. ok djm@
Darren Tucker [Mon, 18 Jul 2016 07:22:49 +0000 (17:22 +1000)]
Explicitly specify source files for regress tools.
Since adding $(REGRESSLIBS), $? is wrong because it includes only the
changed source files. $< seems like it'd be right however it doesn't
seem to work on some non-GNU makes, so do what works everywhere.
Add some unsigned overflow checks for extra_pad. None of
these are reachable with the amount of padding that we use internally.
bz#2566, pointed out by Torben Hansen. ok markus@
Darren Tucker [Sun, 17 Jul 2016 23:33:25 +0000 (09:33 +1000)]
Handle PAM_MAXTRIES from modules.
bz#2249: handle the case where PAM returns PAM_MAXTRIES by ceasing to offer
password and keyboard-interative authentication methods. Should prevent
"sshd ignoring max retries" warnings in the log. ok djm@
It probably won't trigger with keyboard-interactive in the default
configuration because the retry counter is stored in module-private
storage which goes away with the sshd PAM process (see bz#688). On the
other hand, those cases probably won't log a warning either.
Darren Tucker [Fri, 15 Jul 2016 04:48:30 +0000 (14:48 +1000)]
Move VA_COPY macro into compat header.
Some AIX compilers unconditionally undefine va_copy but don't set it back
to an internal function, causing link errors. In some compat code we
already use VA_COPY instead so move the two existing instances into the
shared header and use for sshbuf-getput-basic.c too. Should fix building
with at lease some versions of AIX's compiler. bz#2589, ok djm@
Add a ProxyJump ssh_config(5) option and corresponding -J
ssh(1) command-line flag to allow simplified indirection through a SSH
bastion or "jump host".
These options construct a proxy command that connects to the
specified jump host(s) (more than one may be specified) and uses
port-forwarding to establish a connection to the next destination.
This codifies the safest way of indirecting connections through SSH
servers and makes it easy to use.
Darren Tucker [Fri, 15 Jul 2016 03:49:44 +0000 (13:49 +1000)]
Mitigate timing of disallowed users PAM logins.
When sshd decides to not allow a login (eg PermitRootLogin=no) and
it's using PAM, it sends a fake password to PAM so that the timing for
the failure is not noticeably different whether or not the password
is correct. This behaviour can be detected by sending a very long
password string which is slower to hash than the fake password.
Mitigate by constructing an invalid password that is the same length
as the one from the client and thus takes the same time to hash.
Diff from djm@
Darren Tucker [Fri, 15 Jul 2016 03:32:45 +0000 (13:32 +1000)]
Determine appropriate salt for invalid users.
When sshd is processing a non-PAM login for a non-existent user it uses
the string from the fakepw structure as the salt for crypt(3)ing the
password supplied by the client. That string has a Blowfish prefix, so on
systems that don't understand that crypt will fail fast due to an invalid
salt, and even on those that do it may have significantly different timing
from the hash methods used for real accounts (eg sha512). This allows
user enumeration by, eg, sending large password strings. This was noted
by EddieEzra.Harari at verint.com (CVE-2016-6210).
To mitigate, use the same hash algorithm that root uses for hashing
passwords for users that do not exist on the system. ok djm@
Add missing "recvfd" pledge promise: Raf Czlonka reported
ssh coredumps when Control* keywords were set in ssh_config. This patch also
fixes similar problems with scp and sftp.
Improve crypto ordering for Encrypt-then-MAC (EtM) mode
MAC algorithms.
Previously we were computing the MAC, decrypting the packet and then
checking the MAC. This gave rise to the possibility of creating a
side-channel oracle in the decryption step, though no such oracle has
been identified.
This adds a mac_check() function that computes and checks the MAC in
one pass, and uses it to advance MAC checking for EtM algorithms to
before payload decryption.
Reported by Jean Paul Degabriele, Kenny Paterson, Torben Hansen and
Martin Albrecht. feedback and ok markus@
Explicitly check for 100% completion to avoid potential
floating point rounding error, which could cause progressmeter to report 99%
on completion. While there invert the test so the 100% case is clearer. with
& ok djm@
Darren Tucker [Mon, 20 Jun 2016 05:55:34 +0000 (15:55 +1000)]
get_remote_name_or_ip inside LOGIN_NEEDS_UTMPX
Apply the same get_remote_name_or_ip -> session_get_remote_name_or_ip
change as commit 95767262 to the code inside #ifdef LOGIN_NEEDS_UTMPX.
Fixes build on AIX.
Remove "POSSIBLE BREAK-IN ATTEMPT!" from log message
about forward and reverse DNS not matching. We haven't supported IP-based
auth methods for a very long time so it's now misleading. part of bz#2585,
ok markus@
Darren Tucker [Tue, 14 Jun 2016 03:55:12 +0000 (13:55 +1000)]
Track skipped upstream commit IDs.
There are a small number of "upstream" commits that do not correspond to
a file in -portable. This file tracks those so that we can reconcile
OpenBSD and Portable to ensure that no commits are accidentally missed.
If you add something to .skipped-commit-ids please also add an upstream
ID line in the following format when you commit it.
Back out rev 1.28 "Check min and max sizes sent by the
client" change. It caused "key_verify failed for server_host_key" in clients
that send a DH-GEX min value less that DH_GRP_MIN, eg old OpenSSH and PuTTY.
ok djm@
Darren Tucker [Tue, 14 Jun 2016 00:48:27 +0000 (10:48 +1000)]
Use Solaris setpflags(__PROC_PROTECT, ...).
Where possible, use Solaris setpflags to disable process tracing on
ssh-agent and sftp-server. bz#2584, based on a patch from huieying.lee
at oracle.com, ok djm.
Fix two rare edge cases: 1. If vasprintf() returns < 0,
do not access a NULL pointer in snmprintf(), and do not free() the pointer
returned from vasprintf() because on some systems other than OpenBSD, it
might be a bogus pointer. 2. If vasprintf() returns == 0, return 0 and ""
rather than -1 and NULL.
Besides, free(dst) is pointless after failure (not a bug).
One half OK martijn@, the other half OK deraadt@;
committing quickly before people get hurt.
The function update_progress_meter() calls refresh_progress_meter()
which calls snmprintf() which calls malloc(); but update_progress_meter()
acts as the SIGALRM signal handler.
"malloc(): error: recursive call" reported by sobrado@.
Fix two rare edge cases: 1. If vasprintf() returns < 0,
do not access a NULL pointer in snmprintf(), and do not free() the pointer
returned from vasprintf() because on some systems other than OpenBSD, it
might be a bogus pointer. 2. If vasprintf() returns == 0, return 0 and ""
rather than -1 and NULL.
Besides, free(dst) is pointless after failure (not a bug).
One half OK martijn@, the other half OK deraadt@;
committing quickly before people get hurt.
To prevent screwing up terminal settings when printing to
the terminal, for ASCII and UTF-8, escape bytes not forming characters and
bytes forming non-printable characters with vis(3) VIS_OCTAL. For other
character sets, abort printing of the current string in these cases. In
particular, * let scp(1) respect the local user's LC_CTYPE locale(1); *
sanitize data received from the remote host; * sanitize filenames, usernames,
and similar data even locally; * take character display widths into account
for the progressmeter.
This is believed to be sufficient to keep the local terminal safe
on OpenBSD, but bad things can still happen on other systems with
state-dependent locales because many places in the code print
unencoded ASCII characters into the output stream.
Using feedback from djm@ and martijn@,
various aspects discussed with many others.
deraadt@ says it should go in now, i probably already hesitated too long