Subject: [IPsec] Comments for draft-ietf-ipsecme-labeled-ipsec-04

From: "Valery Smyslov" <[email-masked]>
Date: Wed, 20 Jan 2021 16:09:49 +0300

Hi,

after reading draft-ietf-ipsecme-labeled-ipsec-04 I have a couple of comments.

First, it's not clear for me why zero length TS_SECLABLE is prohibited.
The draft currently says

   A zero length Security Label MUST NOT be used.  If a received TS
   payload contains a TS_TYPE of TS_SECLABEL with a zero length Security
   Label, that specific Traffic Selector MUST be ignored.

I wonder what's rational behind this restriction. From my point of view 
zero length TS_SECLABLE can be used to express that using Security Labels
is optional. I.e. initiator can include zero length TS_SECLABEL Traffic Selector
along with other  TS_SECLABEL Traffic Selectors to indicate that responder
is free to not use security labels for the SA. Currently draft suggests 
the initiator to perform several attempts to establish IPsec SA 
with and without TS_SECLABEL Traffic Selectors in such situation,
which is more complicated and less efficient. Am I missing something?

Another my concern is that draft allows security labels to differ 
in TSi and TSr without giving any possible semantic interpretation for this.
This looks weird. I think that at least some possible interpretation 
of such situation must be given, e.g. TS_SECLABEL in TSi is used 
for traffic from initiator to responder and TS_SECLABEL in TSr 
is used for the return traffic. In this case it is clear that
they can differ in some situations.

And the last (but not the least). It seems to me that Section 3
(which aims to update Section 2.9 of RFC 7296) oversimplifies
the negotiation process. In particular, the sentence

   A responder MUST create its TS response by selecting one of each
   TS_TYPE present in the offered TS by the initiator.

is inaccurate in my opinion, since it implies (as I read it) that responder can pick
exactly one traffic selector of each TS_TYPE and can only return it unmodified. 
If fact, with regard to TS_IPV4_ADDR_RANGE and TS_IPV6_ADDR_RANGE, 
all the proposed traffic selectors are semantically combined and
represent the set of traffic the initiator believes is appropriate for the SA.
The responder then can select several traffic selectors from the proposed list and even modify 
some or all of them provided that the resulted set of traffic they represent 
is a subset of set of traffic proposed by the initiator. This logic is lost in the draft and since 
the draft intends to update RFC 7296 in this regard, readers may decide
that this logic is no more actual. In this Section 3 must be rewritten
to describe the changes more accurately.

Regards,
Valery.

---

From: Paul Wouters <[email-masked]>
Date: Wed, 20 Jan 2021 21:28:18 -0500 (EST)

On Wed, 20 Jan 2021, Valery Smyslov wrote:

> is free to not use security labels for the SA.

We did discuss this earier in the WG. There is no use case of "optional"
security labels. You either insist on these, or you don't use these.

Implementing "optional" ones by using a zero length label is asking for
implementation problems. Especially when people consider these labels
as strings and run strlen() on the empty (but NULL terminated string)
and interpret that as wildcard.

Since there is no use case, and significant implementation danger, the
document makes an explicit case to reject these. Our implementation
will return TS_UNAVAILABLE if it encounters any zero length TS_SECLABEL.

> Currently draft suggests

It was what the WG preferred to do. The reasoning was that labels are
surely very much preconfigured and connections will not be migrated
from "no security" to "security label" where the operator wants to
operate insecurely first. That is, opportunistic security labels is
not a thing.

The case you describe is very unlikely. Either the endpoints wants a
label or doesn't want one. optional security is not security.

> in TSi and TSr without giving any possible semantic interpretation for this.

Correct. But the label is clearly defines as opaque to the IKE layer.
The IKE layer should not be making any kindds of assumptions or
interpretations, but just relay this to the underlying label security
consumer (eg in our case the LSM of SElinux). Note that our
implementation only supports symmetrical labels so far, because
that is how SElinux labeling works. But the protocol can accept
other methods.

> they can differ in some situations.

I'm not sure why this is not clear? TSi and TSr entries apply to
one direction.  TS_SECLABEL does not change that.

> the negotiation process.

Again, we followed the consensus of the group here.

> In particular, the sentence
>

How about this clarification:

 	A responder MUST create its TS response by selecting at least
 	one of each TS Type.  One exception is that a responder MAY omit
 	a TS_IPV4_ADDR_RANGE or TS_IPV6_ADDR_RANGE as long as at least
 	one TS_IPV4_ADDR_RANGE or TS_IPV6_ADDR_RANGE is selected.
 	Furthermore, a narrowed TS_IPV4_ADDR_RANGE and TS_IPV6_ADDR_RANGE content
 	set MAY be selected.

Note that this is mentioned already in section 1.2:

    A Traffic Selector payload (TS) is a set of one or more Traffic
    Selectors of the same or different TS_TYPEs, but MUST include at
    least one TS_TYPE of TS_IPV4_ADDR_RANGE or TS_IPV6_ADDR_RANGE.

If you prefer other text, please provide your suggestion.

Paul

---

From: "Valery Smyslov" <[email-masked]>
Date: Thu, 21 Jan 2021 15:46:24 +0300

Hi Paul,

> security labels. You either insist on these, or you don't use these.

Well, it contradicts what draft says (Section 2.2):

   If the Security Label traffic selector is optional from a
   configuration point of view, the initiator will have to choose which
   TS payload to attempt first.  If it includes the Security Label and
   receives a TS_UNACCEPTABLE, it can attempt a new Child SA negotiation
   without that Security Label.

So you do allow security label to be optional, but negotiate its usage this 
with most complex and ineffective way. It's very strange.

> and interpret that as wildcard.

Well, people will consider meaning of the security labels in accordance 
to what is written in the draft. If we try to envision every possible mis-reading
of the documents we'd better not to start writing them...

And BTW zero length means that there are no any content, including NULL terminator :-)

> will return TS_UNAVAILABLE if it encounters any zero length TS_SECLABEL.

No, the document does allow optional security labels, see above.

> label or doesn't want one. optional security is not security.

Then why text in 2.2 allows the initiator to choose between using them 
and omitting them? Isn't it exactly what you just described as "not security"?

> that is how SElinux labeling works. But the protocol can accept

I still prefer _some_ words to be added about _possible_
semantics when seclabels are different in TSi and TSr.

> one direction.  TS_SECLABEL does not change that.

It's unclear what this case could mean:

         TSi = ((0,0,198.51.0-198.51.255),
                TS_SECLABEL1)

         TSr = (((0,0,203.0.113.0-203.0.113.255),
                TS_SECLABEL2)

It's true that IKE doesn't interpret seclabels, but it usually
does some sanity checks and as implementer I'm not sure whether
this is allowed (what does it mean?) or not.

>  	Furthermore, a narrowed TS_IPV4_ADDR_RANGE and TS_IPV6_ADDR_RANGE content

Much better, thank you. For best clarity I would have added a sentence that this draft
doesn't change a logic described in Section 2.9 of RFC 7296 for 
types TS_IPV4_ADDR_RANGE and TS_IPV6_ADDR_RANGE, it only
defines additional rules for how TS_SECLABEL traffic selectors are negotiated.

Regards,
Valery.

---

From: Paul Wouters <[email-masked]>
Date: Thu, 21 Jan 2021 12:50:35 -0500 (EST)

> with most complex and ineffective way. It's very strange.

Again, this was the result of discussion in the WG where you were very
insistent on how to interpret and send TS payloads. So I'm a little
confused about this being an issue now.

The text above shows how you could implement optional labels, which is
deemed a rare or not needed feature for most implementations. It tells
you to basicaly reconfigure on the fly. The advantage is that the whole
discusion of "optional" does not leak into the IKE TS negotiation
protocol. If you think it would be more clear if we don't describe
the above option, I'm fine with removing that text.

> of the documents we'd better not to start writing them...

It is not about misreading the document. It is to ensure there are less
conditions where there is ambiguity. Does a NULL TS_SECLABEL mean "no
sec label" or "optional sec label" or "wildcard sec_label". We can write
it clearly in the draft, but it is still a flawed concept. The clearest
solution is if you want no restructions on sec_label, to not include any
TS_SECLABEL traffic selector. Removing the zero length from being valid
makes it very clear this cannot be used for anything, including meaning
"a wildcard" or "nothing". If you want a wildcard, you can send a
TS_SECLABEL containing the string "*" (and then figure out whether it is
a single character label or has a string NUL termination :P )

> And BTW zero length means that there are no any content, including NULL terminator :-)

I bring it up because it is a real danger this will get badly
implemented. And it already is. The Linux kernel takes these values in
a struct:

struct xfrm_user_sec_ctx {
 	__u16			len;
 	__u16			exttype;
 	__u8			ctx_alg;  /* LSMs: e.g., selinux == 1 */
 	__u8			ctx_doi;
 	__u16			ctx_len;
};

So you specify the label with ctx_len. Yet the "ip xfrm state" output
shows a trailing @ character, suggesting it is already treating this
as a NUL terminated string. Oddly enough, the output of "ip xfrm policy"
shows the same label without the trailing @.

This is why I originally had stated that the label could not contain a
NULL, but during the process, all this got removed for treating it as
an opaque blob. The libreswan implementation, which uses "strings" of
SElinux values, checks for embedded NULLs and rejects these. But that
is arguably part of the "label consumer" and not IKE.

> No, the document does allow optional security labels, see above.

Not really. It says you first try negotiating "with labels" and if that
fails try to negotiate "without labels". That is different from
supporting a negotiation with optional labels.

Again, it was you who was very strongly of the belief that optional
traffic selectors should not be allowed and that one MUST pick one
of each TS type. And that you must be _selecting_ on something, and
you cannot select on nothing.

Additionally, an implementation accidentally outputting an empty label
into the TS_SECLABEL payload should not introduce a wildcard label in
my opinion. It is just a little safer for them to need to create a "*"
payload.

> and omitting them? Isn't it exactly what you just described as "not security"?

It just describes something you can do with any new Traffic Selector
that will ever be introduced to IKE. You can always try it with it
first, and without it as fallback. It is not a propery of TS_SECLABEL.

> semantics when seclabels are different in TSi and TSr.

Please provide text, but be aware that the document treats the label as
opaque to IKE.

> this is allowed (what does it mean?) or not.

It just tells you that both endpoints will apply the specific label
to the specific IP/proto/ports range when receiving these packets.

I mean, it could even mean a specific group, eg group "seclabel1" and
group "seclabel2". eg:

          TSi = ((0,0,198.51.0.0-198.51.0.255),
                 "jeeps")

                 "awacs-plane")

And the jeeps could only accept awacs-plane traffic, and the awacs-plane
could only accept jeeps traffic.

Sure, in my use case, these will be symmetric, eg: system_u:object_r:ipsec_spd_t:s0

> defines additional rules for how TS_SECLABEL traffic selectors are negotiated.

The idea was to update the TS handling in general for new TS types, not
just TS_SECLABEL. So that new TS Types in the future do not need to
update RFC 7296. So how about this change:

 	A responder MUST create its TS response by selecting at least
 	one of each TS Type. For this selection, TS_IPV4_ADDR_RANGE
 	and TS_IPV6_ADDR_RANGE are considered one (IP) type. That is,
 	it is valid to choose 1 TS of type TS_IPV4_ADDR_RANGE and zero
 	TS of type TS_IPV6_ADDR_RANGE. TS Types supporting narrowing
 	may be narrowed down upon their selection, as it described in
 	Section 2.9 of RFC 7296.

Paul

---

From: "Valery Smyslov" <[email-masked]>
Date: Fri, 22 Jan 2021 10:59:40 +0300

Hi Paul,

> the above option, I'm fine with removing that text.

Then I would remove it for the sake of consistency.

Instead I would add few words about the situation when one peer supports
TS_SECLABLE, while the other one doesn't. RFC7296 isn't clear
what to do if unknown TS_TYPE is present in TS payload.
I think some implementations would return TS_UNACCEPTABLE,
while other may omit unknown traffic selectors in response.

> supporting a negotiation with optional labels.

What is the difference? If using security labels were mandatory for the initiator
it should have failed after the first negotiation and never started the second.

> you cannot select on nothing.

I don't see any contradiction here :-) 

If you allowed zero length TS_SECLABEL to mean "no security label"
and initiator included such a traffic selector (along with non-empty TS_SECLABEL
traffic selectors) then the responder which didn't want to use security labels
for this SA would pick and return an empty TS_SECLABEL.
If it returned no TS_SECLABEL at all, it would have been an error.

Since you strongly object, I'm not insisting on this capability to negotiate "no security labels",
it's only an illustration that I'm consistent in my considerations :-)

> first, and without it as fallback. It is not a propery of TS_SECLABEL.

No, the text in 2.2 refers to Security Label traffic selector only and 
not to any new TS_TYPE.

> Please provide text, but be aware that the document treats the label as

Please, see below.

> could only accept jeeps traffic.

Well, that exactly the sort of explanation I'd like to see in the draft.
Can you please add some words along the lines of the text above 
(and with an example)?

>  	may be narrowed down upon their selection, as it described in

I think the text is good. 

But I'm a bit concerned with the idea to define general rules
for handling new TS types. In particular, you draft says 

   Each TS payload (TSi and TSr) MUST contain at least one TS_TYPE of
   TS_IPV4_ADDR_RANGE or TS_IPV6_ADDR_RANGE.

which is a wrong requirement, since IKEv2 is already used for 
negotiation of non-IP traffic (see RFC 4595).

I think it is better if you only define rules for negotiation
of TS_SECLABEL. If you still want to define general rules,
then the text must be more accurate, not breaking 
the status quo.

Regards,
Valery.