Daniel Lenski [Sat, 30 Sep 2023 05:02:33 +0000 (22:02 -0700)]
Bugfix GP XML config: always include portal
Ever since 8e7efd51f, the GlobalProtect *portal* has been included in the
newly-written XML config (`<ServerList>`) only if the portal config XML
contained a `<portal-name>` tag.
We should include the portal even if it doesn't have a name for itself.
Daniel Lenski [Fri, 22 Sep 2023 16:54:11 +0000 (09:54 -0700)]
GlobalProtect SAML completion pages sometimes have the SAML fields only in comments
This modifies the fake GP server to have a 'saml_comments_only' option. If
set, the SAML completion fields ('saml-username', 'prelogin-cookie', etc.)
will be sent to the client *only* in a blob of XML wrapped in HTML comments,
and *not* in HTTP headers.
Some real GP servers are known to behave like this, and authentication
handlers like 'gp-saml-gui' need to be able to handle this case correctly
(see https://github.com/dlenski/gp-saml-gui/issues/51 and
https://github.com/dlenski/gp-saml-gui/pull/59).
Some GlobalProtect servers complain about old versions of the client
software connecting to them.
In the case of a connection via the GlobalProtect "portal" interface,
we capture the preferred software version from the portal and parrot it back,
as of https://gitlab.com/openconnect/openconnect/-/commit/c0d2daeaa85f69ed2f89330a53d97ae7eafdffb1?merge_request_iid=333.
However, we should update the GlobalProtect software version used as a fallback
in the case of a direct connection to the "gateway" interface.
Signed-off-by: Dimitri Papadopoulos <3234522+DimitriPapadopoulos@users.noreply.github.com> Signed-off-by: Daniel Lenski <dlenski@amazon.com>
Daniel Lenski [Tue, 22 Aug 2023 19:02:19 +0000 (12:02 -0700)]
Shim for renaming of GNUTLS_NO_EXTENSIONS in GnuTLS v3.8.1
The constant `GNUTLS_NO_EXTENSIONS` was renamed in
https://gitlab.com/gnutls/gnutls/-/commit/a7c4a04e (released in v3.8.1), and
then a backwards-compatibility shim was belatedly added in
https://gitlab.com/gnutls/gnutls/-/commit/abfa8634, which has not yet been
released.
We need to re-add the constant ourselves in order to build correctly with
GnuTLS v3.8.1. This should fix
https://gitlab.com/openconnect/openconnect/-/issues/650.
Audric Schiltknecht [Wed, 2 Aug 2023 15:15:50 +0000 (15:15 +0000)]
Fix invalid reset of URL variable in csd-wrapper
The URL variable is constructed from the CSD_HOSTNAME at the beginning of
the script. However, prior to parsing the command line, it was reset to
an empty value.
[DRL: This bug has existed since
https://gitlab.com/openconnect/openconnect/-/commit/cb83e535213ff2132643d2a68c50abc294b43b82,
when I modified the `csd-wrapper.sh` script to parse its `-url` command-line
argument, but forgot to remove the subsequent line `URL=`.]
Daniel Lenski [Wed, 26 Jul 2023 20:41:15 +0000 (16:41 -0400)]
Request help with the interpretation of F5 URIs in the docs
Some F5 VPNs use these to complete authentication and handoff to the
proprietary client, and we currently don't know how to interpret them in a
way that would allow OpenConnect to be used instead.
See https://gitlab.com/openconnect/openconnect/-/issues/639 and
https://lists.infradead.org/pipermail/openconnect-devel/2021-August/005035.html
for further discussion.
David Woodhouse [Tue, 25 Jul 2023 22:13:03 +0000 (23:13 +0100)]
Fix changelog entry for Pulse OS reporting
This was added under v9.12 instead of the HEAD section. Next person to do
that gets to implement a CI test for it :)
Perhaps we should have a policy of adding in reverse chronological order
so that newly-added lines are always immediately below the 'HEAD' title,
which would mean that merging older PRs would *conflict* instead of
silently merging into the older changelog?
Fixes: ff86be7281 ("update changelog") Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Sun, 23 Jul 2023 17:18:09 +0000 (13:18 -0400)]
Fix juniper-auth test
In 57160c9f2673adbbe468db137b28da4187549061, I updated
fake-juniper-server.py to use a "persistent" configuration (as already done
for fake GlobalProtect, Fortinet, F5 servers), but thne I somehow forgot to
update the actual juniper-auth test script accordingly.
Daniel Lenski [Wed, 19 Jul 2023 14:41:16 +0000 (07:41 -0700)]
Replace broken link with Wayback Machine link
The article "Why TCP Over TCP Is A Bad Idea" is very useful for explaining
why VPNs perform better when using UDP-based transport (DTLS or ESP) rather
than TCP-based transport (TLS), but unfortunately the original site is no
longer available.
Replace it with a link to the Internet Archive's Waback Machine, specifically
https://web.archive.org/web/20230228035749/http://sites.inka.de/~W1011/devel/tcp-tcp.html
Dimitri Papadopoulos [Sat, 20 May 2023 12:10:39 +0000 (14:10 +0200)]
Update supported protocols
* Standardise on Array Networks, not Array Networks AG
From https://arraynetworks.com/ssl-vpn/:
> Array SSL VPN gateways provide secure remote access to
> applications, desktops, file shares, networks, and Web
> sites from a broad range of remote and mobile devices.
> Deployed at the network perimeter or in front of
> business-critical resources, the AG provides secure
> remote access for employees, guests, partners, and
> other communities of interest. SSL VPNs are ideal for
> simplifying the user experience while reducing potential
> attack vectors.
>
> Every AG SSL VPN provides a complete secure access
> feature set, including TLS encrypted connectivity,
> device validation, endpoint and server-side security,
> advanced AAA, and granular policy controls. Available
> as physical or virtual appliances, or on your choice
> of public cloud, the AG Series is ideal for businesses
> needing enterprise-wide remote access, and for cloud
> service providers needing flexible remote access to
> meet broad ranging customer requirements.
I think AG refers to the gateway series that support SSL VPN,
not to the protocol.
* PAN → Palo Alto Networks
End-users may not know of this abbreviation, which is not
used in the documentation and marketing material.
* Add Ivanti to Pulse Connect Secure
* List these protocols separately:
- Juniper Network Connect
- Pulse/Ivanti Connect Secure
Daniel Lenski [Tue, 13 Jun 2023 19:10:33 +0000 (12:10 -0700)]
OpenConnect should report the client operating system to Pulse servers
We already know from a MITM capture on Windows how and where this is
reported by the official clients.
As seen with other protocols, some Pulse VPN servers may rely on the
presence of OS information in order to respond with a complete and correct
main configuration packet (see possible cases of this requirement in
https://gitlab.com/openconnect/openconnect/-/issues/459).
Daniel Lenski [Fri, 26 May 2023 19:39:33 +0000 (12:39 -0700)]
Handle Pulse main config packets up to 1 MiB
Our implementation has assumed that the entirety of the main Pulse
configuration “packet” will fit in one TLS record; however,
https://gitlab.com/openconnect/openconnect/-/issues/617 demonstrates that it
can in fact exceed 16 KiB if it includes e.g. a large proxy configuration.
In order to handle this, we need to dynamically allocate the space to hold
this packet, and read it in a loop.
(See https://gitlab.com/openconnect/openconnect/-/commit/2d77040a870851a625de16938fcdda6a5494d7ed
for a previous case where a configuration packet unexpectedly exceeded the
limits of a single TLS record.)
Daniel Lenski [Fri, 2 Jun 2023 21:48:32 +0000 (14:48 -0700)]
Log attributes for proxy auto-config (PAC) in Pulse configuration
Per https://gitlab.com/openconnect/openconnect/-/issues/617#note_1413539553,
Pulse servers may send proxy auto-config information
(https://en.wikipedia.org/wiki/Proxy_auto-config) in two forms
in the main configuration packet:
- attr 0x4023 contains a URL where the PAC file can be downloaded
- attr 0x4009 contains the full contents of the PAC file (may
be very large)
Daniel Lenski [Fri, 30 Jun 2023 20:50:33 +0000 (13:50 -0700)]
CI: Allow Android jobs to fail (error → warning)
Until we figure out how to make these reliable, they're preventing automatic
merging of several MRs. Android is decidedly a third- or fourth-class
platform in terms of OpenConnect developers' ability and willingness to
support it.
David Woodhouse [Wed, 14 Jun 2023 08:20:53 +0000 (09:20 +0100)]
Fix TPMv2 ECDSA signature ASN.1
I lifted this code to use it elsewhere and found that 'openssl dgst -verify'
didn't like the resulting signatures.
So ensure we have a definite lengh for the overall SEQUENCE and that we
don't have gratuitous zeroes at the start of each INTEGER. Even 'openssl
asn1parse' whines about the latter, calling it a :BAD INTEGER:.
I can't find any documentation which mandates DER, and I don't see the
point since there's a randomly generated salt so there's no 'canonical'
signature result anyway. But it doesn't hurt, and this matches what
GnuTLS does in 3.6.0 onwards where it *does* provide this function.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 19 May 2023 13:54:26 +0000 (14:54 +0100)]
Clean up ifreq_set_ifname() and use it from bsd_open_tun() too
Currently, if we set a name with --interface which is too long to fit in
ifr->ifr_name, it gets silently truncated with strncpy(). This in itself
is not immediately broken, although the FreeBSD build does complain:
tun.c:262:17: warning: 'strncpy' output may be truncated copying 15 bytes from a string of length 74 [-Wstringop-truncation]
262 | strncpy(ifr.ifr_name, tun_name + 5, sizeof(ifr.ifr_name) - 1);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
It's not *immediately* broken, and there are no string overflows; the NUL
termination is there anyway. But it *is* broken eventually, because we'll
spawn vpnc-script with the *originally* intended name, and it won't find
the device with that name.
So fix it up to check the length and then return an error if the requested
name is too long, and just use memcpy() to put the string into ifr_name,
which was pre-zeroed anyway.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 19 May 2023 13:16:53 +0000 (14:16 +0100)]
Fix unaligned accesses in ESP checksum calculation
The FreeBSD 14 build complains:
gpst.c: In function 'gpst_esp_send_probes':
gpst.c:1512:57: warning: taking address of packed member of 'struct ip6_hdr' may result in an unaligned pointer value [-Waddress-of-packed-member]
1512 | uint32_t sum = csum_partial((uint16_t *)&iph->ip6_src, 8); /* 8 uint16_t */
| ^~~~~~~~~~~~~
gpst.c:1513:49: warning: taking address of packed member of 'struct ip6_hdr' may result in an unaligned pointer value [-Waddress-of-packed-member]
1513 | sum += csum_partial((uint16_t *)&iph->ip6_dst, 8); /* 8 uint16_t */
| ^~~~~~~~~~~~~
gpst.c:1525:17: warning: converting a packed 'struct icmp6_hdr' pointer (alignment 1) to a 'uint16_t' {aka 'short unsigned int'} pointer (alignment 2) may result in an unaligned pointer value [-Waddress-of-packed-member]
1525 | sum += csum_partial((uint16_t *)icmph, icmplen / 2);
| ^~~
Rather than loading a potentially unaligned uint16_t directly, use
load_be16() instead. And pass (void *) pointers around instead of
(uint16_t *).
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 19 May 2023 11:06:28 +0000 (12:06 +0100)]
Fix time_t handling in parsing F5 session timeout
We can't assume that time_t is 'long'. When building for win64 we get:
../f5.c: In function 'f5_configure':
../f5.c:690:63: warning: format '%ld' expects argument of type 'long int *', but argument 6 has type 'time_t *' {aka 'long long int *'} [-Wformat=]
690 | if (sscanf(cookie->value, "%dz%dz%dz%ldz%ld%c", &junk, &junk, &junk, &start, &dur, &c) >= 5
| ~~^ ~~~~~~
| | |
| long int * time_t * {aka long long int *}
| %lld
../f5.c:690:67: warning: format '%ld' expects argument of type 'long int *', but argument 7 has type 'time_t *' {aka 'long long int *'} [-Wformat=]
690 | if (sscanf(cookie->value, "%dz%dz%dz%ldz%ld%c", &junk, &junk, &junk, &start, &dur, &c) >= 5
| ~~^ ~~~~
| | |
| long int * time_t * {aka long long int *}
| %lld
Make it explicitly 'unsigned long long' instead.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Dimitri Papadopoulos [Sun, 8 Jan 2023 12:09:01 +0000 (13:09 +0100)]
Ignore non-sensical NBNS/WINS server address
A VPN server sent the non-sensical NBNS/WINS server IP address 0.0.0.0.
I assume this is the default value in the VPN configuration. If so, it
could happen again. Do not pass this invalid default value to the script.
Mike Gilbert [Thu, 18 May 2023 19:04:17 +0000 (15:04 -0400)]
Move JSON_CFLAGS before LIBPROXY_CFLAGS
Depending on build options, libproxy-1.0.pc depends indirectly
on json-c.pc:
libproxy-1.0 -> gio-2.0 -> mount -> libcryptsetup -> json-c
This causes "pkg-config --cflags libproxy-1.0" to emit
"-I/usr/include/json-c".
json-c installs a "json.h" file that conflicts with the one provided by
json-parser. If json-c comes before json-parser on the compiler command,
we get a build failure:
openconnect-internal.h:1654:59: error: unknown type name 'json_value'
[ dwmw2: This is a combination of at *least* three different bugs in
three different packages conspiring to be my problem. See
https://gitlab.com/openconnect/openconnect/-/merge_requests/476#note_1397129468
But still, working around it does no harm for now.
Ironically, if the presence of json-c on the include path
wasn't *entirely* gratuitous then hiding it by putting it
last wouldn't actually work because then something would
fail to include the json-c version of <json.h> instead. ]
Bug: https://bugs.gentoo.org/906662 Signed-off-by: Mike Gilbert <floppym@gentoo.org> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 18 May 2023 15:49:29 +0000 (16:49 +0100)]
Move openconnect_set_sni() to API v5.9
We retrospectively added openconnect_set_sni() with the @OPENCONNECT_5_8
symbol version, *long* after API v5.8 was set in stone with the v9.00
release in April 2022.
Fix that by retconning it into a @OPENCONNECT_5_9 version which will be
part of the *next* release.
We have a unit test to prevent us from doing it again, and this commit
is the exception to the general rule that we should *never* commit to
libopenconnect5.symbols except as a side-effect of 'make tag' creating
a new release.
Fixes: 494edf49e628 ("Add openconnect_set_sni API function and Java setSNI() wrapper") Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Thu, 18 May 2023 13:07:33 +0000 (14:07 +0100)]
Add dpkg-gensymbols template file and test for ABI violations
Symbol versioning is hard.
Add some sed magic to build a symbols file of the form consumed by
dpkg-gensymbols, which maps symbols+versions to the first version
of the package in which they appeared.
This serves two purposes.
Firstly it allows us to have a unit test which helps prevent us from
retrospectively adding symbols to a given version after it is first
released — as we did for example when we added openconnect_set_sni() to
OPENCONNECT_5_8 in the 9.10 release.
Secondly, it helps the Debian packaging to get dependencies right. In
RPM distributions, symbol versions map automatically to RPM dependencies
and everyhing Just Works. The package with the library gets a virtual
Provides: of e.g. 'libopenconnect.so.5(OPENCONNECT_5_8)(64bit)', any
package which *uses* symbols from the library will get a corresponding
virtual Requires: — for the symbols it's actually *using* — and it all
works out perfectly. Debian packages, on the other hand, appear to be
held together with duct tape and tears, and need the developer or the
packager to manually curate a file with the mapping of symbol versions
to the first version of the package in which they appeared.
Look Ma! I can sed!
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Wed, 17 May 2023 08:51:48 +0000 (09:51 +0100)]
Rebuild all test certificates
The CA has expired. Rebuild it (and remove the old GnuTLS CA from the
ca-key.pem file where it was just noise).
Rebuild all other certificates while we're at it, but leave the keys
as they were. Extend the validity to 10000 days which should expire
in 2050, by which time it probably won't be my problem.
Dan seems young and healthy; maybe he can thank me then for pedantially
scripting it all instead of doing it manually. Or maybe it'll have
bitrotted so much by then that it won't help.
Most of it worked out of the box this time, but I re-imported the certs
into SoftHSM manually because I didn't want to start from scratch using
the softhsm-setupX make targets. I think some of the behaviour of the
GnuTLS tools (not importing pubkeys, etc) has changed since I did this.
Arguably we should rewrite those rules to import things the same way
into each token and then explicitly tweak them, deleting the public
keys and explicitly marking objects public or private as needed for
each token.
The SoftHSM modifications also had to be done with an older version
of SoftHSM (I used 2.2.0 on Ubuntu 18.04) because doing it with a
newer version meant the newly-imported certs weren't visible in the
Ubuntu 18.04 or CentOS 9 test runs.
Fixes: #609 Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 16 May 2023 16:08:45 +0000 (17:08 +0100)]
Fix order-only rule dependency variables
When I made the cert rules order-only to prevent all the certs from being
rebuilt unnecessarily, I forgot to switch $< to $| in referencing the
names of the dependencies.
Fixes: e24ef965a96a ("Make all cert rules order-only") Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Daniel Lenski [Tue, 16 May 2023 22:54:12 +0000 (15:54 -0700)]
Fix broken ESP config parsing for GlobalProtect
This was broken in
https://gitlab.com/openconnect/openconnect/-/commit/e2bbc2a1f#efecf80fa476ca5abf1502940e60d7984c6d1df9_426_430
As the comment below that change notes, "We ignore the Legacy IP tag
(<gw-address>) if we've already gotten the IPv6 (<gw-address-v6) tag."
We do indeed want to prioritize ESP-over-UDP-over-IPv6 over
ESP-over-UDP-over-IPv4.
However, this change broke things by making it so that effectively, "We
ignore either tag, unless we've already received a tag."
Thanks to nemo44@gmail.com for bringing this to our attention in
https://gitlab.com/openconnect/openconnect/-/merge_requests/475. I've
modified that patch slightly to make it a bit easier to read and more
idiot-proof in the future (while giving the idiot in question a cold hard
stare in the mirror.)
[Incidentally, a misordered `#endif` / `}` pair also made it so that
https://gitlab.com/openconnect/openconnect/-/commit/e2bbc2a1f and later
wouldn't even compile unless `HAVE_ESP` was `#define`d. Probably no one
is building without ESP support… why would they?]
David Woodhouse [Thu, 11 May 2023 14:55:41 +0000 (15:55 +0100)]
Consolidate browser spawn functions
These were almost identical except that the one in main.c would allow the
browser to be overridden. Combine them, as it's only going to end up with
more duplication if we manage to add Windows support.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Wed, 10 May 2023 11:22:20 +0000 (12:22 +0100)]
Fix stray (null) in URL path after Pulse authentication
When using 'openconnect --authenticate' with a Pulse server, if the urlpath
is empty we append '(null)' to the URL instead of appending nothing as we
should. This also affects NetworkManager-openconnect, since it started to
use openconnect_get_connect_url() in v1.2.8 (commit 911151fc966790c).
Fixes: ec6c0caed28e ("Add openconnect_get_connect_url(), use it in --authenticate output") Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Dimitri Papadopoulos [Sat, 6 May 2023 16:18:19 +0000 (18:18 +0200)]
Document that OpenConnect calculates TOTP/HOTP codes on its own
OpenConnect has calculated TOTP/HOTP token codes without liboath since 554454bf;
we should document that.
Alo:
- Remove the unnecessary downloading and building of liboath from 'android/Makefile'.
- Remove obsolete references to liboath in comments and error messages
- Fix man page formatting surrounding token mode
Signed-off-by: Dimitri Papadopoulos <3350651-DimitriPapadopoulos@users.noreply.gitlab.com> Signed-off-by: Daniel Lenski <dlenski@gmail.com>
Daniel Lenski [Fri, 17 Jun 2022 16:59:03 +0000 (09:59 -0700)]
Add os-tcp-mtu utility
Makes a host connection to an arbitrary TCP/IP host:port, and checks the
estimates of the MTU/MSS provided by various getsockopt() calls, just as
OpenConnect uses in calculate_mtu().
TODO:
1. Implement a working os-tcp-mtu for Windows, and build that too.
2. Use https://github.com/morristech/android-ifaddrs as
as a drop-in replacement for `getifaddrs(3)` on Android
David Woodhouse [Thu, 4 May 2023 17:58:14 +0000 (18:58 +0100)]
Fix use-after-free in realloc_inplace()
In file included from auth-globalprotect.c:20:
auth-globalprotect.c: In function 'parse_prelogin_xml':
openconnect-internal.h:1180:17: warning: pointer '__realloc_old_176' may be used after 'realloc' [-Wuse-after-free]
1180 | free(__realloc_old); \
| ^~~~~~~~~~~~~~~~~~~
openconnect-internal.h:1178:13: note: call to 'realloc' here
1178 | p = realloc(p, size); \
| ^~~~~~~~~~~~~~~~
This is a true warning. The second argument to the realloc_inplace()
macro includes a strlen() of the first. Evaluate it first, before the
attempt to realloc().
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Fri, 28 Apr 2023 07:22:19 +0000 (09:22 +0200)]
Rework ESP probe retries
We weren't attempting to resend ESP probes at all, except at the retry
interval of about a minute. In a lossy network, or perhaps when the
server is slow to configure its end and start accepting ESP probes,
this meant that users sometimes saw the ESP failing to establish for
a whole minute (or multiple thereof).
Drop the loops in the protocol-specific udp_send_probes() functions
which were a primitive attempt to handle packet loss, and instead
deliberately send one probe a second for five seconds, before giving
up for the remainder of the dtls_attempt_period.
Fix up the reconnect handling with vpninfo->dtls_need_reconnect while
we're at it; it looks like that would just cause us to keep sending
probes and the flag would never be cleared.
Fixes: #601 Signed-off-by: David Woodhouse <dwmw2@infradead.org>
Dimitri Papadopoulos [Fri, 17 Mar 2023 06:27:25 +0000 (07:27 +0100)]
pulsesecure.net → ivanti.com
We should also rename Pulse Connect Secure to Ivanti Connect Secure
at some point. For now, even the Ivanti web site uses both, perhaps
we should wait before we switch Pulse to Ivanti.
Alex Samorukov [Fri, 28 Apr 2023 17:53:29 +0000 (17:53 +0000)]
Add MacOS support to the hipreport
DRL tweaked:
1. Fixed indentation
2. Populated datemon/dateday/dateyear attributes
3. If reporting MacOS, but not actually running on MacOS, then fall back to
reporting OS version 10.16.0 (see https://ss64.com/osx/sw_vers.html)
Signed-off-by: Alex Samorukov <samm@os2.kiev.ua> Signed-off-by: Daniel Lenski <dlenski@gmail.com>
David Woodhouse [Wed, 19 Apr 2023 11:49:42 +0000 (12:49 +0100)]
Attempt to handle Legacy IP frames in the middle of oNCP config
Is there no end to the stupidity of these proprietary protocols?
So... TCP provides you with a byte stream. Forget IP packets underneath;
TCP hides all that for you, and all there is is a hosepipe of bytes.
On top of that, TLS provides 'records' of up to 16KiB, encrypting each
record and sending it down the TCP pipe, then collecting the whole record
on the receiving end, decrypting it and providing it to the user. Mostly,
nobody cares much about TLS records, and they choose to treat TLS just as
a byte stream too.
The Juniper oNCP protocol adds its own layers on top of that TLS byte
stream. First there's the thing which OpenConnect has been calling the
'SSL record', which has a little-endian 16-bit length followed by that
many bytes of data. Then another 16-bit length and that much data, etc.
Those "SSL records" may be correlated with the TLS records, or maybe I
just chose a really stupid name back in 2015 when I first did this. Who
knows, who cares, since we mostly ignore TLS records anyway.
On top of *this* there is an actual packet-based protocol which may
perhaps be called something like KMP. There's a 20-byte header, most of
which we don't understand but it has a type field and a length that we
do understand. This one is big-endian, unlike the layer below it The
type 300 is data, while type 301 is the configuration we get when we
first connect.
Within the KMP300 packets there can be multiple actual IP packets; we
need to look at the length field of the IP packet and split them apart
for delivering them to the tun device. There is no apparent relationship
between the KMP300 packets and the SSL record below; you can have many
KMP300 packets within an SSL record, and the SSL record boundary can
fall *within* a KMP300 packet. So the SSL records have, thus far, just
been a pointless nuisance which means we occasionally have to drop a
2-byte length field from what would otherwise just be treated as a
simple byte stream of data which packetises itself.
Thus far.
If you have a large enough KMP301 config frame (for example, if you have
lots of split include routes), it might get sent in more than one SSL
record. So OpenConnect has code to keep reading those records until the
KMP301 frame is complete. And it *did* assume that the SSL record would
end at the end of the KMP301 frame, since we haven't even finished the
negotiation at this point; we have to send a KMP303 frame back to the
server with our idea of the MTU.
Today we have learned that the server might throw a KMP300 packet with
an IP frame into the *middle* between the two consecutive records that
comprise the KMP301 config frame:
https://gitlab.com/openconnect/openconnect/-/issues/562#note_1358466466
It's not clear if this will only ever be *one* IP packet, or if it can
end up being lots of packets and split across multiple SSL records. In
the example I've seen, it was a *unicast* packet for the VPN client
address.
Attempt to work around the case we've already seen, by discarding a
continuation that looks like a KMP300 with precisely one Legacy IP
packet in it. We *are* also assuming that we at least get the *start*
of the KMP301 config frame first.
Perhaps we'll end up having to keep a list of the records we receive,
then attempt to piece them together more flexibly. But this should
suffice for now.
Fixes: #562 Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Tue, 18 Apr 2023 21:37:05 +0000 (22:37 +0100)]
Fix EINTR handling for select() on cmd_fd
KDE plasma-nm was failing with external browser authentication, reporting
'Socket connect cancelled' immediately after spawning the browser command.
This was caused by the SIGCHLD which kded handles (instead of it being
ignored as in the standalone openconnect process). Thus, the select()
call returns with EINTR, and the fd sets are not changed.
So when check_cmd_fd(vpninfo, &rd_set) is called, it looks like the cmd_fd
was readable. And since plasma-nm uses the legacy cancel_fd setup, the
mere fact of it being *readable* is enough to set vpninfo->got_cancel_cmd.
Instead of forging ahead and interpreting the fd_sets after select()
returns with EINTR, loop and go straight back into select() instead.
For signals like SIGINT from Ctrl-C, we *handle* those and deliver the
cancel command anyway, so we don't depend on the EINTR return in that
case.
Daniel Lenski [Tue, 18 Apr 2023 18:27:34 +0000 (11:27 -0700)]
More specific error message with proposed workaround for Pulse EAP-TLS requests
See https://gitlab.com/openconnect/openconnect/-/merge_requests/414#note_1149887354
for discussion of the apparent layering insanity that's going on in this
corner case.
Daniel Lenski [Sat, 30 Jul 2022 22:59:14 +0000 (15:59 -0700)]
Add --no-external-auth option, and follow it for Cisco protocol
This option is intended to prevent OpenConnect from advertising to the
server that it supports any kind of authentication mode that requires an
external browser. Some servers will force the client to use such an
authentication mode if the client advertises it, but fallback to a
"scriptable" authentication mode if the client doesn't appear to support it.
See https://gitlab.com/openconnect/openconnect/-/issues/470#note_1028595620
for an example.
This option is only implemented here for the Cisco protocol, in which case
it causes OpenConnect not to advertise the 'single-sign-on-v2' or
'single-sign-on-external-browser' auth-methods.
I have verified on one Cisco VPN that this works as intended, as has one
other user (see
https://gitlab.com/openconnect/openconnect/-/issues/470#note_1045509425).
David Woodhouse [Wed, 12 Apr 2023 20:10:34 +0000 (21:10 +0100)]
Fix --server vs. positional argument handling
If we have a --server argument, we shouldn't expect to find a positional
argument at argv[optind]. We mostly got this right, except that we still
called config_lookup_host() with argv[optind] even when --server was
given.
The only thing that saved us from dying with a strcmp() on NULL was
the fact that the loop over the XML elements is using the fact that
vpninfo->hostname gets set as its terminating condition, so it never
got that far.
Fix that, because it's icky. And make the --server argument work for
XML config lookups too.
Fixes: a2fd6f4f2e8a ("New option to define server name in config file") Signed-off-by: David Woodhouse <dwmw2@infradead.org>
David Woodhouse [Wed, 12 Apr 2023 15:28:50 +0000 (16:28 +0100)]
Set SOCK_CLOEXEC on listening socket for Cisco external browser support
Not entirely sure, but it seems to be accused of causing a hang in
https://gitlab.gnome.org/GNOME/NetworkManager-openconnect/-/merge_requests/49#note_1720281
and regardless of whether that's the case or not, we should be consistent
about using {SOCK,O}_CLOEXEC whenever we can.
Signed-off-by: David Woodhouse <dwmw2@infradead.org>
projects / users / dwmw2 / openconnect.git / log