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: f652995b21 ("implemented dynamic detection of TNCCS protocol")
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: b27663399b ("watcher: Avoid allocations due to enumerators")
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 partially reverts commit d7608ca192.
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.
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.
This prevents other processes from binding the same ports and
misusing the protocol/port-specific bypass rules installed in WFP to
bypass the VPN.
Fixes: 11e7d0677c ("socket-win: Install IKE bypass policies using bypass_socket()")
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: d019764ab6 ("Add support for RADIUS Interim accounting updates")
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: d4a0dd9f93 ("mem-pool: Fix issue with make-before-break reauth and multiple IKE_SAs")
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 69232e2d3d ("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: 2ea1dac203 ("libimcv: Support symlinks introduced by usrmerge")
Also fixes a double free in that case (the `break` only leaves the loop,
not the switch statement).
Fixes: 076aac7069 ("imv-attestation: Fixed file hash measurements")
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.
Fixes: 076aac7069 ("imv-attestation: Fixed file hash measurements")
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: 74d43cbde9 ("aesni: Implement a AES-NI based CTR crypter using the key schedule")
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: 552cc11b1f ("merged the modularization branch (credentials) back to trunk")
Fixes: 8c06e9c0ed ("added #define LDAP_DEPRECATED in order to use old ldap_init() function")
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: 5a4126b490 ("unbound: Implemented resolver_response_t as unbound_response_t")
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.
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.
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.
This avoids logging the password that's potentially contained in the URI
and also gives clearer instructions about what's missing.
Also clears the memory that stores the URI/password.
On typical systems that have overcommitting enabled, the 16 MiB maximum
that's allocated via the 24-bit length field in the AVP header shouldn't
be an issue as there are various limits that affect how much data can
actually be written to the allocated buffer (e.g. the maximum IKE message
size, the maximum TLS record size, or the maximum number of accepted
EAP-TTLS payloads), so this is primarily a defense-in-depth measure.
The length field of an EAP payload, which is the only type of AVP we
accept, is 16 bits, so that's what we now enforce as maximum.
This is unlikely to be an issue in practice because only one caller
actually uses the ID and it does so immediately afterwards. So there is
only a tiny window in which the peer could terminate or rekey its SA to
cause the returned ID to get destroyed.
Fixes: d5cc175833 ("experimental P2P-NAT-T for IKEv2 merged back from branch")
Same as the previous commit for consistency, but not an issue here as
only transport mode SAs are handled.
Fixes: b8973b2661 ("connmark: Add CONNMARK rules to select correct output SA based on conntrack")
Not an issue in typical scenarios where the plugin is used on a gateway
with roadwarriors that use /32 addresses. But could be an issue if used
on a client that tunnels everything to the gateway.
Fixes: e5ad2e6614 ("forecast: Add the broadcast/multicast forwarding plugin called forecast")
There are multiple paths that can trigger a crash. One is if transport
mode is negotiated via NAT and `get_transport_nat_ts()` is called to
substitute addresses in the received TS. If that's not the case,
`narrow_ts()` will select the configured TS and continue. Then,
`narrow_and_check_ts()` eventually attempts to destroy `this->tsi/tsr`,
which triggers a crash in any case.
The referenced commit refactored `select_and_install()` into
`narrow_and_check_ts()` and `install_child_sa()`. It move a check
for TS from that function to only `build_r()`.
Fixes: d7760416d6 ("child-create: Add support for multiple key exchanges")
Due to the shared database connection, the previous code, while tracking
transaction metadata per thread, didn't actually enforce that separation
on the database level. Which basically meant the transactions created
by multiple threads were shared.
This change uses an approach similar to the mysql plugin, using a pool of
connections. However, we always use thread-specific connections, not
only during transactions. That's because the implicit transactions
that are active in SQLite during queries block further queries from
other connections while enumerating (the pool utility uses such patterns).
It also fixes the issue that calling `rollback()` on the outer-most
transaction didn't have an effect.
Since it's very unlikely SQLite was built in single-thread mode and
handling that properly would require locking the mutex during
transactions, we remove that locking and move the check to the constructor
to refuse initialization.
Fixes: fad11d602d ("sqlite: Implement transaction handling")
Before, `ref_cur()` used RELAXED memory ordering, which is sufficient
for diagnostic reads but provides no ordering guarantees against
concurrent `ref_put()` operations on other threads. Since `ref_put()`
already uses ACQ_REL ordering, readers should use ACQUIRE ordering
so that observing a given refcount value (particularly zero) also
makes all prior stores by the releasing thread visible.
There is no significant performance impact as on x86 ACQUIRE loads
compile to the same instruction as RELAXED loads. But this fixes
potential issues on weakly-ordered architectures (e.g. ARM).
The __sync* and spinlock fallbacks already provide full ordering (they
might not actually be necessary anymore nowadays).
The policies reference the provider, so it might not actually get removed
and be left dangling in the WFP system (maybe Windows refcounts it and
still removes it).
With the referenced commit, SHA-512 was added as a possible hash
function, but the buffer was not adjusted accordingly.
Fixes: 3ef5b23903 ("pts: Variable size PCR banks")
Such endpoints are used to request a peer reflexive endpoint, but are
never expected in a response.
Fixes: d5cc175833 ("experimental P2P-NAT-T for IKEv2 merged back from branch")
This could cause a crash due to signature verification with missing
key (although, memcpy() likely just returns with length 0), but it
also makes the signature predictable as the key is omitted.
Fixes: d5cc175833 ("experimental P2P-NAT-T for IKEv2 merged back from branch")
This avoids accepting expired certificates again should the time get
rolled back for some reason.
Fixes: c81b87ac26 ("systime-fix: Add timeout option to stop waiting for valid system time")
Fixes: 295e42a47f ("systime-fix disables certificate lifetime validation if system time not synced")
This is documented as a requirement in the man page, so the caller of the
conversation function might expect such an array. Although, since we
only accept non-interactive requests, for which no response is expected,
it's doubtful whether it will actually try to access elements in the
array. And if the passed pointer was initialized to NULL, passing it to
free() is probably also fine.
Fixes: 2312504d1e ("xauth-pam: Open/close a PAM session for each connected client")
Even if `poll()` indicates that the socket is ready it might block if
it's in blocking mode. This change avoids blocking in such cases (accept
will fail with EAGAIN/EWOULDBLOCK and `watch()` will return TRUE).
As the non-blocking mode is inherited on Windows (on Linux, the man page
documents the non-inheritance as a Linux specialty), we set the mode for
the accepted socket explicitly to blocking to match the expectations of
`stream_t`.
Fixes: daf1880b39 ("stream: add a stream service class abstracting services using BSD sockets")
Only skip the header, the `ip6e_len` field is reserved in these headers
and does not represent the length (I guess it's usually 0, but still).
Fixes: bdcaca76c5 ("ip-packet: Correctly determine protocol in fragmented IPv6 packets")
The referenced commit moved the key derivation to `get_shared_secret()`
and broke the handling of ECDH public value as the copied struct now
referred to a buffer allocated on the stack.
Also fixes potential session leaks if generating key pairs fails.
Fixes: 26ca0c9f70 ("pkcs11: Move shared secret calculation to get_shared_secret()")
The shift would overflow the value which could produce garbage output
that might get interpreted as real OIDs (in case strings are compared).
This limit allows OID sub-identifiers to consist of at most 4 bytes,
which should be enough for any real-world OIDs (it's also the maximum we
used in tests so far).
Fixes: f813069e89 ("fixed asn1_oid_to_string() conversion")
Seems difficult to exploit when wrapped in EAP/IKEv2, and the keys for
those TLS sessions are short-lived anyway.
Fixes: c0bf721357 ("tls: Check all bytes of the padding if they equal the padding length")
While we could use _Generic() C11 expression to let the compiler select
between the different versions, this only allows selection based on one
of the arguments, which seems a bit fragile. So make this explicit for
now. In the future we might consider using the overloadable attribute.
The presence of a peer ID is enforced by the message rules for ME_CONNECT,
this just mirrors the safety check also present in `process_r_ms()`.
Fixes: d5cc175833 ("experimental P2P-NAT-T for IKEv2 merged back from branch")
Fixes: 7fbe2e27ec ("tls-server: TLS 1.3 support for TLS server implementation")
Fixes: d2fc9b0961 ("tls-server: Mutual authentication support for TLS 1.3")
Fixes: 760f3b730f ("tls-server: Add flag that makes client authentication optional")
These were never correct. And the first referenced commit, unfortunately,
just simplified the incorrect code.
Fixes: 08a3ee0cce ("bus: Change ike_update() signature and only call it once")
Fixes: b8973b2661 ("connmark: Add CONNMARK rules to select correct output SA based on conntrack")
If only parts of the total data could be written to the kernel, the result
of the next read chunk would incorrectly get written at the beginning of
the output buffer again.
Also makes sure to close the accepted FD in error cases.
Fixes: 1b5de7ce3b ("Use a generic AF_ALG wrapper for common operations")
The `transaction` variable was previously still set to the one that
was enumerated last even if the received DHCP OFFER did not match
it. So the transaction was incorrectly modified. Once the real
DHCP OFFER would arrive, it would get modified again, but it could
allow adding unintended DNS servers.
The previous code could potentially truncate and change ownership of
a file that's a symlink to an unintended target file (requires the
attacker to be able to create the symlink in the directory the PID file
is located, which generally requires root privileges).
When initiating a Child SA with a CREATE_CHILD_SA exchange, the initial
exchange could fail with an INVALID_KE_PAYLOAD notify. The initiation
would then get retried from scratch (the `child_sa_t` object is destroyed
and a new one created). If it fails again, the previous code didn't
signal the waiting thread. If it did not set a timeout, it would wait
indefinitely.
Using a different `status_t` value avoids any confusion with NEED_MORE
that's used to indicate that the callback returned FALSE (e.g. if the
IKE_SA would go down before concluding the retry).
Fixes: c39b0c883d ("controller: Keep following initiate() if the first DH guess was wrong")
This is not necessarily an issue, but we should avoid not using the
full identity data as best as possible. The change also avoids the
dynamically sized buffer on the stack.
Fixes: 324528700d ("Added identification constructor using a chunk of data, guessing id type")
If we find a stale CRL in the cache and finding a newer one via
CRLIssuer fails for some reason, the validation state would get
overwritten with VALIDATION_SKIPPED. This would then prevent
fetching delta CRLs.
Fixes: 7d7beaa1fa ("Use certificate CRLIssuer information to look up cacched CRLs or CDPs")
EAP-PEAP transports results as separate AVPs, we expect one after the
first phase 2 authentication (if not using certificates). However, with
the previous caching of the result of that, the client could then send
another confirmation as response to the EAP-TNC request to skip it.
Since no group membership is set if EAP-TNC is not complete, it shouldn't
really affect its outcome, though.
Fixes: 1be296dfb2 ("implemented the PEAP tunneling protocol as an EAP plugin")
Holding the lock could potentially cause a deadlock depending the
behavior of the called cleanup functions. The TLS removal happens in
the context of the respective thread, so no locking is necessary.
Looks like removing these lines was missed when the referenced commit
partly reverted 204098a752 ("thread-value: Immediately cleanup all
Windows TLS values on destroy"), which added the locking originally.
Fixes: 23750961d5 ("thread-value: Defer cleanup handling to thread termination on Windows")
The entry was updated and the previous EAP method and IKE SA destroyed
under a read lock, which is not safe (there could be threads in
get_state(), or even some that called it before and now still hold
that read lock while interacting with the EAP method).
Fixes: 37884ab10f ("Add locking to TNC-PDP connections")
Because no lock was held while comparing the type of the already stored
cache entry, it was theoretically possible that a NULL-pointer dereference
or use-after-free was caused if another thread concurrently replaced the
stored certificate.
While the cached type is also read without lock, the worst that can
happen is that we acquire the lock of an entry that doesn't match
eventually. The double check actually uses the stored certificate,
just in case the cached value gets out of sync due to a future
refactoring.
Fixes: 2271ebb325 ("Newer CRLs replace older versions of the CRL in the cache")
Fixes: 47e8b21c76 ("cert_cache: Replace cached stale OCSP responses in-place")
We only allow unencrypted change_cipher_spec records (as before), which
are handled by the upper layers accordingly. Without the check, we
also accepted unprotected alerts and handshake records that could
potentially cause state confusion.
Fixes: 7a2b02667c ("libtls: Implement TLS 1.3 handshake on client-side")
As `tv_sec` is a `time_t`, i.e. typically 64 bits, assigning the result
of `htonl()` leaves the upper 32 bits zero. Copying from the
`sizeof(time_t) - 4` offset then copies those zeroes on little-endian
systems, which is not what was intended according to the comments.
Another issue was that the type of `tv_usec`, `suseconds_t`, is typically
a `long`, i.e. signed, so shifting the maximum value 0x000f423f (999'999)
by 12 bits technically overflows this. The cast fixes that.
Fixes: 1aba82bfd7 ("eap-aka-3gpp: Add plugin that implements 3GPP MILENAGE algorithm in software")
Basically the same as the previous commit.
Fixes: edcb2dd35b ("Moved reauth/pseudonym functionality from eap-sim-file to separate plugins, usable by any SIM/AKA backend")
This also protects access to the RNG, which is not always thread-safe.
Fixes: edcb2dd35b ("Moved reauth/pseudonym functionality from eap-sim-file to separate plugins, usable by any SIM/AKA backend")
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")
Tobias Brunner