Tobias Brunner [Wed, 21 Feb 2018 10:04:45 +0000 (11:04 +0100)]
child-sa: Don't update outbound policies if they are not installed
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.
Tobias Brunner [Thu, 22 Feb 2018 10:31:14 +0000 (11:31 +0100)]
Merge branch 'trap-manager-uninstall'
This changes how trap policies are deleted in order to avoid conflicts if a
trap policy with changed peer config is concurrently removed and reinstalled
under a different name (the reqid will be the same, so the wrong policy
could have been deleted by the old code).
Tobias Brunner [Fri, 3 Nov 2017 10:49:45 +0000 (11:49 +0100)]
child-sa: No need to find reqid of existing trap policy
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).
Tobias Brunner [Fri, 3 Nov 2017 10:32:04 +0000 (11:32 +0100)]
trap-manager: Remove reqid parameter from install() and change return type
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.
Tobias Brunner [Fri, 16 Feb 2018 10:55:54 +0000 (11:55 +0100)]
appveyor: Allow events to trigger early in threading unit tests
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.
Tobias Brunner [Wed, 21 Feb 2018 10:53:55 +0000 (11:53 +0100)]
charon-nm: Fix building list of DNS/MDNS servers with libnm
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.
Tobias Brunner [Wed, 14 Feb 2018 13:51:24 +0000 (14:51 +0100)]
ha: Double receive buffer size for HA messages and make it configurable
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).
Liu Qun (liuqun) [Mon, 12 Feb 2018 03:39:00 +0000 (11:39 +0800)]
README: Fix paths to private keys
Since version 5.5.1, different keys can be put together in
/etc/swanctl/private.
See:
* tobiasbrunner@7caba2eb5524be6b51943bcc3d2cb0e4c5ecc09a
swanctl: Add 'private' directory/section to load any type of private key
Signed-off-by: Liu Qun (liuqun) <qunliu@zyhx-group.com>
Tobias Brunner [Fri, 9 Feb 2018 14:54:36 +0000 (15:54 +0100)]
Merge branch 'mobike-nat'
These changes improve MOBIKE task queuing. In particular we don't
want to ignore the response to an update (with NAT-D payloads) if only
an address list update or DPD is queued as that could prevent use from
updating the UDP encapsulation in the kernel.
A new optional roam trigger is added to the kernel-netlink plugin based
on routing rule changes. This only works properly, though, if the kernel
based route lookup is used as the kernel-netlink plugin does currently
not consider routing rules for its own route lookup.
Another change prevents acquires during address updates if we have to
update IPsec SAs by deleting and readding them. Because the outbound policy
is still installed an acquire and temporary SA might get triggered in
the short time no IPsec SA is installed, which could subsequently prevent the
reinstallation of the SA. To this end we install drop policies before
updating the policies and SAs. These also replace the fallback drop policies
we previously used to prevent plaintext leaks during policy updates (which
reduces the overhead in cases where addresses never or rarely change as
additional policies will only have to be tracked during address updates).
Tobias Brunner [Fri, 9 Feb 2018 07:48:07 +0000 (08:48 +0100)]
ike-mobike: Don't trigger update for NAT mapping change detected during an address update
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.
Tobias Brunner [Tue, 6 Feb 2018 17:07:34 +0000 (18:07 +0100)]
child-sa: Install drop policies while updating IPsec SAs and policies
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.
Tobias Brunner [Mon, 29 Jan 2018 14:26:17 +0000 (15:26 +0100)]
kernel-netlink: Optionally trigger roam events on routing rule changes
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.
Tobias Brunner [Mon, 29 Jan 2018 11:34:33 +0000 (12:34 +0100)]
ike-mobike: Only ignore MOBIKE responses if an actual update is queued
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.
Tobias Brunner [Mon, 29 Jan 2018 10:49:50 +0000 (11:49 +0100)]
ikev2: Update currently queued MOBIKE task
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).
Tobias Brunner [Fri, 26 Jan 2018 08:51:07 +0000 (09:51 +0100)]
libradius: Pad received MSK to at least 64 bytes
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.
Tobias Brunner [Fri, 9 Feb 2018 09:28:44 +0000 (10:28 +0100)]
Merge branch 'dh-group-rekey'
These changes improve rekeying after the peer initially selected a
different DH group than we proposed. Instead of using the configured DH
group again, and causing another INVALID_KE_PAYLOAD notify, we now reuse
the previously negotiated group. We also send the selected DH group
first in the proposals (and move proposals that don't contain the group
to the back) so that implementations that select the proposal first and
without consulting the KE payload (e.g. strongSwan when preferring the
client's proposals) will see the preferred group first.
Tobias Brunner [Fri, 2 Feb 2018 09:59:25 +0000 (10:59 +0100)]
ike-init: Make DH group reuse optional to test INVALID_KE_PAYLOAD handling
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.
Tobias Brunner [Fri, 2 Feb 2018 09:48:21 +0000 (10:48 +0100)]
child-rekey: Use previously negotiated DH group when rekeying CHILD_SAs
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.
Tobias Brunner [Fri, 2 Feb 2018 09:26:36 +0000 (10:26 +0100)]
ike-init: Move creation of DH instance after INVALID_KE_PAYLOAD to build_i()
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).
Tobias Brunner [Mon, 22 Jan 2018 13:33:40 +0000 (14:33 +0100)]
child-cfg: Strip DH groups from both compared proposals
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.
Tobias Brunner [Wed, 20 Dec 2017 11:32:52 +0000 (12:32 +0100)]
ike: Don't handle roam events if no IKE config is available
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.
Tobias Brunner [Thu, 8 Feb 2018 11:15:36 +0000 (12:15 +0100)]
android: Always send the client certificate
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.
Tobias Brunner [Thu, 1 Feb 2018 16:10:19 +0000 (17:10 +0100)]
testing: Ignore IP-in-IP SAs created with IPComp SAs that remain in the kernel
The kernel creates such SAs to handle uncompressed small packets. They
are implicitly created and deleted with IPComp SAs. The problem is that
when we delete an IPComp SA only that state is deleted and removed from
the SA lists immediately, the IP-in-IP state is not removed until the IPComp
state is eventually destroyed. This could take a while if there are still
references to it around. So the IP-in-IP states will keep getting reported
by ip xfrm state until that happens (we also can't flush or explicitly delete
such kernel-created states).
In kernels before 4.14 this wasn't really a problem but since ec30d78c14a8 ("xfrm: add xdst pcpu cache") the kernel seems to keep the
references to the last used SAs around a lot longer.
Also, usually a test scenario following an IPComp scenario will create
and use new SAs and thus the cached SAs will disappear before the kernel
state is checked again. However, if a following scenario uses different
hosts the states might remain, which caused some unrelated scenarios to
fail before adding this fix.
Tobias Brunner [Wed, 24 Jan 2018 13:42:28 +0000 (14:42 +0100)]
revocation: Skip any zero bytes when comparing serials in CRLs
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).
Reinhard Pfau [Tue, 23 Jan 2018 09:09:14 +0000 (10:09 +0100)]
eap: Reset errno before calling strtoul() to parse EAP type
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.
Tobias Brunner [Wed, 20 Dec 2017 09:28:31 +0000 (10:28 +0100)]
ikev1: Default remote identity to %any for PSK lookup if not configured
Otherwise, the remote identity is ignored when matching owner identities
of PSKs and this way matching PSKs that explicitly have %any assigned is
improved.
Tobias Brunner [Wed, 20 Dec 2017 09:13:39 +0000 (10:13 +0100)]
stroke: Don't ignore %any as owner of shared secrets
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.
Robin McCorkell [Mon, 27 Nov 2017 16:46:22 +0000 (16:46 +0000)]
testing: Override user environment PATH in chroot
chroot will capture the user environment's PATH variable, which may be
wrong (e.g. not include /bin:/sbin, as it is on Arch). We should set a
known-working PATH variable in the chroot.
Tobias Brunner [Fri, 8 Dec 2017 08:59:13 +0000 (09:59 +0100)]
travis: Disable warning that causes a false positive in Xcode 8.3+
Xcode 8.3, to which there recently was a switch, spits out a warning for
the potentially unaligned access to ip6_plen in ip-packet.c, which we
explicitly read via untoh16() hence the access to that pointer is not
actually unaligned. It seems the compiler is not able to determine that
there is no unaligned access even though the function is defined in the
header and marked inline.
Eyal Birger [Thu, 30 Nov 2017 08:47:01 +0000 (10:47 +0200)]
ipsec-types: Don't mask the mark value if it is one of the 'unique' values
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.