Due to the `sleep()` call, a continuous stream of failed authentication
attempts can potentially exhaust the thread pool. While delays can also
happen due to RADIUS or DHCP on the server side, this can directly be
triggered by clients. It's questionable whether the delay ever had the
intended effect. But since the retry functionality is also quite
strange, let's just remove it so retries will require creating an IKE_SA
from scratch. To avoid leaking much of a timing difference if no secret
is found now that the two second delay is gone, we do the calculations
with a dummy NT hash.
Fixes: f98cdf7a47 ("adding plugin for EAP-MS-CHAPv2")
Because `assess()` returns a `status_t`, `FALSE` is interpreted as
`SUCCESS`. So a failure while processing PB-TNC batches terminated
the process successfully.
Fixes: 1e92d5f114 ("Process PB-TNC batches received via PT-TLS asynchronously")
Note that wolfSSL before 5.9.2 required building with
WOLFSSL_ECDHX_SHARED_NOT_ZERO, which was added with 5.3.0, to get and
explicit check. Since the plugin validates the public key, the test case
fails nonetheless.
While RFC 7748 states implementations MAY perform such a check, e.g.
TLS 1.3 explicitly requires it (RFC 8446, section 7.4.2).
Fixes: 7f9bfacd5a ("curve25519: Add a plugin providing Curve25519 DH using backend drivers")
Using the same `hasher_t` instance from different threads concurrently
is not safe. The underlying implementation might e.g. use a single
shared state for multiple API calls within `get_hash()` (e.g. the openssl
plugin does that).
Fixes: 007c47088c ("Implemented permanent certificate coupling plugin")
The WC_RNG instances are potentially shared between different threads
as private key objects are refcounted. This may corrupt their internal
state as they are not thread-safe.
For ECDSA, using separate instances for each signing operation has some
performance impact, but for signature operations that should be fine.
The implementation for RSA uses mutexes. That's due to the weird API.
While RNG instances can be passed for signing and encryption (probably
because they are also required for padding/salt besides blinding), they
can't for verifying and decryption. The latter use an RNG instance that
has to be set on the key object before calling these operations. So we
could potentially split the strategy, but to keep this consistent within
the RSA implementation, just continue with the shared RNG but use a
mutex around the API calls.
Fixes: c92eade82c ("wolfssl: Add wolfSSL plugin for cryptographic implementations")
The `aead_t` interface states that `decrypt()` only allocates a plaintext
buffer if successful, so callers might not free it if the call failed.
Fixes: 313811b72d ("aesni: Add a GCM AEAD based on the AES-NI key schedule")
If an `aead_t` implementation incorrectly allocates memory if the
decryption/ICV verification failed, this avoids a leak. Unfortunately,
many implementations actually did that.
Fixes: 24a8d1253f ("libipsec: Wrap traditional algorithms in AEAD wrapper")
This was added with 5ce3c9b15a ("watcher: Rebuild fdset when select()
fails"), i.e. before switching to poll(), solely to suppress errors when
FDs are closed and select() would return with an error. With poll()
this should not happen result in an error (it potentially indicates this
via POLLNVAL in revents of that FD in the array).
Because the flag was not consistently changed/read with the mutex held,
some analysis tools got confused and imagined wild deadlock scenarios.
In non-blocking mode, the previous code set `in_done` to -1 (SIZE_MAX)
if `recv()` would block and nothing was read yet. If this was followed
by a call to `write()` and `process()` is called and actually processed
application data, the length calculation in the callback underflows and
`memcpy()` would write to `in.ptr + SIZE_MAX`. Since `read()` already
sets `errno` to `EWOULDBLOCK` and returns -1 if `in_done` is 0, the
removed check was redundant anyway.
Also, the buffer from the previous `read()` call might not be valid
anymore when `write()` is called (e.g. `splice()` uses the same buffer
for both, and the buffer could even be defined on a now invalid stack
frame of the function that called `read()` previously). Clearing the
data avoids that and ensures the application data is cached until the
next call to `read()`.
However, triggering this is rather difficult as `write()` should only
reach `recv()` while the handshake isn't complete and until then
`process_application()` doesn't accept application data. But if the
handshake is completed during a call to `write()` that follows a
non-blocking read and data immediately arrives, it's theoretically
imaginable. This scenario is highly unlikely on a TLS server, which
starts the process with a call to `read()` that then basically loops
until the handshake is done. Even if multiple calls are required, the
server will generally not call `write()` before it received application
data. And any calls to `write()` afterwards do not reach `recv()`
anymore (unless no data to send was passed, which would be weird, or
maybe for some weird corner case that lets `build()` fail before all
outbound application data was processed).
The two fields were swapped in the calculation and the new code also
avoids overflowing on 32-bit systems.
Fixes: 4cb0e1bb76 ("Added basic support for PGP certificates (no trust relationships yet)")
This does not seem to work on these runners. Tried setting
dataflowMemoryLimit, but that only seems to affect the analysis (which
causes a warning: "Taint analysis was stopped early due to low memory.
The analysis may not have explored the whole program.") not the
post-processing that evidently causes the runner to get killed due to
OOM (there is no explicit error, but in the debug log we see "The runner
has received a shutdown signal", which apparently can indicate OOM
issues).
Because the CFLAGS applied to the whole plugin, the compiler could
"optimize" the boilerplate plugin code, which could then cause a SIGILL
on hardware that doesn't support such instructions. This change makes
sure only the actual AES implementation is compiled like that, which
would then not get registered depending on the CPU feature detection.
If there are lots of SAs to be synced, the default might be too low
and messages and SAs get dropped. The new default is already 8 MiB,
which should work fine for lots of SAs. The code mirrors the one in
the kernel-netlink plugin (but with a guard around SO_RCVBUFFORCE, even
though this plugin is mostly used on Linux as well).
This fixes handling of SIGINT (CTRL+C) so we don't continue with the
next potentially hanging command (e.g. several pings after another that
fail because a required SA didn't come up correctly). We try to kill
the PID for every SIGINT in case the first one arrived before the
subshell was started. The `TDUP_<host>` variables are not updated in
the main shell when `stop_tcpdump` is called from the subshell, so
it is called redundantly during the cleanup (removed the useless guard
there and use pkill to suppress any messages if no process is found).
If we get interrupted during one of the init/cleanup WAIT_FOR waits,
we just let them running in the background when exiting the script.
They should generally not do any harm (and be terminated once the next
run is attempted, which might require a rebuild during development
anyway).
Since a103f3a284 ("testing: Add options to only run pre- or posttest
scripts of a scenario") we can now also skip the remaining steps easily.
However, we do run the posttest script to clean up properly (e.g.
terminate the daemons, restore firewall rules etc.). That's also why
it's not running in a subshell.
These are apparently not necessary anymore and since there are often
sync issues (package version mismatch if security fixes are shipped
because there is no *-security suite in the ddebs repo), lets not
install them anymore.
It seems that 18a94525a7 was a bit hasty. Apparently, it's still the
case that there were reports (at least in some test scenarios). Luckily,
the new facility added in the previous commit allows us to whitelist
these allocations without having to ignore all unknown memory.
With glibc, there is an issue if TZ is not set, which causes a change
of the internally cached TZ value. Because the original value was
cached before LD was active via `init_static_allocations()`, the memory
is freed as unknown memory later. This change allows whitelisting
a function that might free such memory (tzset() only for now).
This prevents an OOB read if the AT_RAND data is shorter than the
expected 16 bytes.
The check for AT_AUTN is changed for consistency even though its length
is already enforced by the parser (for AT_RAND it isn't because EAP-SIM
expects a length of either 32 or 48 bytes).
Fixes: aea334ec1c ("Splitted EAP-AKA in peer and server implementations, use libsimaka helper library")
When resolving the remote host, we first determine if a particular
address family is preferred locally. With `0.0.0.0` that's IPv4, with
`%any` that's not the case. So we use the latter to allow resolvers
to return an IPv6 address.
Adds IKE and IPsec proposals to the `list-conn` VICI event. Currently
not printed in `swanctl --list-conns` to keep the output compact (`--raw`
can be used to see the proposals).
Closesstrongswan/strongswan#3067
This allows clients to distinguish between algorithms of different
transform types more easily. The names are similar to those used
when returning the algorithms of the selected proposal in list-sas (except
for `ke` instead of `dh` and `sn` instead of `esn` to reflect the
latest IETF/IANA changes).
The option was renamed with 7f9f9bd375 ("Fixed some typos, courtesy of
codespell"), the check was not.
Fixes: dd7b0283ef ("plugin-loader: Add option to change log message if plugin is not found")
Because the `derived` flag was not reset (it's set after the initial
IKE_SA_INIT exchange), no keys would get derived when sending
IKE_INTERMEDIATE during the next try. As there is then no `aead_t`
available, encrypting the message would fail and the initiation would
remain stuck.
Fixes: 0d49ddec2e ("ike-init: Add support for multiple key exchanges")
This reverts commit b998695344.
Seems like this is not necessary anymore. Possibly because of
8ff3238027 ("openssl: Prevent OpenSSL from using posix_memalign() if
LD is enabled").
In a targeted request, the software ID is provided by the IMV. If no
database is used (which is not the recommended setup), the ID is not
validated and could potentially contain special characters. With the
previous command string construction and use of popen(), which runs a
shell, that could potentially allow running arbitrary commands.
Unlike `struct ifreq` that's used for IPv4, `struct in6_ifreq` contains
not a `struct sockaddr[_in6]` but only a `struct in6_addr`.
Setting addresses like this is currently not used on Linux (the feature
was added to install virtual IPs on FreeBSD/macOS).
Fixes: fccc76449d ("tun-device: Fix handling of IPv6 addresses")
Keys loaded via generic loader (KEY_ANY) or from a PKCS#12 file (or an
engine) don't go through the openssl_ec_private_key_load() constructor
that checks for explicit parameters.
This was broken since the Botan 3 release, which removed the EMSA1
class and the define. The "EMSA1()" wrapper when signing/verifying is
technically not necessary anymore since then (it's deprecated but still
accepted). But to still support Botan 2, we keep that in for now.
The missing parentheses around the additions when calculating optlen
in the previous code can cause an out-of-bound read of up to 228 bytes
if no DHCP_OPTEND is found in the message (the calculation basically
evaluated to `- 20 + 8 + 240`).
Since the buffer for the received packet (via pf_handler_t) is located
on the stack, this shouldn't cause much of an issue in practice.
We don't want to build old versions using these caches, so we don't
need old entries (if header files change, there could be lots of
differences that increase the cache size unnecessarily).
These are shared by many tests, in particular the "all", "coverage",
"no-dbg" and "no-testable-ke" tests, which each would otherwise require
their own large cache.
Similarly, the "codeql" and "sonarcloud" tests rely on the same
dependencies but only the latter uses ccache for the strongSwan build.
Also reduce the maximum size per cache for all workflows to keep them
in check over time (some could even be set lower, we'll have to see
how this develops).
As mentioned in the previous commit, ASAN does a fine job detecting leaks
during the tests. We just add a single LD-enabled build of the "default"
test here to test the basic functionality.
And we continue to use leak detective in our testing environment to keep
the memory requirements low.
We don't need a separate cache for the "apidoc" test and while the
"dist" test is similar as well, it builds in a different directory,
which means that config.h causes a cache miss for everything but the
configure checks.
The special handling caused a significant diff between builds with and
without LD, which made ccache less effective as we only store the cache
once for the build without LD.
However, despite this change, while it previously was the case that the
LD vs. non-LD builds didn't differ much, that's not the case anymore
nowadays. In particular the --disable-asan option and the BFD-based
backtraces for the native OpenSSL builds (e.g. default or openssl-sys)
cause quite a significant diff. As cache storage is limited, we keep
the current behavior for now. But it might be an option to reduce or
even remove the LD builds in the future as ASAN seems do the job pretty
well and we still use LD in the testing environment.
The others are either included already in the "all" build (to which we
now switch) or they have a relatively small diff to that (e.g. gcrypt
only differs in that relatively small plugin). For the "openssl-sys"
build, we can rely on the "default" build but only on ubuntu-latest as
we don't build that on ubuntu-22.04.
Let's try this again :) Since cache entries with the same key are not
updated/replaced and there is no option to do so, we manually delete the
previous entry for the current branch.
This reduces the cache storage for active branches, which can cause
caches of the master branch to get evicted, which in turn will slow down
builds of not only master but also new branches as they can't fall back
on those caches.
Permission has to be explicitly granted in order to delete the cache
entries when not using the legacy all-write tokens that are the default
for old repositories.
The continue-on-error option is set for the step that deletes the old
cache entry as it's expected that cache-hit will be true for a new feature
branch when restoring the cache from the master branch. However, because
there won't be anything to delete for this branch yet, the command will
fail. The --succeed-on-no-caches option of the command unfortunately
only works with --all.
For the Linux tests, several jobs use the same cache key. So there is
a chance that two jobs try to store a new entry concurrently, which will
fail (it works if there was a cache hit and they are slightly off as
previous entries are first deleted). To avoid that, we store the cache
only for one particular config.
Also made sure that the "openssl" test does not remove "openssl-3/4"
caches by adding a suffix to the former.
For alpine, the repository had to be set explicitly as gh wasn't able to
determine it (didn't detect the Git working dir).
While continue-on-error can be configured more specifically (e.g. also
would allow to handle "default" and "printf-builtin" tests that rely
on debug symbols), it also lets the workflow succeed if any of these
jobs fail. That's not ideal if there is an actual error and not
just an intermittent package sync problem.
The leak detective doesn't wrap this function and calling the original
causes unknown memory frees and even segmentation faults. This is now
triggered with OpenSSL 4 as the implementation of ECP256 uses
OPENSSL_aligned_alloc_array().
Setting a custom memory functions forces OpenSSL to implement aligned
allocations internally, using the registered allocation function.
Tobias Brunner