The default is apparently "Connection: keep-alive", which somehow keeps
the socket around, which leaks file descriptors with every connection
that fetches OCSP and/or CRLs. Over time that could result in the number
of FDs reaching a limit e.g. imposed by FD_SET().
Closesstrongswan/strongswan#1160
The linter complained that two of the strings don't actually contain any
printf-specifiers (i.e. don't expect any arguments) and therefore
shouldn't be used with String.format().
This was originally required when pluto and charon both bound sockets to
the same port to send messages. Pluto also received messages on them but
charon didn't and used a raw socket instead. Since the removal of pluto
we don't need to set this option anymore, which might actually mask
mistakes like running charon and charon-systemd concurrently (that could
result in messages getting sent fine by both daemons but only received
by one).
Note that a failure to create/bind the sockets will not immediately
result in a shutdown of the daemon. Instead, there will be an error
once the receiver tries to read any messages and also whenever the sender
attempts to send a request.
Changes the type for EAP vendor IDs from uint32_t to pen_t, which has
explicitly been added to represent three-byte IANA-allocated Private
Enterprise Numbers (PEN), which the EAP RFC called "SMI Network
Management Private Enterprise Codes".
References strongswan/strongswan#581
There should be no need for such a persistent documentation on a removed
component in the repository. The commit history is enough. And besides
that, there is user-facing documentation about it in the docs and the
changelog/NEWS.
There are a lot of calls like this:
this->dispatcher->raise_event(this->dispatcher, "...", 0,
b->finalize(b));
However, if finalize() fails, e.g. because a previous call to add()
failed due to the size limit, it returns NULL. This then caused a
segmentation fault in raise_event() when it interacted with that value.
Closesstrongswan/strongswan#1278
This fixes a race condition during shutdown between the main thread
flushing the IKE_SA manager and worker threads still creating IKE_SAs.
Closesstrongswan/strongswan#1252
Because flush() has to release the segment locks intermittently, threads
might add new entries (even with the change in the previous commit as the
IKE_SA might already be created, just not registered/checked in yet).
Since those entries are added to the front of the segment lists, the
enumerator in the previous step 2 didn't notice them and did not wait
for them to get checked in. However, step 3 and 4 then proceeded to
delete and destroy the entry and IKE_SA, which could lead to a crash
once the other thread attempts to check in the already destroyed IKE_SA.
This change combines the three loops of steps 2-4 but then loops over
the whole table until it's actually empty. This way we wait for and
destroy newly added entries.
Without ability to create SPIs, other threads are prevented from creating
new IKE_SAs while we are flushing existing IKE_SAs. However, there could
still be IKE_SAs already created that might get checked in while the
segments are temporarily unlocked to wait for threads to check existing
SAs in.
This is more consistent and e.g. allows to properly take into account
some settings that are also relevant during IKE_AUTH (e.g. childless).
We also already use the peer_cfg_t's ike_cfg_t when rekeying,
reauthenticating and reestablishing an IKE_SA (and e.g. for DSCP).
Also changed are some IKEv1 cases where get_ike_cfg() is called before
set_peer_cfg() without taking a reference to the ike_cfg_t that might
get replaced/destroyed (none of the cases were problematic, though, but
it also wasn't necessary to keep the ike_cfg_t around).
Closesstrongswan/strongswan#1238
The pattern currently is to call get_cache(), generate the encoding
if that failed and then store it with cache(). The latter adopts the
passed encoding and frees any stored encoding. However, the latter means
that if two threads concurrently fail to get a cached encoding and then
both generate and store one, one of the threads might use an encoding
that was freed by the other thread.
Since encodings are not expected to change, we can avoid this issue by
not replacing an existing cache entry and instead return that (while
freeing the passed value instead of the cached one).
Closesstrongswan/strongswan#1231
At least for the tests where it is available and works. It conflicts
with the instrumentation used by the coverage and fuzzing (and possibly
sonarcloud) tests, the toolchain for the Windows builds doesn't seem to
support it, and on FreeBSD the test executables hang due to a
compatibility issue with FreeBSD's qsort(), which has been fixed [1],
but that has not made it into the clang version in the base system.
For the custom OpenSSL build, debug symbols are enabled so we can
suppress some leaks properly.
[1] https://github.com/llvm/llvm-project/issues/46176
Only the second was reported by the compiler (depending on the version
and similarly to the previous commit only with AddressSanitizer active).
The strncpy() call for UTUN_CONTROL_NAME was simply wrong.
Normally, GCC sees that we terminate the destination with a zero byte.
However, when using `-fsanitize=address`, there seems to be additional
instrumentation code after strncpy() so GCC produces warnings like
these:
‘__builtin_strncpy’ specified bound 16 equals destination size [-Wstringop-truncation]
This causes odr-violation errors with libasan as some symbols will be
defined twice, once in the linked libimcv and once in the test
executable itself.
Closing the socket from the main thread, while the server thread is
still in accept() (or is just about to enter it), seems to
occasionally cause a deadlock on macOS.
Seems to be required on macOS (libtls tests didn't run before the recent
implicit enabling via pki). Other platforms apparently let accept() fail
if the socket is shutdown/closed in teardown_creds(), macOS apparently
doesn't do that.
Since the allocated data was smaller than sizeof(eap_mschapv2_header_t),
the following compile error was triggered (with newer GCC versions):
eap_mschapv2.c: In function 'process_peer_success':
eap_mschapv2.c:945:12: error: array subscript 'eap_mschapv2_header_t[0]' is partly outside array bounds of 'unsigned char[6]' [-Werror=array-bounds]
945 | eap->code = EAP_RESPONSE;
| ^~
In file included from /usr/include/stdlib.h:587,
from ../../../../src/libstrongswan/utils/printf_hook/printf_hook.h:26,
from ../../../../src/libstrongswan/library.h:101,
from ../../../../src/libcharon/sa/eap/eap_method.h:28,
from eap_mschapv2.h:27,
from eap_mschapv2.c:18:
eap_mschapv2.c:944:15: note: object of size 6 allocated by '__builtin_alloca'
944 | eap = alloca(len);
| ^~~~~~
Closesstrongswan/strongswan#1188Closesstrongswan/strongswan#1215
The previous code did not ensure that there was a delay of at least
`try` seconds after each sent request. Instead, whenever the condvar was
signaled, which could be due to retransmitted responses or messages for
unrelated transactions (there could even be spurious wakeups), the counter
was increased and a retransmit sent. So instead of actually waiting for
15 seconds for a response (and sending 4 retransmits over that timespan),
it could happen that all five messages were sent within a second without
enough time to actually receive a response.
Using an absolute timeout that we reuse as long as there was no timeout
and the condvar was signaled for something unrelated, should ensure we
wait at least the intended delay after each sent message.
Closesstrongswan/strongswan#1154
This allows using the upper parts of the marks for other purposes. For
instance, with `mark_in=mark_out=%unique/0x0000ffff` mark values in the
upper two bytes would not get reset by the rules installed by this plugin.
However, note that in this example the daemon would have to get restarted
after 65'535 CHILD_SAs to reset the counter for unique marks, which is a
global 32-bit counter that's unaware of any masks.
Closesstrongswan/strongswan#1087
get_traffic_selectors() is called the same way also as responder when
selecting child configs via peer_cfg_t::select_child_cfg(). Replacing
TS for all child configs could lead to selecting one that later fails
to actually narrow the traffic selectors. Ignoring non-matching TS also
helps if we have a trap config with multiple remote subnets (otherwise,
we'd have to filter duplicates afterwards).
When installing traps, the hosts might be %any, in which case we allow
the configured (technically non-matching) TS for the wildcard use case.
Fixes: da82786b2d ("child-cfg: Always apply hosts to traffic selectors if proposing transport mode")
Closesstrongswan/strongswan#1143
Setting -Werror in CFLAGS passed to configure is not ideal as that affects
all the checks performed by the script.
This caused an issue with newer versions of Autoconf and the AC_PROG_LEX
macro that insisted on finding a lexer library. But due to warnings from
the generated test lexer (misleading indentation) that got turned into
errors no library was found (none would have been necessary), so LEX was
not set and no lexers were built.
With this option enabled, we add -Werror to CFLAGS after all tests ran.
It also enables additional warnings via -Wextra.
The option is auto-enabled when building from the repository.
Making the variable volatile avoids a "variable ‘failure’ might be
clobbered by ‘longjmp’" warning (or error when compiling with
-Werror) that's triggered via -Wextra.
The 5.4.0 update changed the default bignum implementation to what
could explicitly be enabled via `--enable-sp-math-all`. Since this uses
fixed-sized buffers sufficient for key sizes of SP_INT_BITS, with a default
of 4096, modp6144 and modp8192 didn't work anymore (wc_DhGenerateKeyPair()
returned MP_EXPTMOD_E). So we have to adapt the feature checks for this.
To support the larger DH groups we can either increase the buffer size
via `--with-max-rsa-bits` or add `--enable-heapmath` so buffers get
(re-)allocated as needed. We go with the latter for now.
This merge includes changes that were created for the upcoming IKEv2
extension for multiple key exchanges over the last four years, but which
are not directly related to the actual protocol changes.
Changes include renaming diffie_hellman_t to the more generic
key_exchange_t (also renamed are some of the interface's methods),
making utility functions that deal with DH groups more generic, and let
tasks handle the first IKE_AUTH message more reliably by not depending
on e.g. specific message IDs.
One significant change is delaying the IKEv2 key derivation until
the keys are actually needed to process or send the next message. So
instead of deriving the keys directly while processing an IKE_SA_INIT
request (which could come from a spoofed address), this is delayed until
the corresponding IKE_AUTH request is received. Implementations of the
key_exchange_t interface are now expected to do the key derivation and
any costly public key validation in get_shared_secret() and not
set_public_key().
Sent IKE_SA_INIT messages are now also not pre-generated anymore to collect
their encoding for the authentication. Instead, a new post_build() hook
allows the ike-auth task to do so after the actual message has been built,
which allows later tasks and plugins (via message() hook) to modify the
message (e.g. add notifies) after the ike-auth task's build() method
already ran.
Also changed is how inbound requests are processed and retransmits are
detected. Instead of parsing all inbound messages right away (which
might trigger a key derivation or require keys we don't have anymore in
the multi-KE use case), we now first check a request's message ID and
compare its hash to that of the previous request to decide if it's a
valid retransmit. For fragmented messages, we only keep track of the
first fragment so we can send the corresponding response immediately if
a retransmit of it is received, instead of waiting for all fragments
and reconstructing the message, which we did before.
This way we avoid having to pre-generate the message when it could
theoretically still get modified by a task that follows or from a plugin
via message() hook.
In particular as responder, this delays costly cryptographic operations
until the IKE_AUTH request is received, which is preferable to reduce
the impact of DoS attacks.
Another advantage is that the key material is not changed until all tasks
built or processed a message.
This way we avoid parsing messages with unexpected message IDs, which
might not even be possible if we don't have the keys anymore. However,
the next commit should avoid the latter and this way we avoid deriving
keys for retransmits or unexpected messages.
This also changes how retransmits for fragmented messages are triggered.
Previously, we waited for all fragments and reconstructed the message
before retransmitting the response. Now we only track the first
fragment and if we receive a retransmit of it respond immediately
without waiting for other fragments (which are now ignored). This is in
compliance with RFC 7383, section 2.6.1, and can avoid issues if there
are lots of fragments.
This way we can use the IKE_REKEYED state for both redundant and old SAs
to suppress ike_updown().
In the ike-delete task we don't suppress events in state IKE_REKEYING as
that's the case when we delete an SA the peer is currently rekeying with
multiple key exchanges.
This gives us more flexibility with tasks that return NEED_MORE (currently
none of the colliding tasks do, but that will change with multi-KE
rekeyings). The active task has to check itself if the passive task is
done and should be removed from the task manager.
Since f67199378d ("ike-rekey: Handle undetected collisions also if
delete is delayed") we only ever track tasks of type TASK_IKE_REKEY, so
there is no need to check the type or use the generic task_t interface.
Also changed some of the comments to clarify collision handling.
If the peer successfully rekeyed the SA it gets marked as IKE_REKEYED
and it remains until the peer deletes it (or a timeout). There is no
point in rekeying such SAs again.
IKE_REKEYING will be relevant if we have multi-KE rekeyings and are
waiting for followup key exchanges for a passive rekeying.
Tobias Brunner