David Woodhouse [Sun, 29 Dec 2019 13:58:10 +0000 (13:58 +0000)]
pulse: Attempt to handle EAP-TTLS fragmentation
This seems entirely gratuitous but has been seen in the wild, with a
server bizarrely fragmenting a TTLS message of only 3637 bytes, into a
first fragment of 3550 bytes.
We limit the individual IF-T/TLS fragments to 16KiB which is the TLS
record length, because we've never seen IF-T/TLS messages which aren't
precisely aligned in a TLS record (although NC did do horrible things
there).
Each call to pulse_eap_ttls_recv() will receive data from only a single
fragment; another call will be needed to send the Acknowledge frame and
then receive data from the next fragment.
Also implement outbound fragmentation with a fragment limit of 8KiB,
in case it's ever needed.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Mon, 14 Oct 2019 14:08:53 +0000 (15:08 +0100)]
When select() returns with errno == EINTR, that isn't an error
This stopped us from actually sending the BYE packet and closing the
session cleanly on exit.
Fixes: a07183b79f ("Check select() return value in main loop") et al.
This is why we don't blindly "fix warnings" reported by tools like
Coverity, and should sometimes be a little more reticent, and only
actually fix *bugs* that are highlighted.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Mon, 14 Oct 2019 13:07:11 +0000 (14:07 +0100)]
Fix pulse session kill request
Fixing leaks is good. Fixing them by freeing a string we were about to
use and then setting it to NULL, thus triggering a later check to report
-ENOMEM, less good.
Stupid dwmw2, no biscuit.
Fixes: 097586fe ("Fix leaks in Pulse duplicate session handling") Signed-off-by: David Woodhouse <dwmw2@infradead.org>
John Spencer [Wed, 9 Oct 2019 15:41:23 +0000 (16:41 +0100)]
Fix build with libressl 2.7.x/2.9.x
rather than hardcoding version numbers with ifdefs, we simply check
whether the functionality is available or not.
[dwmw2: Use #ifndef instead of #if !HAVE_SSL_CIPHER_FIND] Signed-off-by: John Spencer <maillist-openconnect@barfooze.de> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
GlobalProtect: Ensure timeout is less than DPD when DTLS connecting
When transitioning from DTLS_CONNECTING to DTLS_CONNECTED ensure that
the current timeout is less than or equal to 10-second DTLS DPD
otherwise timeout might be greater than 2x DPD, eg set to 60-second
DTLS attempt period from the ESP main loop where we were "connecting",
and we might sleep right through the DTLS DPD period and falsely
detect a dead peer and needlessly fall back to HTTPS.
This is only relevant to reconnects because during the initial
connection the timeout is artificially set low, ie 1 second, by the
OpenConnect mainloop because the TUN device is not yet up.
David Woodhouse [Fri, 11 Oct 2019 15:32:29 +0000 (16:32 +0100)]
Correct handling of empty responses to HTTP requests
If there's nothing to return, read_http_header() doesn't append anything
at all to hdrbuf, and it ends up with hdrbuf->data still NULL.
As long as *something* has once been added to it, there is a guarantee
that it's OK to use hdrbuf->data as a NUL-terminated string, as we do
when looking for HTTP/1.x responses.
Spotted when I just hit 'enter' when testing with openssl s_server.
Fixes: 093de80507 ("Rework HTTP header fetching to receive into oc_text_buf") Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 10 Oct 2019 11:20:17 +0000 (12:20 +0100)]
Silence Coverity warning in oncp_obtain_cookie()
For some reason Coverity doesn't realise that if buf_alloc() fails that's
only because it didn't allocate.
I suppose in that case, perhaps I should only have checked for !resp_buf
when checking for error, instead of using buf_error(resp_buf). Doing that
would probably make Coverity shut up too, but instead just let it use the
existing 'goto out' error handling.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 10 Oct 2019 10:49:46 +0000 (11:49 +0100)]
Clean up openconnect_protos handling
Do it with a bit less pointer arithmetic and it won't need a sentinel
at the end of the array.
It's a little bit cleaner, and hopefully Coverity will shut up about
leaks from openconnect_get_supported_protocols() potentially returning
non-negative after allocating the returned pointer.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 10 Oct 2019 00:03:22 +0000 (01:03 +0100)]
Fix potential leak of saml_path in GlobalProtect parse_prelogin_xml()
Coverity spotted the leak if we see two saml-request nodes. As far as
I can tell, it didn't spot the bogus realloc() usage which would also
have caused a leak and a crash.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Wed, 9 Oct 2019 22:36:03 +0000 (23:36 +0100)]
Fix potential NULL dereference in GlobalProtect XML parsing
Spotted by Coverity.
Perhaps the intent in reusing xml_node here for the inner loop was that
by leaving it NULL we'd automatically break out of the outer loop too,
which is reasonable. Except it wouldn't have done that because it'd
have died on trying to move to xmlnode->next. Break out of the outer
loop explicitly, instead.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Wed, 9 Oct 2019 14:27:17 +0000 (15:27 +0100)]
cstp: Tweak DTLS header matching
Coverity doesn't like the (i = 7) && … construct and thinks it's an
"assign instead of compare" bug. Which it isn't; the extra brackets
around it were enough to stop the compiler from making the same complaint.
Try doing it differently, purely for Coverity's benefit.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Wed, 9 Oct 2019 14:21:01 +0000 (15:21 +0100)]
oncp: Tear down ESP if receiving new config fails
Spotted by Coverity as an 'unused value' because the return value from
oncp_receive_espkeys() was ignored.
Use oncp_esp_close() to do the shutdown, and fix it only to send the
'ESP off' message to the server if an 'ESP on' message has previously
been sent (i.e. if vpninfo->dtls_state >= DTLS_CONNECTING).
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Wed, 9 Oct 2019 11:59:22 +0000 (12:59 +0100)]
Fix undefined shift in decode_base32() for invalid input
In the case where fewer than 2 characters were present in a base32 group,
Coverity complained of an undefined shift. Return -EINVAL early for this
case instead of triggering the undefined behaviour.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 8 Oct 2019 14:56:36 +0000 (15:56 +0100)]
Silence Coverity warning in cancellable_connect()
It's whining that we are reading a single byte into an int variable.
Which would be wrong if we were doing anything with it, and is often
a sign of little-endian-itis. As it happens it's harmless here because
it's only an attempt to capture the correct error condition by reading
from a defunct socket. But fix it anyway. There was no need for the
mismatch.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 8 Oct 2019 14:12:54 +0000 (15:12 +0100)]
Avoid strcpy() in Esys install_tpm_passphrase
Coverity didn't like it, because it couldn't see that we had actually
checked the length manually. By doing it like this we get to use
free_pass() on the full initial password instead of just the truncated
part, so it's not just a bogus "warning fix".
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 8 Oct 2019 12:11:56 +0000 (13:11 +0100)]
Clean up GnuTLS token info more
Coverity is still whining. I don't think a zero-length memset is actually
a problem, but again it does no harm to be careful, and we might as well
consolidate it all into one function instead of duplicating the code.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 8 Oct 2019 10:42:49 +0000 (11:42 +0100)]
Coverity cosmetics
Put the branch/version information into the description so it's visible
in the Coverity snapshot information, and put Java first so it doesn't
mess up the pathnames.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 8 Oct 2019 09:35:55 +0000 (10:35 +0100)]
Explicitly check for PKCS#11 token info overrun
Could probably hever have happened but Coverity doesn't like it, and it
doesn't hurt to err on the side of caution. We're already carefully
checking for weird behaviour from old versions of GnuTLS anyway.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Mon, 7 Oct 2019 17:39:32 +0000 (18:39 +0100)]
openssl: Fix certificate load failure harder
Note to self: Don't do half-arsed fixes just to test the Coverity/CI
integration while on a conference call, without actually paying full
attention.
We needed to actually return -EINVAL here, not just set ret = -EINVAL
which *still* left us using 'f' after closing it, and introduced a
*new* Coverity 'Unused value' defect.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Sun, 29 Sep 2019 20:29:26 +0000 (13:29 -0700)]
another GP error string that tells the client to stop trying to reconnect
As shown in https://gitlab.com/openconnect/openconnect/issues/78, the
message "Allow Automatic Restoration of SSL VPN is disabled" in a
GlobalProtect error response indicates that the server will not accept the
previously-valid auth cookie, so the client should give up retrying.
Daniel Lenski [Sun, 29 Sep 2019 20:36:44 +0000 (13:36 -0700)]
Fix double-free when client repeatedly fails to pull GlobalProtect client config
When openconnect attempts to rebuild the GP connection, upon rekey or
loss-of-connectivity, it re-requests the client configuration XML
(/ssl-vpn/getconfig.esp). It saves the old `cstp_options` prior to querying
the new ones, and then free()'s them after verifying that the IP addresses
and netmasks haven't changed.
If the config request fails to return valid XML twice in a row, the old
`cstp_options` would be double-freed, causing the crash described in
https://gitlab.com/openconnect/openconnect/issues/78.
The fix is to ensure that the old `cstp_options` are set to NULL as soon as
they're copied into `old_cstp_options`.
David Woodhouse [Tue, 10 Sep 2019 16:30:12 +0000 (17:30 +0100)]
Fix buffer overflow with chunked HTTP handling (CVE-2019-16239)
Over a decade ago, I was vocally sad about the fact that I needed to
implement HTTP client code for myself because none of the available
options at the time gave me sufficient control over the underlying
TLS connection.
This is why. A malicious HTTP server (after we have accepted its
identity certificate) can provide bogus chunk lengths for chunked
HTTP encoding and cause a heap overflow.
Reported by Lukas Kupczyk of the Advanced Research Team at CrowdStrike
Intelligence.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Omar Sandoval [Tue, 27 Aug 2019 16:27:51 +0000 (09:27 -0700)]
Don't crash if gnutls_x509_crt_list_import() fails
On error, gnutls_x509_crt_list_import() deinitializes any certificates
that it loaded (this isn't documented, of course, but see [1]). However,
we're also deinitializing them in the error handling case, resulting in
a double-free. Set nr_extra_certs to zero in that case so that we don't
crash.
Daniel Lenski [Wed, 28 Aug 2019 23:21:03 +0000 (16:21 -0700)]
GlobalProtect: try to connect to portal interface before gateway
This makes OpenConnect behave more like the official GP clients, which
should make more sense to new users especially when troublesheeting, without
removing the useful ability to connect directly to a gateway.
(See https://gitlab.com/openconnect/openconnect/merge_requests/56#note_209428777)
Corey Wright [Sun, 11 Aug 2019 10:00:04 +0000 (05:00 -0500)]
Only add packet to oNCP control queue with nc and pulse protocols
Don't add packets to the oNCP control queue if not using Juniper
Network Connect or Pulse Connect Secure protocols otherwise a number
of packets equal to the maximum queue length can be queued and disable
reading from the TUN device for the duration of the VPN connection
because the packets will never get dequeued except when using those
two protocols.
Commit b4f50f8 broke OpenConnect transmitting across the GlobalProtect
protocol with ESP packets when:
1. The tun device has an IPv6 address (eg link local).
2. IPv6 packets (eg router solicitation) are transmitted in quantity
equal to maximum queue length.
[dwmw2: Check the udp_send_probes function insted of two string compares] Signed-off-by: Corey Wright <cwright@digitalocean.com> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 2 Aug 2019 21:52:22 +0000 (14:52 -0700)]
Simplify openconnect_set_http_proxy() and report errors
There was a failure mode where openconnect would exit silently if given
a --proxy= argument it didn't like. Make it print errors in all cases,
and eliminate an -ENOMEM case which seems entirely gratuitous; I don't
think internal_parse_url() needs the 'const char *url' it's passed to
be hackishly writeable, so there is no need to allocate our own copy.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 2 Aug 2019 21:05:08 +0000 (14:05 -0700)]
Fix proxy username and password parsing.
We are supposed to take the *last* (unescaped) @ sign as the separation
between user:pass and hostname, not the first. So use strrchr() instead
of strchr().
Conversely, the first colon is the separation between user and pass so
strchr is still correct there.
Also actually support unescaping the resulting username and password.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 2 Aug 2019 17:51:09 +0000 (10:51 -0700)]
Implicitly enable basic auth for SOCKS if creds are provided.
Forcing the user to add --proxy-auth=basic on the command line as well as
providing the creds in the proxy URL is horrid. It took me a long time to
work out why it wasn't working.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>