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>
Daniel Lenski [Mon, 3 May 2021 21:27:24 +0000 (14:27 -0700)]
GP: Pass 'preferred-ipv6' parameter among auth requests, just like 'preferred-ip'
Testing against a real GP server with IPv6 support shows that the
"preferred" IPv6 address can be passed in the /ssl-vpn/login.esp request
arguments, as well as returned by it. (Even if unprompted; some GP VPNs
seem to track a persistent mapping of user accounts to IP addresses on the
server side.)
We should save the 'preferred-ipv6' parameter from the login results in
vpninfo->cookie, just as we do with its Legacy IP counterpart,
'preferred-ip'.
Also adds Flask tests to verify the correct passing of
'preferred-ip'/'-ipv6'.
Daniel Lenski [Thu, 29 Apr 2021 18:08:20 +0000 (11:08 -0700)]
GP: fix bug in blind retry of login credentials after portal-to-gateway redirect
We had been incorrectly relying on the first character of the 'auth_id'
being '_' to indicate a non-challenge form, in which case the
username/password can be "blindly retried" from portal to gateway.
However, this has been wrong since v8.09 (specifically, the commit 593df6b1c09ea525a913d4d8401a95ffdb1877db). Unfortunately, it may be
responsible for some user reports of inability to login via portal
interface.
Discovered while writing gp-auth-and-config tests.
Daniel Lenski [Thu, 29 Apr 2021 18:15:20 +0000 (11:15 -0700)]
GP auth: don't modify URL path if it ends with .esp
If the URL path ends with .esp (possibly followed by a query string, e.g.
/ssl-vpn/prelogin.esp?magic_parameter=123), then let's assume that the user
knows exactly what they're doing and that we shouldn't rewrite the path.
This will help with GP auth tests, by allowing us to get parameters into the
test session setup (just as fake-{f5,fortinet,juniper}-server.py do), in
order to configure gateways, 2FA requirement, etc.
Daniel Lenski [Wed, 28 Apr 2021 18:53:02 +0000 (11:53 -0700)]
Add fake-gp-server.py and gp-auth-and-config test
Another set of Flask-based tests.
TODO: need a way to get parameters into the initial session setup (just as
fake-{f5,fortinet,juniper}-server.py do), in order to configure gateways,
2FA requirement, etc.
Daniel Lenski [Fri, 23 Apr 2021 02:49:53 +0000 (19:49 -0700)]
GlobalProtect IPv6 ESP support
ESP for IPv6 is be initiated in the same way as for Legacy IP: send a
ping (ICMPv6 echo request) to the "magic address" from <gw-address-v6>,
with the same "magic payload."
With a hop limit of 128 and random ICMPv6 ID (I think), this exactly
matches what the Windows client sends out. (Good job by me with the PAN
GlobalProtect mind-meld. 🧠😝)
Based on my testing, it appears that the resulting tunnel works for
*both* IPv4-over-ESP and IPv6-over-ESP. (Whew! No need to do 2 separate
sets of "magic pings.") It appears that when `<gw-address-v6/>` is
present in the config, we *must* send the magic pings via ICMPv6 to that
address (rather than ICMPv4 to the `<gw-address/>`) in order for the
IPv6-over-ESP tunnel to work.
Given the pre-existing weirdness of GP ESP, it could be worse. ¯\_(ツ)_/¯
[ dwmw2: Fix type aliasing abuse, update for new ip_info handling ]
From the original separate commit fixing the aliasing issues:
I believe we get away with it for the address fields and the icmph
because all those structures have uint16_t members, and the compiler
realises that in valid C code, those *might* alias to the uint16_t*
argument to csum_partial().
But the pseudo header, being an array of bytes in Dan's original
version, couldn't *possibly* have been what csum_partial() was going to
look at, so the compiler was free to reorder the writes into it so they
happen *after* the call to csum_partial(). With predictably unhappy
results.
Setting it up as a type which some uint16_t members would make it work,
but really there's not a lot of point. Just add the length and the
next_header field directly to the checksum.
Signed-off-by: Daniel Lenski <dlenski@gmail.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Tue, 27 Apr 2021 21:48:23 +0000 (14:48 -0700)]
GP config: hush warning about unknown <quarantine>no</quarantine>
Commit 958da82445d ("Warn if <quarantine> is set in GlobalProtect XML
config") attempted to avoid emitting the warning for the value "no".
But the way the condition was constructed, it left the "quarantine"
node unhandled and triggered the later 'else' condition reporting it
as an 'Unknown GlobalProtect config tag'.
Fix it so that it really does silently ignore the "no" case.
Daniel Lenski [Fri, 23 Apr 2021 01:58:03 +0000 (18:58 -0700)]
Split ESP checksum functions into csum_partial and csum_finish
csum_partial() feeds more uint16_t words into the checksummer;
csum_finish() does the final condensation down to uint16_t, and
conversion to network order.
[dwmw2: Note this still doesn't cope with odd-sized packets but
we don't care for now.]
David Woodhouse [Tue, 27 Apr 2021 23:14:52 +0000 (00:14 +0100)]
Attempt to allow Fortinet reconnect over TCP
We can't just reconnect; we have to fetch the XML config again. Even that
isn't always sufficient; it seems the server only allows *one* reconnect
before it starts to fail.
The *first* reconnection shows this in server logs:
[2326:root:0]ipcp: down ppp:0x7f1e1d218000 caller:0x7f1e1d15f900 tun:37
[2326:root:78]sslvpn_ppp_deassociate_fd_to_ipaddr:318 deassociate 10.212.134.200 to tun (ssl.root:37)
[2326:root:78]tunnel is down, wait for next connection.
[2326:root:78]sslvpn_release_dynip:1309 free app session, idx[0]
...
[2326:root:79]req: /remote/fortisslvpn_xml
[2326:root:79]deconstruct_session_id:426 decode session id ok, user=…
[2326:root:79]deconstruct_session_id:426 decode session id ok, user=…
[2326:root:79]sslvpn_reserve_dynip:1275 tunnel vd[root] ip[10.212.134.200] app session idx[0]
[2326:root:79]req: /remote/sslvpn-tunnel
[2326:root:79]sslvpn_tunnel_handler,52, Calling rmt_conn_access_ex.
[2326:root:79]deconstruct_session_id:426 decode session id ok, user=…
[2326:root:79]sslvpn_tunnel_handler,153, Calling tunnel.
[2326:root:79]tunnelEnter:498 0x7f1e1d15f900:0x7f1e1d1e9000 sslvpn user[dwmw2],type 1,logintime 0 vd 0
[2326:root:79]sconn 0x7f1e1d15f900 (0:root) vfid=0 local=[178.238.156.110] remote=[90.155.92.213] dynamicip=[10.212.134.200]
[2326:root:79]Prepare to launch ppp service...
The *second* reconnection doesn't say anything about waiting for next
connection:
David Woodhouse [Tue, 27 Apr 2021 23:10:31 +0000 (00:10 +0100)]
Abort if PPP transport is closed in PPPS_ESTABLISH
The Fortinet server just closes the connection on us when it screws up
reconnection and thinks it can't allocate an IP address. Don't retry
ad infinitum; give up.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 27 Apr 2021 23:08:45 +0000 (00:08 +0100)]
Only set ip_info addresses from PPP if they aren't already set
Fortinet reconnection, in particular, didn't like the addr6 being set
because it *isn't* set in the new ip_info we receive in XML; only the
netmask6 is set there.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Tue, 27 Apr 2021 18:20:42 +0000 (11:20 -0700)]
Fix Fortinet IPv6 config and add tests for it
1. Fortinet IPv6 config uses 'prefix-len' attributes where the Legacy IPv4
config uses 'mask' attributes.
2. Also, we need to set ip_info.netmask6 instead of ip_info.addr6 if it
includes a prefix-len (for compatibility with the vpnc-script, which was
originally written based on Cisco's dichotomy here).
3. Align the cstp_option name, "ipaddr6", with other protocols (cf. 792647ee1b9a1eca9cffc79dab746f8273d2279a).
4. Add IPv6 config to Fortinet test server:
projects / users / dwmw2 / openconnect.git / log