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")
Replace `set()` with `extend()` in verify methods so the PCR value is
independently computed rather than taken from the evidence.
Also make `pcr_before` mismatch a hard failure instead of a warning.
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")
Tobias Brunner