windows: Avoid potential races with canceling threads
The previous code did not synchronize some of the flags, so it was
possible that `cancel()` did not work properly (e.g. no APC queued).
This change uses atomics to set/read the flags, it also avoids using
the global thread lock for synchronization as getting the condvar before
queuing the APC avoids having to sync with `docancel()`. However, to
make sure we don't use a stale and potentially already freed condvar
when the thread is canceled a new lock is added and held until the
condvar has been signaled (using atomics for that member doesn't work as
the ordering rules enforced by atomics can not avoid that the thread in
`cancel()` potentially reads a stale value, or the race between reading
and signaling it).
The protocol has long been replaced by IKEv2 and is officially deprecated
since several years (RFC 9395). As a first step to removing support for
it completely, this makes the configure option disabled by default and
warns users about its use.
This was primarily used in our labs to visualize some TNC aspects but
the third-party daemon and frontend we used have not seen any development
in a decade. There never was any industry interest in this protocol
anyway, so just remove it.
Besides the relatively recent update to libsoup-3, this has not seen
much development and lacks several features. There does not seem to be
any advantage over using the curl plugin. So just remove it to reduce
the maintenance burden.
This has not seen any significant changes for years. So it lacks support
for modern algorithms and would require quite some work for an overhaul.
Given that we support several other crypto backends, let's just remove
this to reduce the maintenance burden.
The test scenarios and other references are also removed.
This plugin was developed for a customer who had very specific
requirements. It never did anything useful for regular users and
usually caused confusing errors if they enabled it by mistake. So
just remove it.
There is no reason to use Blowfish nowadays. Given that there are some
other plugins that still provide it, there is especially no reason to
maintain this custom implementation. Also removed the two test scenarios
that used the plugin to avoid promoting the use of this algorithm.
medsrv/medcli: Remove prototypical medsrv web application and plugins
This was from a student project that has never been developed further.
And similar to the manager web application it lacks all sorts of modern
standards. So just remove it and the two plugins it relied on.
The test scenario is renamed to avoid confusion (neither of the two
p2pnat scenarios uses medsrv/medcli).
file-logger: Prevent potential race when opening log files and change permission
This fixes a potential TOCTOU issue with opening log files. The use of
`chown()` instead of `fchown()` could theoretically allow modifying the
ownership of an unintended file.
The log file now also is not world-readable anymore.
Also, the patch fixes the log groups for the two `(f)chown()` errors.
Fixes: d35d6691807d ("Make syslog and file loggers configurable at runtime")
chunk: Improve constant time comparison for chunks with unequal length
While for most uses the length is fixed and public (e.g. PRF/MAC outputs),
there are a few (e.g. in xauth-generic) that compare variable length
data.
The previous code directly leaked a differing length by short-circuiting
before comparing anything. While we could limit the comparison by the
minimum length (and call `memeq_const()`), that could still leak the
length because the time will plateau once the secret's length is reached.
Similarly, if the comparison was bound by the longer chunk (would prevent
the use of `memeq_const()`), the length could also be revealed once the
input gets longer than the secret and the time increases.
This changes the semantics of the function by declaring the first
argument the expected/reference secret and the second the variable input.
This strictly makes the function constant-time, bound by the secret's
length. So the length can't be guessed by providing different input (but
if an attacker can trigger the comparison against different secrets, of
potentially known lengths, it might still be possible). If the chunks
are known to have the same length, the order doesn't matter.
Callers of this function have been updated accordingly.
encrypted-payload: Do basic decryption check only once
The check only references the original chunk, so for each parsed payload
it checks the same thing. The length of each individual payload is
checked by the parser anyway. So I think this was primarily added for
the IKEv1 "wrong PSK" use case. Let's keep it for now.
Fixes: dd5c3787dc38 ("Give a hint that decryption failed if payload length invalid")
pem: Avoid integer underflow when verifying padding after decryption
If the padding was larger than the whole blob, all available bytes were
checked for a match and the blob's length was eventually adjusted and
SUCCESS returned. Due to the underflow this could result in a huge size
that would then get copied. Zero-length padding was also incorrectly
accepted as was padding larger than the block size.
Fixes: 160f4c225db0 ("moved PEM parsing functionality to its own plugin")
eap-ttls: Fix memory leaks in error cases related to EAP-Identity handling
Should be rare that the eap-identity plugin is not loaded when
authenticating clients with EAP. And the second error path will
currently never get used as `process()` always succeeds.
Fixes: 79f2102cb442 ("implemented server side support for EAP-TTLS")
printf-hook-builtin: Avoid leaking stack contents when printing very long strings
Because `builtin_vsnprintf()` returns the length of the (theoretically)
produced string even if the buffer is too small, the `fwrite()` calls
would read past the buffer. While it rarely happens that log messages
are even close to the current buffer size, it might get triggered by an
overlong IKE/EAP identity or similar.
For `vasprintf()`, the allocation for the complete required length is
now correctly handled (capped at `INT_MAX` as that's what
`builtin_vsnprintf()` can technically return).
Also fixed is an incorrect mapping of the return value of `fwrite()` in
case of an error. While the latter returns the elements written so far,
the expected return value from `vfprintf()` is negative.
Fixes: cabe5c0ff40f ("printf-hook-builtin: Add a new "builtin" backend using its own printf() routines")
I've overlooked the "m = q if d = 12" note in the pseudo-code for
Algorithm 6. The text further up in the section actually clarifies
this:
For d = 12, ByteDecode produces integers modulo q as output...(it)
converts each 12-bit segment of its input into an integer modulo 4096
and then reduces the result modulo q. This is no longer a one-to-one
operation.
This did not have any practical impact (other than accepting public keys
that are technically non-compliant) because the values would get properly
reduced anyway by the Barrett reduction used in `mul_modq()`.
Fixes: 89f4b345e336 ("ml: Add software implementation of ML-KEM")
stroke: Avoid sharing a certificate printer between multiple clients
If multiple clients call list commands concurrently, each would replace
the global certificate printer instance the previous client created
and then operate on shared state. The destruction then causes a
double-free or NULL-pointer dereference.
Fixes: 02d431022c45 ("Refactored certificate management for the vici and stroke interfaces")
windows: Fix potential use-after-free when joining a thread
Because the flag was set before running the TLS cleanup, a thread
waiting in `join()` could exit the loop and destroy the thread object
before `docleanup()` is called in `end_thread()`.
This change removes the `terminated` flag and instead properly waits for
the thread to exit in `join()`. By always removing the threads from the
hashtable in `end_thread()`, we also avoid requiring to check any flags
in `cleanup_tls()`, as it now only finds an object for external threads.
We now also make sure to call `docleanup()` before removing the thread
from the hashtable. Otherwise, if a TLS cleanup callback calls
`thread_current[_id]()`, a new thread object would get created that is
never cleaned up.
Fixes: 0fa9c958114f ("windows: Provide a complete native Windows threading backend")
child-sa-manager: Correctly remove replaced entry in lookup tables
In the (very) unlikely case that a unique ID is reused (e.g. due to
counter wraparound) while the original SA is still in this manager,
the entries should properly get removed from the other lookup tables
before the entry is destroyed to prevent stale pointers from getting
used in later lookups.
Fixes: e732fb11a915 ("child-sa-manager: Add a global manager storing CHILD_SA relations")
chunk: Add utility that compares the prefix of two chunks
Unlike chunk_compare() this first compares the prefix of two nonces,
then falls back to comparing the length. This is basically intended to
compare nonces as specified in RFC 7296:
"Lowest" means an octet-by-octet comparison (instead of, for instance,
comparing the nonces as large integers). In other words, start by
comparing the first octet; if they're equal, move to the next octet,
and so on. If you reach the end of one nonce, that nonce is the
lower one.
gcrypt: Fix zeroing padding when extracting RSA value from S-expression
When left-padding a value shorter than the RSA key, the code previously
calculated the length incorrectly so that some bytes might have been
cleared if the value was shorter than half the required length.
Fixes: a2f1bb238ecc ("enforce correct RSA signature lenght in gcrypt")
kernel-netlink: Properly clean up pending entry if VIP installation failed
This could prevent the VIP from getting installed later and actually
causes those threads to block indefinitely as they wait for the entry to
either get removed or the VIP marked as installed, which will never
happen.
Fixes: c6b401581a75 ("Changed how kernel-netlink handles virtual IP addresses")
This applies some of the same fixes found in the previous commit but also
ensures that the offsets are valid before accessing the bitmask. Because
of an off-by-one error in the latter, the last address could get released
incorrectly (the pool constructor explicitly excludes it).
Fixes: 98d03438706f ("Implemented a HA enabled in-memory address pool")
mem-pool: Enforce size limit also for range-based pools and fix overflow
Due to the overflow when mapping addresses to offsets, no addresses could
get released when the pool was defined in a way that the last address
assignable is 255.255.255.255 (probably never the case in practice).
While not really useful in practice, the range 0.0.0.0-255.255.255.255
resulted in an empty pool (`size = 0xffffffff + 1`), which triggers the
%config like behavior. Since the implementation currently uses signed
integers throughout, we make sure the largest assignable offset is
INT_MAX.
The two off-by-one fixes (`>= pool->size`) were never an issue in practice
because `get_new()` has a guard that caps the offset at the size and
released addresses originally came from the pool so are always in range.
Fixes: 0897cda33b63 ("Add a constructor to create in-memory pools from an address range")
shunt-manager: Fix potential race between install and uninstall
If the installation fails while a shunt is concurrently uninstalled,
the entry could already be destroyed when trying to remove and destroy
it after acquiring the lock again in `install()`.
This change handles the conflict the same way trap-manager does since 69cbe2ca3fe4 ("trap-manager: Wait for install to finish before
uninstalling").
Fixes: 616ff9a2369f ("shunt-manager: Remove stored entries if installation fails")
connmark: Only install one rule per Child SA in PREROUTING
These rules don't depend on traffic selectors, so for SAs with multiple
traffic selectors we'd install several duplicate rules. Since this
plugin is used for transport mode SAs, it probably never was an issue
in practice.
Fixes: b8973b266131 ("connmark: Add CONNMARK rules to select correct output SA based on conntrack")
charon-tkm: Fix potential use-after-free in chunk map
This isn't a problem in practice as the nonces stored in the map are
tied to the IKE SA that's currently checked out, so a found entry can't
get removed/replaced concurrently.
sha3: Make sure state and rate input buffers are 8-byte aligned
Both buffers are accessed directly by casting to `uint64_t`. On platforms
that don't allow unaligned accesses this could cause a SIGBUS. The
reorder should avoid extra padding between the two buffers.
Fixes: 5ff88c96228c ("xof: Implemented SHAKE128 and SHAKE256 Extended Output Functions") Fixes: 83c1883d0bb0 ("Use word-aligned XOR in sha3_absorb()")
ike-init: Make sure we received a KE payload as initiator
On the responder, dc8b479eb890 ("ike-init: Fix key derivation if SA is
reset after IKE_INTERMEDIATE retransmits") fixed this. But the initiator
still accepted IKE_INTERMEDIATE exchanges without KE payload.
Fixes: 0d49ddec2ef5 ("ike-init: Add support for multiple key exchanges")
xauth-generic: Make sure to only use secrets that match the username
This ensures that secrets that are associated with the server identity,
usually they are assigned to the username exclusively, will only be tried
if the username also matched. Otherwise, it could be possible for users
who know at least one password to impersonate another user.
Fixes: 462c9a4f72ec ("Try all matching XAuth secrets we find, not only the first one")
kernel-wfp: Add support for SA deletion in either order
The code was written with `child_sa_t::destroy` in mind, which deletes
the inbound SA before the outbound SA. The problem is that the rekeying
code was changed meanwhile so the outbound SA is removed before the
inbound SA in order to avoid traffic loss. That could cause a
use-after-free as the already destroyed item remained in the `isas` list.
This change fixes this so the SAs can be removed in any order. The SPIs
are used as marker for whether a specific direction is installed. It
also fixes an issue in `expire_job()`, which removed the entry from
`osas` without holding the lock.
Fixes: f351d9ef7d70 ("kernel-wfp: Reference SA/SP sets by SPI and destination, not reqid") Fixes: 44107cb7b755 ("child-delete: Delay the removal of the inbound SA of rekeyed CHILD_SAs")
host: Do some proper validation when parsing CIDR-style subnets
The previous code returned negative or too large values (e.g. /33 or /-1)
to the caller, which some would then use unchecked. In particular the
attribute parser in the vici plugin would use it directly to generate a
subnet mask using shifts by `32 - mask`, which could trigger undefined
behavior.
Fixes: 65697c27345b ("Added a CIDR notation based host constructor")
simaka-message: Prevent buffer overflow when generating messages
A malicious (authenticated) server could send a long AT_NEXT_PSEUDONYM
or AT_NEXT_REAUTH_ID attribute (max. 1016 bytes) that the client then
attempts to return in an AT_IDENTITY attribute that could overflow the
fixed-sized stack buffer `out_buf` by a few bytes (writing into the
`encr_buf` buffer).
This refactoring first calculates the length required to encode the
attributes (with and without encryption) and accounts for the overhead
necessary for AT_ENCR_DATA, AT_PADDING, and AT_MAC (some limits are
enforced and error handling is added where appropriate). Then correctly
sized buffers are allocated and the attributes are encoded pretty much
as before. Except that the encoding of AT_RES is corrected (it actually
worked fine for up to 31 bytes, so in practice it wasn't a problem),
AT_PADDING is not added if data is already block-aligned, and it corrects
handling of AT_NOTIFICATION (they are never sent encrypted according to
the RFCs, but protected with a MAC).
Fixes: f8330d03953b ("Added a libsimaka library with shared message handling code for EAP-SIM/AKA")
pkcs11: Avoid race condition in token hot-plug handling
If a token is removed during initialization, where `token_event_cb()` is
called manually, the callback could be triggered after the credential set
was added to the list but before it was registered with the manager.
This could then cause a use-after-free if the manager accesses it after
the other thread destroyed it. Note that there is still a race if the
removal runs before the other thread even acquires the mutex. We'd end
up with a registered but defunct credential set that is not backed by a
valid token. But that shouldn't cause any crashes.
Fixes: a6d2ec331ba8 ("Implemented a credential set on top of a PKCS#11 token")
hasher: Avoid theoretical memory leaks for hashers that could potentially fail
These `get_hash()` implementations could potentially fail (realistically
only for serious system errors like OOM). This change ensures we comply
with the documented behavior (i.e. only allocate memory on success), as
no callers currently expect they have to clean up on failure.
pkcs11: Fix memory leak in the hasher_t implementation
If state was stored (for incremental hashing), the allocated memory was
leaked. Added some limits for the allocation and use chunk_t to simplify
it. The state is now also wiped just to be safe.
Also removed the useless mutex. If the goal was to protect access
to the hasher from multiple threads, then no other hasher currently
implements such protection. And if the idea was to serialize access to
the token (i.e. only allow a single hasher to concurrently load its state
into the token and update it), then a per-hasher mutex was not the right
approach. If that was the reason, we'd need a token-level mutex that
all hashers shared. Should `get_hash()` fail due to such an issue with
a transient error (e.g. `CKR_DEVICE_MEMORY`), we now at least don't leak
memory from `allocate_hash()`.
Fixes: 6e4f4d2fdfca ("Save/Load state of PKCS#11 hasher")
tnccs-dynamic: Add missing checks for underlying TNCCS instance
In order for the TNCCS instance to get created, `process()` has to be
called first. However, if the client responds to the initial request
with an empty EAP packet, `tls_eap_t` interprets that as acknowledgement
and directly calls `build_pkt()`, which attempts to call `build()` here
and triggers a NULL-pointer dereference. Similarly, `process()` is
skipped if the client sends an EAP packet that has the EAP_TLS|PT_START
flag set.
The `get_pdp_server()` method is called when the EAP-TNC method that
owns this instance is destroyed and would likewise trigger a crash if
e.g. the client never responded and the EAP-TNC instance is destroyed
without `process()` ever being called.
Fixes: f652995b2123 ("implemented dynamic detection of TNCCS protocol")
watcher: Properly handle conflict during concurrent FD removal
If an FD we intend to remove is currently busy in a callback, we wait
on a condvar to retry later. If the FD is not the first in the list,
`prev` will be set to the previous entry in the list. This is fine
when no other threads are concurrently removing FDs, the same entry will
be found on the next try and prev points to the same value again.
However, if other threads also remove one or more FDs and the initial FD
is now the first in the list `prev` should be NULL and not point to a
removed entry.
Fixes: b27663399b4b ("watcher: Avoid allocations due to enumerators")
wolfssl: Use Absorb/SqueezeBlocks API for SHAKE-128/256 XOFs
This API is available since 5.5.1 (released in 2022) and allows us to
avoid the inefficient previous implementation (don't think it's worth
keeping that around for older versions). Also added support for
SHAKE-128. Note that the only user of SHAKE is the ml plugin nowadays
and since wolfSSL also provides ML-KEM, this might not actually
get used much.
This was not actually an improvement over just doing the access as root.
The auth-dialog is not run by NM directly. Instead, it requests secrets
via DBus from anybody who implements the SecretAgent interface. It's
then e.g. nm-applet who starts the auth-dialog. But this can really be
anybody, i.e. it's easy to return arbitrary information to the plugin.
So we can't trust the returned username. This means that we can't
improve the situation for system-wide connections that don't provide
a username from the config.
We keep resetting the cached agent socket that the commit also added.
capabilities: Log warning if UID changes and no capability backend is compiled in
In this case, we preserve the complete set of capabilities not just the
ones we actually need. Removing the `prctl()` call isn't an option as
the daemon wouldn't be functional without the capabilities. But we now
warn users about this. We also only call `prctl()` if we actually switch
to a non-zero UID, `has_capability()` in turn already checks that we are
running as root in the `!CAPABILITIES` case.
A similar warning has been added to the configure script if a user has
been set at compile time.
eap-radius: Fix access to IKE_SA when sending interim updates
The previous code checked in the IKE_SA before using it again to
add attributes based on it to the interim update message. This
change ensures the IKE_SA is only accessed while it is checked out.
The Acct-Status-Type attribute is still sent first, but the
Acct-Session-Id and Class attributes are now sent after the IKE
parameters, which shouldn't really matter, though.
Fixes: d019764ab626 ("Add support for RADIUS Interim accounting updates")
array: Avoid issue when re-inserting existing element in value-based arrays
The insertion can cause the existing data to get reallocated/moved. So
if the caller attempts to insert another copy of an existing element into
the array via its pointer, this can cause a undefined behavior or even
a use-after-free because the pointer might get invalid.
There is such a case in `mem_pool_t::get_existing()` since the referenced
commit.
Fixes: d4a0dd9f93d6 ("mem-pool: Fix issue with make-before-break reauth and multiple IKE_SAs")
constraints: Simplify policy constraint handling and fix some TODOs
The previous code was too strict in some respects but also contained
other flaws.
Let's start with the latter, the loop that checked requireExplicitPolicy
constraints didn't use the correct offset. Because the subject wasn't
part of the list, it was one off (should have been `len - expl + 1`).
So the last relation was not checked. However, that check was neither
necessary, nor correct anyway. If a certificate didn't have policies or
mappings, it was accepted, so nothing was enforced in that case. It also
didn't validate the policies to the root, it only looked at two immediate
siblings and basically checked their immediate consistency. So whether
any policies were valid (i.e. would end up in a top-down built
`valid_policy_tree`) wasn't actually checked. But as mentioned, such an
explicit check wasn't necessary anyway.
Because the only thing we care about is collecting valid policies in the
subject to match against configs. So we implicitly enforce any
requireExplicitPolicy constraints by enumerating and validating them.
And while we don't reject certificates with invalid policies anymore
since 69232e2d3dd1 ("constraints: Don't reject certificates with invalid
certificate policies"), we now require at least one valid policy in the
subject if a requireExplicitPolicy constraint applies to it. For chains
where that's not the case, we still accept subjects without any valid
policy.
Next, the enforcement of the inhibitPolicyMapping and inhibitAnyPolicy
constraints was too strict. It checked the chain and rejected
certificates just if they encoded anyPolicy or policyMapping. The RFC
only uses the constraints to disable their function at a certain depth.
This is now corrected by first calculating thresholds for the two
constraints and then applying them when validating policies in the
subject.
Another thing that was technically incorrect is that mappings between
anyPolicy were allowed. It didn't have much of an effect because the
mapped issuerDomainPolicy is always checked against the same
certificate's policies (in the RFC, encoding that policy is a SHOULD,
so we are stricter here).
The tests that previously failed due to the latter were adapted, the
certificates are now accepted but the asserts make sure the policy is
missing. New and updated tests cover more edge cases, some make use
of new helpers that allow encoding two policies, checking how multiple
policies are handled.
The policy violation hook now also receives the subject certificate as
it's that certificate's missing/invalid policies that trigger it.
When the passed path was longer than the local buffer, while still
passing the `opendir()` call (e.g. `/\0` followed by padding), the access
via original chunk length to write a final `/` wrote outside of the
buffer. The same could happen later when adding directory names because
`path_len` was based on that same length.
Fixes: 2ea1dac2030d ("libimcv: Support symlinks introduced by usrmerge")
pts-database: Reject reference file measurements with incorrect size
If the received measurement hash exceeded 64 bytes, the `hex_meas_buf`
buffer overflowed in the `chunk_to_hex()` call. Since this is only
called when collecting measurements from a trusted reference system,
it shouldn't be an issue in practice.
There are no real CPUs that support AES-NI/PCLMULQDQ but don't support
SSSE3. However, in VMs the vCPU features might not exactly match those
of the underlying CPU. So if SSSE3 is missing, the PSHUFB instruction
would cause a SIGILL. The referenced commit is the first one that uses
the `_mm_shuffle_epi8` intrinsic.
Fixes: 74d43cbde999 ("aesni: Implement a AES-NI based CTR crypter using the key schedule")
ldap: Replace deprecated function calls and support LDAPS
The use of the deprecated `ldap_init()` meant that LDAPS, although
announced by the plugin, was not actually supported. The plugin just
always used a plaintext connection. Now we use the current API and
get support for LDAPS (requires a bit of an awkward URI construction).
Based on the URI's scheme we also set an option to enforce a certificate
check. The new NEWCTX option creates a connection-specific TLS context.
Without that we get a global default context once bind is called that is
not freed until the daemon exits (it leaks in LD and also seems unsafe
in regards to multiple threads fetching CRLs via LDAP).
Fixes: 552cc11b1f01 ("merged the modularization branch (credentials) back to trunk") Fixes: 8c06e9c0edc5 ("added #define LDAP_DEPRECATED in order to use old ldap_init() function")
unbound: Make sure RRs match the queried or canonical name
While `ub_resolve()` verifies the response is valid, only the `data`
array provided in `ub_result` contains filtered results. The raw
response packet we parse here could theoretically contain (validated)
RRs for a different owner that would get accepted and returned in the
provided `rr_set_t`.
Fixes: 5a4126b49009 ("unbound: Implemented resolver_response_t as unbound_response_t")
sec-updater: Use process_t to avoid potential command injection
Since the version passed to the `dpkg` command comes from a local
`Packages` file, it is presumably trusted. But if the script is called
on a random file or the attacker can control its contents, it might be
possible to inject commands.
key-exchange: Rename function that verifies pubkey lengths
The previous name confused LLMs as they assume it is intended to actually
cryptographically verify the public key. The new name more clearly
describes what it actually does.
botan: Remove confusing documentation for DH helper function
This comment only referred to not calling `key_exchange_verify_pubkey()`,
not what Botan does, which will verify the passed public value as needed.
LLMs get confused by this and assume Botan doesn't so that.
projects / thirdparty / strongswan.git / log
