This makes sure the event is only triggered after the IKE_SA is fully
established and e.g. virtual IPs, additional peer addresses or
a modified reauth time (on the initiator) are assigned to it. This was
e.g. a problem for the selinux plugin if virtual IPs are used.
We use a separate task to trigger the event that's queued before the
child-create task so the event is triggered before the child_updown()
event. Same goes for the state change to IKE_ESTABLISHED.
A new condition is used to indicate the successful completion of all
authentication rounds, so we don't have to set the IKE_ESTABLISHED state
in the ike-auth task (it was used as condition in other tasks).
Since set_state() also sets the rekey and reauth times, this required
some minor changes in regards to how AUTH_LIFETIME notifies are handled.
Since e334bd46b1 ("ike-auth: Move packet collection to post_build()
method") tasks and plugins can modify the IKE_SA_INIT message independent
of the ike-auth task.
Otherwise, we can't open a dedicated IPv4 socket on the same port as the
IPv6 socket already is set up do receive IPv4 packets (unless we'd again
enable SO_REUSEADDR).
Fixes: 83da133712 ("socket-default: Don't set SO_REUSEADDR on IKE sockets anymore")
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.
Content-Transfer-Encoding is actually not a valid HTTP header, but a MIME
header, and must not be used. The original RFC7030 specifies this wrong,
and an errata discusses this issue.
The use of base64 encoding has been clarified in RFC8951, and the
recommendation is to always use/expect base64 encoding, but not send/expect
the Content-Transfer-Encoding header.
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
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
When watching the output of `swanctl -l` during debugging, the debug
messages in query_sa/policy() cause a lot of noise in the logs (level 2
for DBG_KNL still has actually useful information that we want to see
in the logs) and they're not very useful.
Compared to the messages in the functions above, the ones in update_sa()
and get_replay_state() are not seen often. But since there already is a
log message on level 2 in update_sa(), they're kinda redundant.
Closesstrongswan/strongswan#1271
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.
When using the statement expression and a stack object along with
clang-11 and libasan, we get quite a lot of errors about reading
invalid memory. This is due to clang making the actual listener_t local
to the block, such that the access outside of the macros using
_assert_payload is (correctly) considered an error.
By using a heap allocated object, we can destroy it once the listener
returns FALSE (cleaning up properly), and since bus_t does not touch the
listener after that, we don't get any errors from libasan.
Co-authored-by: Tobias Brunner <[email protected]>
As the cleanup function reads from the correct address on the parent frame,
it is currently unclear why AddressSanitizer complains about that pointer
dereference.
Some third party IKEv2 products expect an RSA-PSS ASN.1
algorithmIdentifier with an explicit trailerField value (CONTEXT3)
instead of the DEFAULT value if the trailerField is missing.
The setting charon.rsa_pss_trailerfield = yes enables the explicit
encoding.
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.
Tobias Brunner