Project

General

Profile

Feature #2497

Slightly improve IKEv1 PSK lookup with rightid=%any (e.g. connection1: leftid=left=<IP1> rightid=right=%any connection2: leftid=left=<IP1> rightid=right=<IP2>)

Added by Andreas Weigel almost 3 years ago. Updated almost 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Tobias Brunner
Category:
ikev1
Target version:
5.6.2
Start date:
Due date:
Estimated time:
Resolution:
Fixed

Description

Hello everyone,

Again I'm messing with the lookup of PSKs (see #2223).

Affected Versions:

Linux strongSwan U5.3.5/K4.4.104-yocto-custom
Linux strongSwan U5.6.1/K4.9.61-yocto-custom

Note that the patch chaning the PSK-lookup order to first use identities (introduced in 5.5.2) from a matched ike-config -- while it does change the behavior -- does not change the outcome for the described scenario.
Note also that I'm aware that using IKEv2 probably solves those problems and that there are much better and secure ways to configure the given scenario.

Scenario

  • Configuration with multiple IKEv1 (PSK) connections that share a common IP address X as the left=leftid parameter
  • One connection uses right=rightid=%any, the others use distinct IP addresses

Result of connection attempt from remote peers

All connections with explicitly configured IP addresses work perfectly well. For those that use %any as right|rightid, a wrong PSK is selected. The selection of a PSK in this case is more or less random, depending on the order within the configuration file. What happens is:

  1. A connection with a explicit IP address is checked
  2. the local ID produces a perfect match, the remote ID is either
    • "nil" (strongswan > 5.6, with commit e92d8a56b376e6964e5f1b53b2d261f167e0b166 from #2223); in this case, no match for the remote ID is performed and checks on the correct shared secret do not produce a better match and is discarded
    • "<remote IP>" (e.g. strongswan 5.3.5), which, however, yields the same result, because no match against "%any" is performed, because no "%any"-ID is added to the list of owner for the shared secret credential_manager by the stroke plugin for the above config

IMHO, this can be considered a bug, because charon actually does have enough information to unambiguously lookup the correct PSK and actually pluto was able to do so before it went into well-deserved retirement.

Possible Solution

I implemented some small but possibly far-reaching changes into the PSK lookup order and the way the stroke plugin parses ipsec.secrets (see the attached patch against 5.6.2dr3). While I think that the applied logic is sound, it'd be a lie to claim that I checked all scenarios that are possibly affected by the change and would be thankful for some input in this regard. However, I did run the (whole) strongswan test suite with the patched version of strongswan:


Passed : 393
Failed : 8

The 8 failures are exclusively in the TNC section and also fail for the unpatched strongswan (5.6.3dr3), thus I'm quite sure they do not have to do anything with the suggested patch.

To make sure the correct version is deployed on all hosts:

for pair in "alice 10.1.0.10" "venus 10.1.0.20" "moon 10.1.0.1" "bob 10.2.0.10" "carol 192.168.0.100" "winnetou 192.168.0.150" "dave 192.168.0.200"; do host=($pair); printf "${host[0]}: "; ssh -c arcfour -o StrictHostKeyChecking=no root@${host[1]} "ipsec --version" 2>/dev/null | head -n 1; done

alice: Linux strongSwan U5.6.2dr3/K4.9.68(psk_lookup_patch)
venus: Linux strongSwan U5.6.2dr3/K4.9.68(psk_lookup_patch)
moon: Linux strongSwan U5.6.2dr3/K4.9.68(psk_lookup_patch)
bob: Linux strongSwan U5.6.2dr3/K4.9.68(psk_lookup_patch)
carol: Linux strongSwan U5.6.2dr3/K4.9.68(psk_lookup_patch)
winnetou: Linux strongSwan U5.6.2dr3/K4.9.68(psk_lookup_patch)
dave: Linux strongSwan U5.6.2dr3/K4.9.68(psk_lookup_patch)

Conclusion

Please comment on the suggested changes and if your are willing to apply them upstream. I could also assemble a PR if that's preferred make changes to the structure of the patch. I guess I could also try to setup a test scenario for the given configuration (and probably the one mentioned in #2223).

change_psk_lookup_logic.patch (2.57 KB) change_psk_lookup_logic.patch Andreas Weigel, 13.12.2017 13:32
shared_filter_remove_redundancy.patch (1.7 KB) shared_filter_remove_redundancy.patch Andreas Weigel, 13.12.2017 13:33

Associated revisions

Revision a9f3016e (diff)
Added by Tobias Brunner almost 3 years ago

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.

Fixes #2497.

Revision 419ae9a2 (diff)
Added by Tobias Brunner almost 3 years ago

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.

Fixes #2497.

History

#1 Updated by Tobias Brunner almost 3 years ago

  • Status changed from New to Feedback

All connections with explicitly configured IP addresses work perfectly well. For those that use %any as right|rightid, a wrong PSK is selected. The selection of a PSK in this case is more or less random, depending on the order within the configuration file. What happens is:

  1. A connection with a explicit IP address is checked
  2. the local ID produces a perfect match, the remote ID is either

As pointed out numerous times, configuring the local IP/ID in ipsec.secrets is basically pointless as every secret will match that way. So if you'd just list only the remote IP/ID for secrets with specific IP/IDs you won't have a problem.

IMHO, this can be considered a bug, because charon actually does have enough information to unambiguously lookup the correct PSK and actually pluto was able to do so before it went into well-deserved retirement.

It only has the IPs available, so if that's what you consider enough, define your secrets and configs that way (just set the remote IP/ID, no %any and no local IP, and no IDs at all for the secret that should be used for the right=%any connection).

#2 Updated by Andreas Weigel almost 3 years ago

Thanks for the quick reply, Tobias.

So if you'd just list only the remote IP/ID for secrets with specific IP/IDs you won't have a problem.

While I'm quite upset that I was not able to figure out that solution for the given configuration, I do not completely buy your argument. Maybe this is a somewhat a contrived example, but add another site-to-site connection that has rightid=%any but is distinguished from the other rightid=%any connection by its leftid and your suggestion fails for one of the two connections in this case (although it could match the correct PSK)

As pointed out numerous times, configuring the local IP/ID in ipsec.secrets is basically pointless as every secret will match that way

But this is just because it's implemented this way. I'd still argue that the lookup logic for PSKs is rather contra-intuitive. Why have multiple selectors if they are not used? The proposed patch slightly changes the logic so that both selectors are considered when looking up the PSK.

PS: Please note that I wholeheartedly agree that people should rather use IKEv2 and more sensible authentication mechanisms. However, they just don't seem to care that much or are constrained by other people who don't care that much :-/

#3 Updated by Tobias Brunner almost 3 years ago

  • Tracker changed from Issue to Feature
  • Subject changed from IKEv1 PSK Lookup failure (connection1: leftid=left=<IP1> rightid=right=%any connection2: leftid=left=<IP1> rightid=right=<IP2>) to Slightly improve IKEv1 PSK lookup with rightid=%any (e.g. connection1: leftid=left=<IP1> rightid=right=%any connection2: leftid=left=<IP1> rightid=right=<IP2>)
  • Target version set to 5.6.2

Maybe this is a somewhat a contrived example, but add another site-to-site connection that has rightid=%any but is distinguished from the other rightid=%any connection by its leftid and your suggestion fails for one of the two connections in this case (although it could match the correct PSK)

That only works if you also use different local IPs (otherwise both connections match equally well and there is no telling which connection and hence which IDs are used to look up the PSK). And if that's the case you could probably associate the PSK with that second IP if that's only used for that connection (otherwise you'd presumably have the same issue you reported). So yes, it's definitely not ideal for that specific setup.

As pointed out numerous times, configuring the local IP/ID in ipsec.secrets is basically pointless as every secret will match that way

But this is just because it's implemented this way. I'd still argue that the lookup logic for PSKs is rather contra-intuitive. Why have multiple selectors if they are not used?

They are used, but the situation with IKEv1 is a bit special. In all other scenarios we have two identities for the PSK lookup. Even if none are configured (or e.g. the initiator does not send an IDr payload) they will be set to either the IP or %any.

Another difference for IKEv1 is that we don't use the actual identities but the configured ones. These could contain wildcards (e.g. *.strongswan.org), which is relevant because the order when calling identification_t::matches() matters.

For instance, identities of any type match %any with ID_MATCH_ANY (id->matches(id, any)), however, %any only matches %any (i.e. any->matches(any, id) only succeeds if id is %any). Same with wildcards, while vpn1.strongswan.org matches *.strongswan.org there is no match the other way around. Because the passed identities for the PSK lookup are matched against the configured owners (id->matches(id, owner)), which are expected to contain wildcards or to be subnets/ranges etc. (or even be %any), the matching could be a bit odd.

So simply setting the remote ID to %any doesn't have that much of an effect as it won't match anything but %any. Unles, as your patch did, we allow to explicitly configure %any as owner identity for PSKs via stroke (it's already possible via vici). Then the match for such secrets will be slightly better than for secrets where only the local ID matches. The easiest way to do that is to just not ignore identities of type ID_ANY. And instead of allocating and assigning %any in every credential set during the lookup I propose we set it in phase1.c before we do the lookup. I did this in the 2497-ikev1-psk-lookup branch.

With these changes the matches look as follows:

Secret <local> %any <local> <remote> <local> <other>
: "PSK" (3) ANY + ANY (6) ANY + ANY (3) ANY + ANY
<local> : "PSK" (2) PERFECT + NONE (5) PERFECT + NONE (2) PERFECT + NONE
<local> %any : "PSK" (1) PERFECT + ANY (4) PERFECT + ANY (1) PERFECT + ANY
<remote> : "PSK" (-) NONE + NONE (3) NONE + PERFECT (-) NONE + NONE
<remote> %any : "PSK" (3) ANY + ANY (2) ANY + PERFECT (3) ANY + ANY
<local> <anything> : "PSK" (2) PERFECT + NONE (5) PERFECT + NONE (2) PERFECT + NONE
<remote> <anything> : "PSK" (-) NONE + NONE (3) NONE + PERFECT (-) NONE + NONE
<anything> : "PSK" (-) NONE + NONE (-) NONE + NONE (-) NONE + NONE
<anything> %any : "PSK" (3) ANY + ANY (6) ANY + ANY (3) ANY + ANY
<local> <remote> : "PSK" (2) PERFECT + NONE (1) PERFECT + PERFECT (2) PERFECT + NONE

Or put differently:

<local> %any <local> <remote> <local> <other>
<local> %any : "PSK" <local> <remote> : "PSK" <local> %any : "PSK"
<local> : "PSK"
<local> <anything> : "PSK"
<local> <remote> : "PSK"		 <remote> %any : "PSK" <local> : "PSK"
<local> <anything> : "PSK"
<local> <remote> : "PSK"
<remote> : "PSK"
<remote> <anything> : "PSK"
<local> %any : "PSK"
: "PSK"
<remote> %any : "PSK"
<anything> %any : "PSK"		 <local> : "PSK"
<local> <anything> : "PSK"		 : "PSK"
<remote> %any : "PSK"
<anything> %any : "PSK"
: "PSK"
<anything> %any : "PSK"

If they match equally well, the order in which they are defined/loaded matters.

#4 Updated by Andreas Weigel almost 3 years ago

Hi Tobias,

I've tested your changes as a backport against 5.3.5 and against the current master and successfully got correct matches for all mentioned setups.

Thanks a lot for digging into the issue and providing a more elegant solution!

#5 Updated by Tobias Brunner almost 3 years ago

  • Status changed from Feedback to Closed
  • Assignee set to Tobias Brunner
  • Resolution set to Fixed

Thanks for testing. Applied to master.

Also available in: Atom PDF