Removing and readding the entry to a potentially different row/segment,
while driving out waiting and new threads, could prevent threads from
acquiring the SA even if they were waiting to check it out by unique
ID (which doesn't change), or if they were just trying to enumerate it.
With this change the row and segment doesn't change anymore and waiting
threads may acquire the SA. However, those looking for an IKE_SA by SPIs
might get one back that has a different SPI (but that's probably not
something that happens very often this early).
This was noticed because we check out SAs by unique ID in the Android
app to terminate them after failed retransmits if we are not reestablishing
the SA (otherwise we continue), and this sometimes failed.
Fixes: eaedcf8c00 ("ike-sa-manager: Add method to change the initiator SPI of an IKE_SA")
The button to view the log is now below the status info. And since the
IMC results are just below that we don't need a special handling for
that anymore.
This can happen on systems (e.g. Android 7.x) where Always-on VPNs are
triggered right after booting before the KeyChain is unlocked by the user.
Retrieving the certificate chain or private key then fails with
"KeyChainException: IllegalStateException: keystore is LOCKED" until the
user unlocks the screen once.
The built-in client actually also fails in this situation (e.g. with XAuth
RSA), it tries three times then stops and shows an error notification.
With Android 8.1 this isn't triggered after a reboot until the device
has been unlocked once (solving the issue with the key store) and traffic
may optionally be blocked by the user until the VPN is established.
There are still some issues (e.g. password prompts and fatal errors), and we
might need some workaround for older Android releases.
Only if there is no currently active (or previously active) profile does
this currently operate on the configured (or stored most recently used)
profile. This way it's possible to use a different connection and
quickly disable and re-enable it again. When unlocked the profile name
is shown, when locked a generic text is used (this detection doesn't seem
to work 100% reliably). To disconnect, the user is forced to unlock the
device, connecting is possible without, if the credentials are available
and no fatal error occurs (it even works with the system credential store,
at least on Android 8.1).
Note that the tile is not available right after a reboot. It seems that
the system has to be unlocked once to activate third-party tiles (will
be interesting to see how this works together with Always-on VPN).
Unfortunately, setLockscreenVisibility() doesn't seem to have any
effect. So the full notification is shown unless the user manually
configures the notification settings.
This allows us to add tiles to Quick Settings and enabling the Always-on
VPN feature in the VPN settings (both require API level 24, but 26 will
be required as targetSdkVersion later this year).
Unless there are profiles created with old versions of the app (< 1.8.0)
that were never updated since, all profiles should already have a UUID
assigned. If not, we do that now with a DB migration.
This is either because a third-party VPN app has the always-on feature
enabled, or because the user denied the permission in the system's confirmation
dialog.
If the always-on feature is enabled for a connection of the built-in VPN
client we get an IllegalStateException, for which we show an updated and
clearer error message.
The type of the value was incorrect (void**) if NULL was passed to cas_ptr()
as expected value, which caused a compiler warning with Clang because
__atomic_compare_exchange_n() expects the types of the first two arguments
to be the same.
armeabi has been superseded by armeabi-v7a and the MIPS ABIs were removed
with the latest NDK (r17), after being marked deprecated for a while.
By not specifying APP_ABI we build for all non-deprecated ABIs.
Instead of logging the search parameters for IKE configs (which were already
before starting the lookup) we log the configured settings.
The peer config lookup is also changed slightly by doing the IKE config
match first and skipping some checks if that or the local peer identity
doesn't match.
Although being already logged on level 2, these messages are usually just
confusing if they pop up randomly in the log when e.g. querying the configs
or installing traps. So after this the log messages will only be logged when
actually proposing or selecting traffic selectors during IKE.
This way we don't rely on the order of equally matching configs as
heavily anymore (which is actually tricky in vici) and this also doesn't
require repeating weak algorithms in all configs that might potentially be
selected if there are some clients that require them.
There is currently no ordering, so an explicitly configured exactly matching
proposal isn't a better match than e.g. the default proposal that also
contains the proposed algorithms.
In some scenarios we might find multiple usable peer configs with different
IKE proposals. This is a problem if we use a config with non-matching
proposals that later causes IKE rekeying to fail. It might even be a problem
already when creating the CHILD_SA if the proposals of IKE and CHILD_SA
are consistent.
There was a potential chance for a race condition if the ensured section
was purged for some reason before using it later.
This also changes the behavior for NULL/empty strings via load_string*
with merge == FALSE, which now purges the config/section.
Similar to the `also` keyword in ipsec.conf, the new syntax allows adding
one or more references to other sections, which means all the settings and
subsections defined there are inherited (values may be overridden, even
with an empty value to clear it).
It's important to note that all subsections are inherited, so if this is
used to reference a connection in swanctl.conf all auth rounds and
children are inherited. There is currently no syntax to limit the
inclusion level or clear inherited sections (but as mentioned, settings
in those inherited sections may be overridden).
Another property is that inherited settings or sections always follow
explicitly defined entries in the current section when they are enumerated.
This is relevant if the order is important (e.g. for auth rounds if `round`
is not specified).
References are evaluated dynamically at runtime, so referring to
sections later in the config file or included via other files is no
problem.
The colon used as separator to reference other sections may be used in
section names by writing :: (e.g. for Windows log file paths).
This is based on a patch originally written in 2016.
This way we get early log messages during plugin loading (including
integrity check results).
Instead of the fallback we could also remove the `customlog` namespace,
which was added to avoid conflicts with other settings/sections.
This was quite confusing previously: While calling insert_before()
and then remove_at() properly replaced the current item, calling them the
other way around inserted the new item before the previous item because
remove_at() changed the enumerator's position to the previous item.
The behavior in corner cases (calling the methods before or after
enumeration) is also changed slightly.
This allows switching to probing mode if the client is on a public IP
and this is the active task and connectivity gets restored. We only add
NAT-D payloads if we are currently behind a NAT (to detect changed NAT
mappings), a MOBIKE update that might follow will add them in case we
move behind a NAT.
Allow charon to start as a non-root user without CAP_CHOWN and still be
able to change the group on files that need to be accessed by charon
after capabilities have been dropped. This requires the user charon starts
as to have access to socket/pidfile directory as well as belong to the
group that charon will run as after dropping capabilities.
Closesstrongswan/strongswan#105.
Stater will lose update/reload commands when there is a second signal
coming in when the previous is still processed. This can happen more
easily with big configurations.
Closesstrongswan/strongswan#101.
In case the PRF's set_key() or allocate_bytes() method failed, skeyseed
was not initialized and the chunk_clear() call later caused a crash.
This could have happened with OpenSSL in FIPS mode when MD5 was
negotiated (and test vectors were not checked, in which case the PRF
couldn't be instantiated as the test vectors would have failed).
MD5 is not included in the default proposal anymore since 5.6.1, so
with recent versions this could only happen with configs that are not
valid in FIPS mode anyway.
Fixes: CVE-2018-10811
The keylength fix for ChaCha20Poly1305 (5a7b0be2) removes the keylength
attribute from the AEAD transform. This breaks compatibility between
versions with the patch and those without. The ChaCha20Poly1305 AEAD
won't match in proposals between such versions, and if no other algorithm
is available, negotiating SAs fails.
As a migration strategy, this patch introduces a new string identifier for a
ChaCha20Poly1305 proposal keyword which uses the explicit keylength, exactly
as it was used before the mentioned patch. Administrators that care about
the use of that AEAD with old clients can temporarily add this keyword to
the list of proposals, until all clients have been upgraded.
The used approach is the least invasive, as it just adds an additional
keyword that can't do any harm if not explicitly configured. Nontheless
allows it the administrator to smoothly keep ChaCha20Poly1305 working,
even if upgrading all peers simultaneously is not an option. It requires
manual configuration edits, though, but we assume that ChaCha20Poly1305
is not that widely used, and not as the only transform in proposals.
Removing the compat keyword in a future version is an option; it might
be helpful for other implementations, though, that falsely use an
explicit key length in ChaCha20Poly1305 AEAD transforms.
We now check if there are other routes tracked for the same destination
and replace the installed route instead of just removing it. Same during
installation, where we previously didn't replace existing routes due to
NLM_F_EXCL. Routes with virtual IPs as source address are preferred over
routes without.
This should allow using trap policies with virtual IPs on Linux.
Fixes#85, #2162.
If the certificate is revoked, we immediately returned and the chain was
invalid, however, if we couldn't fetch the CRL that result was not stored
for intermediate CAs and we weren't able to enforce a strict CRL policy
later.
Using such CRLs can be a problem if the clock on the host doing the
revocation check is trailing behind that of the host issuing CRLs in
scenarios where expired certificates are removed from CRLs. As revoked
certificates that expired will then not be part of new CRLs a host with
trailing clock might still accept such a certificate if it is still
valid according to its system clock but is not contained anymore in the
not yet valid CRL.
According to RFC 4945, section 5.1.3.2, a certificate for IKE must
either not contain the keyUsage extension, or, if it does, have at least
one of the digitalSignature or nonReputiation bits set.
The client identifier serves as unique identifier just like a unique MAC
address would, so even with identity_leases disabled some DHCP servers
might assign unique leases per identity.
This increases the chances that subject DNs that might have been cut
off with the arbitrary previous limit of 64 bytes might now be sent
successfully.
The REQUEST message has the most static overhead in terms of other
options (17 bytes) as compared to DISCOVER (5) and RELEASE (7).
Added to that are 3 bytes for the DHCP message type, which means we have
288 bytes left for the two options based on the client identity (host
name and client identification). Since both contain the same value, a
FQDN identity, which causes a host name option to get added, may be
142 bytes long, other identities like subject DNs may be 255 bytes
long (the maximum for a DHCP option).
According to RFC 2131, the minimum size of the 'options' field is 312
bytes, including the 4 byte magic cookie. There also does not seem to
be any restriction regarding the message length, previously the length
was rounded to a multiple of 64 bytes. The latter might have been
because in BOOTP the options field (or rather vendor-specific area as it
was called back then) had a fixed length of 64 bytes (so max(optlen+4, 64)
might actually have been what was intended), but for DHCP the field is
explicitly variable length, so I don't think it's necessary to pad it.
Since we won't read from the socket reducing the receive buffer saves
some memory and it should also minimize the impact on other processes that
bind the same port (Linux distributes packets to the sockets round-robin).
DHCP servers will respond to port 67 if giaddr is non-zero, which we set
if we are not broadcasting. While such messages are received fine via
RAW socket the kernel will respond with an ICMP port unreachable if no
socket is bound to that port. Instead of opening a dummy socket on port
67 just to avoid the ICMPs we can also just operate with a single
socket, bind it to port 67 and send our requests from that port.
Since SO_REUSEADDR behaves on Linux like SO_REUSEPORT does on other
systems we can bind that port even if a DHCP server is running on the
same host as the daemon (this might have to be adapted to make this work
on other systems, but due to the raw socket the plugin is not that portable
anyway).
The previous code compared the port in the packet to the client port and, if
successful, checked it also against the server port, which, therefore, never
matched, but due to incorrect offsets did skip the BPF_JA. If the client port
didn't match the code also skipped to the instruction after the BPF_JA.
However, the latter was incorrect also and processing would have continued at
the next instruction anyway. Basically, DHCP packets to any port were accepted.
What's not fixed with this is that the kernel returns an ICMP Port
unreachable for packets sent to the server port (67) because we don't
have a socket bound to it.
Fixes: f0212e8837 ("Accept DHCP replies on bootps port, as we act as a relay agent if server address configured")
This patch allows for giving strongSwan only the runtime capabilities it
needs, rather than full root privileges.
Adds preprocessor directives which allow strongSwan to be configured to
1) start up as a non-root user
2) avoid modprobe()'ing IPsec kernel modules into the kernel, which
would normally require root or CAP_SYS_MODULE
Additionally, some small mods to charon/libstrongswan ensure that charon
fully supports starting as a non-root user.
Tested with strongSwan 5.5.3.
We don't have MOBIKE and the fallback to reauthentication does also not
make much sense as that doesn't affect the CHILD_SAs for IKEv1. So
instead of complicating the code we just ignore roam events for IKEv1
for now.
Closesstrongswan/strongswan#100.
In very early versions routed CHILD_SAs were attached to IKE_SAs, since
that's not the case anymore (they are handled via trap manager), we can
remove this special handling.
This algorithm uses a fixed-length key and we MUST NOT send a key length
attribute when proposing such algorithms.
While we could accept transforms with key length this would only work as
responder, as original initiator it wouldn't because we won't know if a
peer requires the key length. And as exchange initiator (e.g. for
rekeyings), while being original responder, we'd have to go to great
lengths to store the condition and modify the sent proposal to patch in
the key length. This doesn't seem worth it for only a partial fix.
This means, however, that ChaCha20/Poly1305 can't be used with previous
releases (5.3.3 an newer) that don't contain this fix.
Fixes#2614.
Fixes: 3232c0e64e ("Merge branch 'chapoly'")
Since these are installed overlapping (like during a rekeying) we have to use
the same (unique) marks (and possibly reqid) that were used previously,
otherwise, the policy installation will fail.
Fixes#2610.
If the responder is behind a NAT that remaps the response from the
statically forwarded port 500 to a new external port (as Azure seems to be
doing) we should still switch to port 4500 if we used port 500 so far as
it would not have been possible to send any messages to it if it wasn't
really port 500 (we only add a non-ESP marker if neither port is 500).
This helps to distinguish between port and protocol if only one of them
is set. If no protocol is set it's printed as 0, if the traffic
selector covers any port (0-65535) the slash that separates the two values
and the port is omitted.
If a Quick mode is initiated for a CHILD_SA that is already installed
we can identify this situation and rekey the already installed CHILD_SA.
Otherwise we end up with several CHILD_SAs in state INSTALLED which
means multiple calls of child_updown are done. Unfortunately,
the deduplication code later does not call child_updown() (so up and down
were not even).
Closesstrongswan/strongswan#95.
Until now the configuration available to user for HW offload were:
hw_offload = no
hw_offload = yes
With this commit users will be able to configure auto mode using:
hw_offload = auto
Signed-off-by: Adi Nissim <adin@mellanox.com>
Reviewed-by: Aviv Heller <avivh@mellanox.com>
Until now there were 2 hw_offload modes: no/yes
* hw_offload = no : Configure the SA without HW offload.
* hw_offload = yes : Configure the SA with HW offload.
In this case, if the device does not support
offloading, SA creation will fail.
This commit introduces a new mode: hw_offload = auto
----------------------------------------------------
If the device and kernel support HW offload, configure
the SA with HW offload, but do not fail SA creation otherwise.
Signed-off-by: Adi Nissim <adin@mellanox.com>
Reviewed-by: Aviv Heller <avivh@mellanox.com>
If a daemon PID file references the process that does the check, it is safe
to ignore it; no running process can have the same PID. While this is rather
unlikely to get restarted with the same PID under normal conditions, it is
quite common when running inside PID namespaced containers: If a container
gets stopped and restarted with a PID file remaining, it is very likely that
the PID namespace assigns the same PID to our service, as they are assigned
sequentially starting from 1.
According to RFC 5114 the exponent length for these groups should always equal
the size of their prime order subgroup.
This was handled correctly before the initialization was done during
library initialization.
Fixes: 46184b07c1 ("diffie-hellman: Explicitly initialize DH exponent sizes during initialization")
The previous code was obviously incorrect and caused strange side effects
depending on the compiler and its optimization flags (infinite looping seen
with GCC 4.8.4, segfault when destroying the private key in build() seen
with clang 4.0.0 on FreeBSD).
Fixes#2579.
We can't use ASN1_DEF, which would technically be more correct, as the
ASN.1 parser currently can't handle that. For algorithm identifiers we
often use ASN1_EOC as type (with ASN1_RAW), however, that doesn't work with
ASN1_DEF because the element is assumed missing if the type doesn't match.
On the other hand, we can't set the type to ASN1_SEQUENCE because then the
parser skips the following rule if the element is missing (it does so for
all constructed types, but I guess is mainly intended for context tags),
which in this case overruns the parser rules array.
This way there will be a mismatch if one of the proposals contains
transform types not contained in the other (the fix list of transform
types used previously resulted in a match if unknown transform types
were contained in one of the proposals). Merging the sets of types
makes comparing proposals with optional transform types easier (e.g.
DH for ESP with MODP_NONE).
If we receive an INVALID_KE_PAYLOAD notify we should not just retry
with the requested DH group without checking first if we actually propose
the group (or any at all).
After a rekeying we keep the inbound SA and policies installed for a
while, but the outbound SA and policies are already removed. Attempting
to update them could get the refcount in the kernel interface out of sync
as the additional policy won't be removed when the CHILD_SA object is
eventually destroyed.
When initiating a trap policy we explicitly pass the reqid along. I guess
the lookup was useful to get the same reqid if a trapped CHILD_SA is manually
initiated. However, we now get the same reqid anyway if there is no
narrowing. And if the traffic selectors do get narrowed the reqid will be
different but that shouldn't be a problem as that doesn't cause an issue with
any temporary SAs in the kernel (this is why we pass the reqid to the
triggered CHILD_SA, otherwise, no new acquire would get triggered for
traffic that doesn't match the wider trap policy).
Reqids for the same traffic selectors are now stable so we don't have to
pass reqids of previously installed CHILD_SAs. Likewise, we don't need
to know the reqid of the newly installed trap policy as we now uninstall
by name.
The timed wait functions tested in the threading unit tests often but
randomly trigger a bit early on AppVeyor Windows containers. We allow this
if it is not earlier than 5ms.
g_variant_builder_add() creates a new GVariant using g_variant_new() and
then adds it to the builder. Passing a GVariant probably adds the
pointer to the array, not the value. I think an alternative fix would
be to use "@u" as type string for the g_variant_builder_add() call, then
the already allocated GVariant is adopted.
Fixes: 9a71b7219c ("charon-nm: Port to libnm")
With IKEv1 we transmit both public DH factors (used to derive the initial
IV) besides the shared secret. So these messages could get significantly
larger than 1024 bytes, depending on the DH group (modp2048 just about
fits into it). The new default of 2048 bytes should be fine up to modp4096
and for larger groups the buffer size may be increased (an error is
logged should this happen).
This is really only needed for other exchanges like DPDs not when we
just updated the addresses. The NAT-D payloads are only used here to
detect whether UDP encapsulation has to be enabled/disabled.
If we have to remove and reinstall SAs for address updates (as with the
Linux kernel) there is a short time where there is no SA installed. If
we keep the policies installed they (or any traps) might cause acquires
and temporary kernel states that could prevent the updated SA from
getting installed again.
This replaces the previous workaround to avoid plaintext traffic leaks
during policy updates, which used low-priority drop policies.
This can be useful if routing rules (instead of e.g. route metrics) are used
to switch from one to another interface (i.e. from one to another
routing table). Since we currently don't evaluate routing rules when
doing the route lookup this is only useful if the kernel-based route
lookup is used.
Resolvesstrongswan/strongswan#88.
The counter does not tell us what task is actually queued, so we might
ignore the response to an update (with NAT-D payloads) if only an address
update is queued.
Instead of destroying the new task and keeping the existing one we
update any already queued task, so we don't loose any work (e.g. if a
DPD task is active and address update is queued and we'd actually like
to queue a roam task).
According to RFC 3748 MSKs must be at least 64 bytes, however, that's
not the case for the MSK derived via EAP-MSCHAPv2. The two key parts
received are only 16 bytes each (derived according to RFC 3079,
section 3.3), so we end up with an MSK of only 32 bytes. The eap-mschapv2
plugin, on the other hand, pads these two parts with 32 zeros.
Interestingly, this is not a problem in many cases as the SHA1/2 based
PRFs used later use a block size that's >= 64 bytes, so the shorter MSK
is just padded with zeros then. However, with AES-XCBC-PRF-128, for
instance, which uses a block size of 16 bytes, the different MSKs are an
issue as XCBC is applied to both to shorten them, with different results.
This eventually causes the authentication to fail if the client uses a
zero-padded MSK produced by the eap-mschapv2 plugin and the server the 32
byte MSK received via RADIUS.
This is currently not an issue for CHILD_SA rekeying tests as these only
check rekeyings of the CHILD_SA created with the IKE_SA, i.e. there is
no previous DH group to reuse.
For the CHILD_SA created with the IKE_SA the group won't be set in the
proposal, so we will use the first one configure just as if the SA was
created new with a CREATE_CHILD_SA exchange. I guess we could
theoretically try to use the DH group negotiated for IKE but then this
would get a lot more complicated as we'd have to check if that group is
actually contained in any of the CHILD_SA's configured proposals.
This way we get proper error handling if the DH group the peer requested
is not actually supported for some reason (otherwise we'd just retry to
initiate with the configured group and get back another notify).
This fixes two issues, one is a bug if a DH group is configured for the
local ESP proposals and charon.prefer_configured_proposals is disabled.
This would cause the DH groups to get stripped not from the configured but
from the supplied proposal, which usually already has them stripped. So
the proposals wouldn't match. We'd have to always strip them from the local
proposal. Since there are apparently implementations that, incorrectly, don't
remove the DH groups in the IKE_AUTH exchange (e.g. WatchGuard XTM25
appliances) we just strip them from both proposals. It's a bit more lenient
that way and we don't have to complicate the code to only clone and strip the
local proposal, which would depend on a flag.
References #2503.
IKE_SAs newly created via HA_IKE_ADD message don't have any IKE or peer
config assigned yet (this happens later with an HA_IKE_UPDATE message).
And because the state is initially set to IKE_CONNECTING the roam() method
does not immediately return, as it later would for passive HA SAs. This
might cause the check for explicitly configured local addresses to crash
the daemon with a segmentation fault.
Fixes#2500.
In scenarios where the server accepts client certificates from dozens or
even hundreds of CAs it might be necessary to omit certificate request
payloads from the IKE_SA_INIT response to avoid fragmentation.
As it is rarely the case in road-warrior scenarios that the server
already has the client certificate installed it should not be a problem
to always send it.
Depending on the plugins that eventually parse the certificate and CRL,
serials with MSB set (i.e. negative numbers that have a zero byte prefixed
when encoded as ASN.1 INTEGER) might have (x509 plugin) or not have
(openssl plugin) a zero byte prefix when returned by get_serial() or
enumerated from the CRL. Strip them before doing the comparison or
revocation checking might fail if not both credentials are parsed by the
same plugin (which should be rare and only happen if parsing of either
cert or CRL fails with one of the plugins and there is a fallback to the
implementation provided by the other plugin).
Fixes#2509.
Reset errno to 0 before calling strtoul() since it sets errno only on
error cases. So the following test fails even on correct conversions if
errno had a value != 0.
Fixes#2506.
Otherwise, the remote identity is ignored when matching owner identities
of PSKs and this way matching PSKs that explicitly have %any assigned is
improved.
Fixes#2497.
If users want to associate secrets with any identity, let 'em. This is
also possible with vici and might help if e.g. the remote identity is
actually %any as that would match a PSK with local IP and %any better
than one with local and different remote IP.
Fixes#2497.
Use argument evaluation provided by settings_t instead of using strings
to enumerate key/values.
If section names contain dots the latter causes the names to get split
and interpreted as non-existing sections and subsections.
This currently doesn't work for connections and their subsections due to
the recursion.
libnm-glib is deprecated for several years and reaching the end of its
life. Let's switch to the more up-to-date library.
Closesstrongswan/strongswan#85.
Support for mark=%unique/%unique-dir is implemented by using designated
magic mark values.
Use of masks is orthogonal to the 'unique' feature, as it is useful to be
able to designate portions of the packet mark for other purposes, while
still using different marks for different connections.
When these magic values are masked, their magic meaning is lost.
Perform masking only on explicit mark values.
Closesstrongswan/strongswan#87.
Tobias Brunner