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.
It might not be a good idea to blindly accept such extensions. A scoped
CRL could be accepted for the wrong scope. So just reject them again.
This reverts commit 126778679f.
The enumerator strictly handles pointers to pointers, so passing a
`u_int` is incorrect on 64-bit platforms.
Fixes: 918e92c4c9 ("Support multiple different public key strength types in constraints")
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.
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: 160f4c225d ("moved PEM parsing functionality to its own plugin")
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: cabe5c0ff4 ("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: 89f4b345e3 ("ml: Add software implementation of ML-KEM")
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: 0fa9c95811 ("windows: Provide a complete native Windows threading backend")
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.
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: a2f1bb238e ("enforce correct RSA signature lenght in gcrypt")
Since `key_size` remained zero, this caused a buffer overflow when the
derived key in `pem_decrypt()` was copied to the zero-length local
buffer.
Also fixed two potential memory leaks if hashing fails and make sure
the decryption key is wiped.
Fixes: 160f4c225d ("moved PEM parsing functionality to its own plugin")
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: 5ff88c9622 ("xof: Implemented SHAKE128 and SHAKE256 Extended Output Functions")
Fixes: 83c1883d0b ("Use word-aligned XOR in sha3_absorb()")
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: 65697c2734 ("Added a CIDR notation based host constructor")
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: a6d2ec331b ("Implemented a credential set on top of a PKCS#11 token")
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.
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: 6e4f4d2fdf ("Save/Load state of PKCS#11 hasher")
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.
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.
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.
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")
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.
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).
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")
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")
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.
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")
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")
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")
Tobias Brunner