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>
David Woodhouse [Tue, 16 Jul 2019 10:37:59 +0000 (11:37 +0100)]
For Pulse, send ESP only of the same IP protocol as we're connected over
It really seems that when we're connected over Legacy IP, it only accepts
Legacy IP packets in ESP. And when we're connected over IPv6, it only
accepts IPv6 packets in ESP.
This matches the behaviour of the Windows client too.
If you connect to a NC server over IPv6 it doesn't even offer the ESP
config (since NC doesn't support IPv6 within the tunnel).
Someone really ought to report this bug to Pulse. For IPv6 VPN traffic
to be forced into TCP-over-TCP mode when you happen to be connected to
the VPN over Legacy IP is very bad.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Apparently it makes the server do EAP-TLS within EAP-TTLS if no
certificate is presented by the client. I am not ready for that level of
insanity just yet.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Colin Petrie [Thu, 27 Jun 2019 16:24:51 +0000 (18:24 +0200)]
Update CSTP for IPv6 DNS servers
When the Cisco side is configured with IPv4 and IPv6 DNS resolver IPs,
it will send option X-CSTP-DNS-IP6
This patch captures the IPv6 addresses provided, and puts them in
INTERNAL_IP6_DNS variable for vpnc-scripts (which is already handled
there)
Signed-off-by: Colin Petrie <colin@spakka.net>
[dwmw2: Put them in $INTERNAL_IP4_DNS instead. They shouldn't be split.] Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Mon, 10 Jun 2019 21:55:25 +0000 (22:55 +0100)]
Improve Pulse ESP setup reliability.
Sometimes, the server is slow to process the ESP config so our first probes
don't elicit a response. Abuse the licensing information packet which comes
after the connection is set up, and send a second set of probes when it
arrives.
Perhaps we should actually send three probes, half a second apart.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Mon, 10 Jun 2019 21:35:26 +0000 (22:35 +0100)]
Revert "Set ESP Next Header field to 0x29 for IPv6 packets"
This reverts commit 02ae906bb691c8b342d7ff0875e200ce55c18f2a. Turns out
Pulse doesn't *accept* ESP frames with the correct Next-Header field
for IPv6. You have to send 0x04 (IPIP). So I cleaned it up and removed
the duplication for nothing...
Might need to put this back when we work out how GPST does IPv6.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Mon, 10 Jun 2019 12:53:14 +0000 (13:53 +0100)]
pulse: Handle multiple IF-T/TLS records in a single SSL record
We are still assuming that IT-F/TLS record won't be *split* between SSL
records. That turned out to be a false assumption for Network Connect,
but hopefully they're saner here. We should cleanly complain about that
if it does happen.
There may be better ways to do this; perhaps we should receive the whole SSL
record then handle each record separately. In the common case there's no
real reason for the incoming packet queue anyway. We could just call
os_write_tun() directly. And then only have to resort to memcpy to
split packets up in the very rare case that the tun isn't taking writes
anyway.
This will do for now. The TCP connection *shouldn't* be the fast path
anyway. Not that we've worked out how to make the Pulse server actually
send data in ESP; even with the Windows client it still sends in TCP...
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Sat, 1 Jun 2019 02:10:10 +0000 (19:10 -0700)]
Add hipreport-android.sh
The desktop version of the HIP report doesn't work on Android in part
because the here-doc appears to exceed the size of the read buffer in
Android's rather primitive /system/bin/sh. This is a rather confusing bug
to identify and diagnose.
Include an alternate script with minimal contents (hipreport-minimal.sh)
which is suitable for use on Android.
Daniel Lenski [Fri, 12 Jan 2018 09:44:17 +0000 (01:44 -0800)]
Incomplete, speculative IPv6 for GlobalProtect
Client-side IPv6 support was added in v4.0:
https://live.paloaltonetworks.com/t5/Colossal-Event-Blog/New-GlobalProtect-4-0-announced-with-IPv6-support/ba-p/141593
Server-side IPv6 support was added in PanOS 8.0:
https://www.paloaltonetworks.com/documentation/80/pan-os/newfeaturesguide/globalprotect-features
I've been wanting to get IPv6 working for a while, but don't have access to
a GP VPN that supports IPv6, and haven't found anyone else who does. I'm
adding incomplete, speculative IPv6 support here in the hopes that someone
will use it and report back on partial success/failure:
* Known from Windows client: `ipv6-support=yes` in `/ssl-vpn/login.esp`
request, `preferred-ipv6` in `/ssl-vpn/getconfig.esp` request,
`client-ipv6` in `/ssl-vpn/hipreport{,check}.esp` requests,
`app-version=4.0.5-8`,
* Educated guess: 0x0800 in GPST packet header represents IPv4 ethertype,
and will be replaced with 0x86DD for IPv6 packets.
* Unknown: IPv6 routing configuration tags to expect in
`/ssl-vpn/getconfig.esp` response. This build prints a prominent
error message if it encounters any unknown configuration tags
containing the character '6', and requests feedback to the mailing
list.
Dan Lenski [Sat, 25 May 2019 05:41:19 +0000 (22:41 -0700)]
Report GP session lifetime
OpenConnect doesn't have a mechanism to immediately stop trying to reconnect
after the session expires.
Server-forced session expiration "takes care of itself" when OpenConnect
tries to reconnect repeatedly and fails, though it might be useful to save
the expected expiration time somewhere to be able to report it in a more
user-friendly fashion.
Dan Lenski [Sat, 25 May 2019 04:54:40 +0000 (21:54 -0700)]
Better spoofed HIP report
Some GlobalProtect VPNs appear to actually check the contents of the HIP
report in some way, and require that anti-virus/anti-spyware software be
labeled as up-to-date.
Also, the --computer parameter is no longer needed (now that its value
is included in the "cookie")
David Woodhouse [Fri, 7 Jun 2019 16:04:41 +0000 (17:04 +0100)]
Convert dump_buf_hex() to use oc_text_buf instead of sprintf
I seem to recall the OpenBSD build will complain loudly about the use of
"bad" functions like sprintf. And even though this particular code does
seem to be perfectly correct, they do have a point in the general case.
Just use buf_append() for this, since that's what it was designed for.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 16 May 2019 18:18:30 +0000 (11:18 -0700)]
Include <errno.h> in gnutls_tpm2_ibm.c
Some environments don't pull it in implicitly; we should include it for
ourselves.
gnutls_tpm2_ibm.c: In function ‘install_tpm2_key’:
gnutls_tpm2_ibm.c:485:11: error: ‘EINVAL’ undeclared (first use in this function)
return -EINVAL;
^~~~~~
gnutls_tpm2_ibm.c:485:11: note: each undeclared identifier is reported only once for each function it appears in
gnutls_tpm2_ibm.c:490:11: error: ‘ENOMEM’ undeclared (first use in this function)
return -ENOMEM;
^~~~~~
gnutls_tpm2_ibm.c:528:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
Makefile:1206: recipe for target 'libopenconnect_la-gnutls_tpm2_ibm.lo' failed
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Sat, 11 May 2019 09:41:14 +0000 (10:41 +0100)]
Kill MAX_BUF_LEN
There's no real point in having a hard limit for struct oc_text_buf, the
whole point of which is that it is dynamically allocated. Just guard
against the int buf_len overflowing.
In process_http_response() the hard-coded buf[] array is only used for
headers one line at a time now, so 8KiB should suffice.
Fixes: #39 Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 25 Apr 2019 11:01:02 +0000 (13:01 +0200)]
Clean up memset_s() detection a bit more.
Definining __STDC_WANT_LIB_EXT1__ to get memset_s() is required by the C11
standard, not a Solaris-ism. It's no use just to check for its presence
in the library with AC_CHECK_FUNC() if it isn't going to compile, so make
sure we check for it with AC_LINK_IFELSE() *and* with the warning flags
that might include -Werror-implicit-function-declarations.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
projects / users / dwmw2 / openconnect.git / log