David Woodhouse [Tue, 15 Jun 2021 12:02:49 +0000 (13:02 +0100)]
Support non-AEAD ciphersuites in DTLSv1.2 with GnuTLS
We have encountered a Cisco server in the wild which appears only to
support the legacy ciphersuites. And since we offer a set of DTLSv1.2
ciphers it doesn't fall back to accepting the DTLSv1.0 offer; we end
up with no DTLS at all.
This should fix #249.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Sat, 29 May 2021 17:39:57 +0000 (10:39 -0700)]
Mark obsolete-server-crypto test as XFAIL in Fedora/GnuTLS/* CI
The system-wide minimum crypto policy on Fedora prevents us from enabling
3DES and RC4 ciphers via GnuTLS priority strings. We have unconditionally
disabled it in OpenConnect for now in commit 7e862f2f03 but the
obsolete-server-crypto test is *still* failing, with ocserv reporting
'GnuTLS error (at worker-vpn.c:861): No supported cipher suites have
been found.'
Just mark obsolete-server-crypto test as XFAIL for these builds. It's
the most accurate description of the state of those tests: these
environments do not provide OpenConnect with the capabilities to
reliably enable obsolete/insecure crypto algorithms in a self-contained
way.
See https://bugzilla.redhat.com/show_bug.cgi?id=1960763 for ongoing
discussions about how to come up with a more reliable, testable, and
maintainable mechanism for OpenConnect to enable these algorithms without
compromising the system-wide minimum crypto policy.
Signed-off-by: Daniel Lenski <dlenski@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Sat, 12 Jun 2021 07:50:09 +0000 (08:50 +0100)]
Unconditionally bypass system crypto policy
This makes me extremely sad, but they rolled it out with *no* way to
selectively allow the user to say "connect anyway", as we've always had
for "invalid" certificates, etc.
It's just unworkable and incomplete as currently implemented in the
distributions, so we have no choice except to bypass it and wait for
it to be fixed.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Thu, 10 Jun 2021 22:45:08 +0000 (15:45 -0700)]
Fix Fortinet realm name extraction
1. We were inadvertently capturing 6 characters following the 'realm'
parameter in the query string (e.g. '&lang='). Fix and include extra
parameters in tests to verify.
2. Add another comment about how the 'realm' field is saved in URL-escaped
form, and test to verify.
Daniel Lenski [Mon, 7 Jun 2021 22:46:23 +0000 (15:46 -0700)]
Fix PPP packets split across TLS records
This replicates the behavior of oncp_record_read(): if a (D)TLS record
ends with an incomplete PPP packet, we save the read-so-far length as
vpninfo->partial_rec_size, and continue reading at that point on the
next iteration.
This poorly-layered packetisation behavior was observed on Fortinet and
may exist for other encapsulations as well. So we cope with it for
PPP_ENCAP_F5 because it's trivial, but not yet PPP_ENCAP_F5_HDLC which
would be harder; we'll wait until we actually see it there before
delving into that complexity.
This works as expected to prevent loss of synchronization to the
encapsulation header. Here's an example showing receipt of incomplete
Fortinet/PPP packets, during a high rate of incoming packets generated with
'iperf3 -c $IP_ON_REMOTE_NETWORK -R', followed by recovery on subsequent
mainloop iterations:
Received IPv4 data packet of 1183 bytes over TLS
Fortinet packet is incomplete (837 bytes on wire but header payload_len is 1185). Waiting for more.
Packet contains 539 bytes after payload. Assuming concatenated packet.
Received IPv4 data packet of 1183 bytes over TLS
Fortinet packet is incomplete (539 bytes on wire but header payload_len is 1185). Waiting for more.
No work to do; sleeping for 10000 ms...
No work to do; sleeping for 10000 ms...
No work to do; sleeping for 10000 ms...
No work to do; sleeping for 10000 ms...
Packet contains 13101 bytes after payload. Assuming concatenated packet.
Received IPv4 data packet of 1183 bytes over TLS
Packet contains 11910 bytes after payload. Assuming concatenated packet.
Received IPv4 data packet of 1183 bytes over TLS
Packet contains 10719 bytes after payload. Assuming concatenated packet.
Received IPv4 data packet of 1183 bytes over TLS
Packet contains 9528 bytes after payload. Assuming concatenated packet.
Signed-off-by: Daniel Lenski <dlenski@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Tue, 8 Jun 2021 18:13:39 +0000 (11:13 -0700)]
Rename oncp_rec_size → partial_rec_size
This variable is used to track the size of partially-received tunnel packets
across mainloop invocations.
It was intially used by the oNCP protocol, where tunneled packets can be
split across TLS records. We're now going to use it to deal with an
extremely similar mis-layering of packetisation for PPP-based protocols
as well.
Renaming it to partial_rec_size to emphasize its cross-protocol nature (cf.
https://gitlab.com/openconnect/openconnect/-/merge_requests/151).
Signed-off-by: Daniel Lenski <dlenski@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Mon, 7 Jun 2021 22:14:57 +0000 (15:14 -0700)]
Fix handling of concatenated PPP data packets
This 'continue' prevented us from ever actually processing a concatenated
packet if the first one was a *data* packet. Dan only tested this for
concatenation of config packets in the early stages of the connection. Bad
Dan.
Signed-off-by: Daniel Lenski <dlenski@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 8 Jun 2021 19:34:45 +0000 (20:34 +0100)]
Increase SO_SNDBUF on UDP socket
In commit 3444f811ae ("Set SO_SNDBUF on DTLS socket and handle -EAGAIN
on it") in 2013 I cut the socket wmem to just 2 packets in size, to fix
bufferbloat issues when handling VoIP streams simultaneously with bulk
uploads.
In #250 we discovered that this is a problem for high-bandwidth setups
because we aren't actually keeping the send buffers full. Even though
there's only ~100µs of latency between getting -EAGAIN on the UDP
socket, and finally sending the packet successfully...
... that's 100µs when the physical network device actually has nothing to
send. And it's happening over and over again as the UDP socket buffers
fill and then we poll for them to be sufficiently empty again.
This is exacerbated by the fact that ip_info.mtu is actually zero at
this point for PPP protocols — but only slightly because 2*1500 isn't
actually much more than the minimum value we get when we ask for zero
anyway.
It also doesn't help that the kernel deliberately doesn't wake a waiter
until they can make "significant" progress and the buffers are down to
*half* the sndbuf value (see sock_def_write_space() in net/core/sock.c).
Testing with 'iperf3 -u -b 900M' on my home network with 1Gb/s network
between client and (Fortinet) server, I find I have to have to use a
value of around 20000 (doubled to 40000) in order to avoid seeing drops
on the *tun* interface because we aren't moving packets fast enough.
We need to find a decent balance between high-bandwidth and bufferbloat,
so let's try the following: First "assume" 1500 for the MTU if it isn't
actually set, and then multiply by the configured queue len which
defaults to 10. That ought to be a reasonable compromise for bandwidth
vs. bufferbloat for the general case, *and* allows users to tweak it.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Mon, 31 May 2021 22:56:30 +0000 (15:56 -0700)]
Clarify 'Certificate Validation Failure' error from Cisco servers
Cisco servers send this ambiguous error string when the CLIENT certificate
is absent or incorrect. We rewrite it to make this clearer, while
preserving the original error as a substring. See #160.
Daniel Lenski [Fri, 28 May 2021 19:50:51 +0000 (12:50 -0700)]
Print an error message if dtls_addr is NULL in dtls_setup()
This should reduce confusion and help debugging in situations like #242,
where a Cisco server apparently does not return any 'X-DTLS-*' headers upon
connection.
We were printing this message in connect_dtls_socket(), but since
dtls_setup() gets called — and fails — *first* in that case, the message
never got printed.
Daniel Lenski [Thu, 20 May 2021 22:53:25 +0000 (15:53 -0700)]
Bugfix GlobalProtect ESP magic pings over Legacy IP
GlobalProtect IPv6 support was added in
https://gitlab.com/openconnect/openconnect/-/merge_requests/188, and
specifically support for initiating an ESP connection via ICMPv6 "magic
pings" in specifically 5b98b62883216cf9306f06c6b3c9dde81bcfe789.
Getting the ICMPv6 packets to have correct checksums was quite tricky (see
commit notes) and the commit was revised several times.
Somehow we managed to remove the pre-existing code to compute the checksum
correctly in the case of ICMPv4 "magic pings", leaving behind an ICMPv4
checksum that's always zero (and thus rejected by the server, and never
correctly initiates a connection).
Tom Carroll [Mon, 17 May 2021 05:58:24 +0000 (22:58 -0700)]
Use C99 initializer instead of memset.
Strictly speaking, using memset() here violates strict aliasing rules,
and it would be entirely permissible for an assert() like this example
to *fail*:
David Woodhouse [Wed, 19 May 2021 08:32:41 +0000 (09:32 +0100)]
Fix first line length in buf_append_base64()
We need to add four (for the characters we're about to append) *after*
checking against line_len and resetting ll to zero. Otherwise the first
line thinks it starts at 4 while the others start at 0.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Tom Carroll [Tue, 18 May 2021 06:58:23 +0000 (23:58 -0700)]
Correct calculation of base64 encode buffer length.
In the previous formulation, it would first multiple then divide. It would then
promote to unsigned int. The formula would overflow for large len. For example,
needed = 2 when len == INT_MAX. In the revised formulation, it is promoted,
divided, then multiplied. The revised calculation avoids the overflow and
computes needed correctly over len in {0, 1, ..., INT_MAX}. For len == INT_MAX,
needed is correctly computed as 2863311533.
Signed-off-by: Tom Carroll <incentivedesign@gmail.com>
Tom Carroll [Mon, 17 May 2021 05:56:57 +0000 (22:56 -0700)]
gnutls.c:943:21: warning: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Wsign-compare]
The gci->nr_certs field is an unsigned int. Use that as the loop variable
for freeing them too. And we don't actually need the if (gci->certs)
condition here either because ->nr_certs won't be non-zero in that case
anyway.
Signed-off-by: Tom Carroll <incentivedesign@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Tom Carroll [Wed, 12 May 2021 07:46:03 +0000 (00:46 -0700)]
Remove field free_certs from gtls_cert_info.
There was only one place in the code requiring special treatment supported by
free_cert == 0. Instead, make a copy of the certificate, transforming the case
to free_cert == 1. This allows the removal of free_certs from the gtls_cert_info
structure.
Signed-off-by: Tom Carroll <incentivedesign@gmail.com>
David Woodhouse [Fri, 14 May 2021 10:57:11 +0000 (11:57 +0100)]
Add openconnect_get_connect_url(), use it in --authenticate output
As noted in the long comment in openconnect.h, we need to provide the
actual *URL* for the connection including the real hostname. And that
means we also need to provide a suitable --resolve argument to pass on
to the connecting openconnect.
Add openconnect_get_connect_url() for the GUI auth-dialogs to use, and
make 'openconnect --authenticate' emit the same. If we just put it into
$HOST that might break existing setups that don't use the newly-added
$RESOLVE variable too, so put it in another new variable $CONNECT_URL
and leave the original $HOST alone.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 11 May 2021 12:45:18 +0000 (13:45 +0100)]
Add tests for TPMv2 with both swtpm and hardware
For normal builds, the auth-swtpm test runs a new swtpm with a temporary
state directory, imports a pregenerated TPM state which matches the
keys that are also pregenerated and committed to git.
As with the normal keys/certs and SoftHSM state, there are also rules
for *generating* all this, to ensure that what's committed is entirely
reproducible and serve as documentation. But those rules are mostly
dormant from this point onwards (hence my not caring about parallel
builds being broken for the swtpm startup).
There's an attempt at supporting *real* TPM testing, with a 'hwtpm'
variant. In that case the rules for generating the key/cert and for
importing the existing EC cert into the TPM are *not* dormant. But
this isn't quite working yet and is going to need to be an explicit
invocation by those who care about such things.
There are three keys tested here. Firstly the original EC key used
by all our certificate tests, imported into the TPM with the
create_tpm2_key tool from James's openssl_tpm2_engine. We cannot
import our original RSA key as it deliberately had a strange key
size, and the TPM can't cope.
Then we *generate* EC and RSA keys in the TPM too, using the
tpm2tss-genkey tool from the Intel engine. That one isn't capable
of importing existing keys yet but *is* more widely available in
distro packages.
For swtpm we use both since it's only necessary for the tools to be
present at the time we add all this. For hwtpm where users will want
to do this at build/test time, they get a subset of the keys depending
on which tools they have available.
We preserve the existing nomenclature for matching keys/certs, where
NAME-key-STORAGETYPE.pem matches with NAME-cert.pem. So the *imported*
version of our existing EC key ends up being ec-key-swtpm.pem and
ec-key-hwtpm.pem to match the existing ec-cert.pem
For the *generated* keys, we need a new 'NAME' since they are actually
new keys. So 'swtpm-ec' 'swtpm-rsa' 'hwtpm-ec' 'hwtpm-rsa' are the names
of those keys, and since they were generated in the TPM and never
existed as a file, we need a new pattern-match rule to generate the
corresponding CSR and then certificate, using the ENGINE to do so.
But we want this pattern rule *not* to match the imported keys like
ec-key-swtpm.pem, because we don't want to trigger a generation of
ec-cert.pem from the TPM when it's already right there in a file.
So we *don't* want to use 'swtpm' and 'hwtpm' as the STORAGETYPE part of
the filenames of the generated keys; we just use 'tpm' instead, and then
the pattern rule matches 'swtpm-%-key-tpm.pem' and 'hwtpm-%-key-tpm.pem'.
This doesn't yet test NV-parented keys or any kind of passwords, but
it's a start.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 13 May 2021 12:04:59 +0000 (13:04 +0100)]
Allow TPM_INTERFACE_TYPE=socsim to force swtpm even for Intel TSS
If we want to have a swtpm-based test, we need to *use* the swtpm even
if a real hardware TPM is available. Not for general purpose use, but
allow it to be overridden by using the same TPM_INTERFACE_TYPE variable
that already works for the IBM TSS because the IBM library handles it
internally.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Wed, 12 May 2021 15:19:07 +0000 (16:19 +0100)]
Implement RSA-PSS padding for TPMv2
Now I can connect using TLSv1.3 using a TPMv2 RSA key. And my list of
"stuff I should never have had to do for myself in the application,
just to ask the crypto library to use the key that the user pointed
it at" has *really* jumped the shark now.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 11 May 2021 20:49:33 +0000 (21:49 +0100)]
Tell TPMv2 the hash type based on size
The TPM doesn't really have any business knowing this, and the only thing
that matters is the size. Which is truncated to the curve bit length even
for larger hashes.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 11 May 2021 12:42:52 +0000 (13:42 +0100)]
tss2-esys: Don't try password for TPM2 keys with emptyauth set
The auth-certificate test always sets --key-password=password, and when
a TPM2 key has 'emptyauth' the IBM TSS code was trying the empty auth
first, as it should. But the Esys code was always trying the password,
and then prompting the user; the user had to just press enter.
Try empty auth first if the key says so.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 7 May 2021 16:05:59 +0000 (17:05 +0100)]
GnuTLS: Really only install certs from load_primary_certificate()
Finally actually finish the job: load_certificate() fetches the key
and certs into a data structure that its caller can use as it sees
fit, and the *caller* then installs them into the https_cred in
the case of the primary certificate.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 7 May 2021 15:47:48 +0000 (16:47 +0100)]
GnuTLS: Split out free_gtls_cert_info()
If we want load_certificate() to actually *return* this stuff to its
caller for real and then load_primary_certificate() to actually install
them into the https_cred, then we're going to have to be able to free
it sanely.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 7 May 2021 14:44:37 +0000 (15:44 +0100)]
OpenSSL: Pass certinfo through load_certificate() functions
Much like in GnuTLS except in GnuTLS we already separated out the bit
which installs the key/certs into the https_cred while all these
OpenSSL functions are still just installing directly into the SSL_CTX
so we're going to want to fix that separately.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 7 May 2021 14:24:51 +0000 (15:24 +0100)]
GnuTLS: Pass certinfo into load_certificate() and subordinate functions
Instead of blindly referencing vpninfo->certinfo[0], pass the required
certinfo pointer into load_certificate() and down to the various
subfunctions which get invoked from there.
A couple of places still need fixing; notably gnutls_pin_callback() and
auth_tpm2_key(). Both of which are *callbacks*. I think we'll end up
adding a self-referencing vpninfo pointer to the struct cert_info, then
using the cert_info as the 'private' data for those callbacks. But that
can come later.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 7 May 2021 13:53:45 +0000 (14:53 +0100)]
GnuTLS: Start to factor out load_certificate() for reuse
Introduce a struct with the key/cert information and start pretending
that the load_certificate() function is only obtaining those, and add a
load_primary_certificate() function which will actually be responsible
for installing them into vpninfo->https_cred.
This is step 1 which isn't even pretending very hard; it's just adding
the structure and doing a search/replace to use the fields therein
instead of local variables in load_certificate().
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 7 May 2021 10:33:31 +0000 (11:33 +0100)]
kill redundant free_certs argument to GnuTLS assign_privkey() function
This was added in commit 04ccc265c ("Simplify extra_certs handling
w.r.t. assign_privkey()") because GnuTLS 2 didn't take a copy of the
certs which were assigned to the creds, and we needed to keep track of
which extra certs were used and which weren't.
The GnuTLS 3 variant of assign_privkey() didn't use it, since GnuTLS 3
takes a copy of the certs and we can just free them normally.
Now that we've dropped GnuTLS 2 support, we can drop this argument too
and simplify assign_privkey() a little bit.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 4 May 2021 17:58:42 +0000 (18:58 +0100)]
Implement DTLS support for Array
This is fairly straightforward once you spot the "13" at the end of the
first DTLS packet. Without that, we can *send* data and even do keepalives
but the worker process on the server would crash the moment it has actual
*packets* to send back to us.
Like fairly much every other SSL VPN using anonymous DTLS, they don't
cope well with that first DTLS packet (or the response to it) being
lost. We don't get anything back if we resend it. And if we send a
keepalive that *might* elicit a response... or might cause them to tear
down the connection if they haven't seen the initial auth packet yet.
So... we send only the auth packet first. If we don't see a response we
send the auth packet *and* a keepalive in quick succession. If we start
seeing IP packets (which includes the keepalive as that's Legacy IP
protocol #0xff), we jump to DTLS_ESTABLISHED state too.,
We also need to send a 'dtls off' packet when we want it to switch back
to sending over TCP, much like the oNCP queue_esp_control().
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Sat, 1 May 2021 18:48:57 +0000 (19:48 +0100)]
json: Fix undefined behaviour when converting integer to double
Coverity doesn't like this, complaining that assignment to an object
with overlapping storage without exact overlap and compatible types can
cause undefined behaviour.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Wed, 5 May 2021 17:18:55 +0000 (10:18 -0700)]
Improve Fortinet auth
Use the same 'auth_id' values as GlobalProtect uses ('_login' for the normal
username/password form, and '_challenge' for the MFA challenge form). This
is a follow-on to 613fa87dd64853a042d982aca4a94279cd78ff58.
This also fixes:
- A potential double-free() issue if the challenge form is loaded repeatedly,
adding a test of 2 rounds of challenge 2FA to verify it.
- Warnings about the unused 'invalid_cookie' label.
David Woodhouse [Tue, 4 May 2021 16:05:49 +0000 (17:05 +0100)]
openssl: Add SSL_OP_LEGACY_SERVER_CONNECT to allow-insecure-crypto
OpenSSL 3.0.0 onwards will require secure negotiation by default, which
Cisco servers don't seem to cope with. Let --allow-insecure-crypto turn
that off.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Tue, 4 May 2021 20:16:10 +0000 (13:16 -0700)]
Keep comments next to live code in fortinet.c
In c0ca8c632566bf423d3ac23b8b21ef353e399c33, the fetch of the legacy/HTML
Fortinet config was preprocessor'ed out. The comment about the subtle
behavior of unfetched redirects should be kept next to live code.
Daniel Lenski [Tue, 4 May 2021 20:16:02 +0000 (13:16 -0700)]
Replace all use of inet_ntoa() with inet_ntop()
inet_ntoa() uses a statically-allocated buffer, which means that it wouldn't
be safe for use by an a multithreaded application using libopenconnect (as
yet hypothetical).
We should use inet_ntop() instead in all cases, even though it's slightly
less convenient.
David Woodhouse [Mon, 3 May 2021 19:34:16 +0000 (20:34 +0100)]
Fix setting of IP addresses in ip_info from PPP
We were clearing vpninfo->ip_info.addr if it was already set, as shown in
https://github.com/adrienverge/openfortivpn/issues/112#issuecomment-831471948
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
projects / users / dwmw2 / openconnect.git / log