Rahul Rameshbabu [Thu, 21 Dec 2023 20:46:08 +0000 (12:46 -0800)]
cstp: Check if uri is NULL in sso_detect_done
Passing a NULL value to strcmp is undefined behavior. Some web engines
might have events where cookies are enumerated, but the event does not
contain a uri enumeration. An example is QtWebEngine where it has discrete
signals, QWebEngineView::urlChanged and QWebEngineCookieStore::cookieAdded.
Add a check similar to the one found in gpst_sso_detect_done for the uri
member of struct oc_webview_result.
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>
projects / users / dwmw2 / openconnect.git / log