[Swan-dev] loop structure of score_ends_seclabel()

[Paul Wouters]

On Mon, 1 Mar 2021, D. Hugh Redelmeier wrote:

> [These comments would have been better made during code review.  I'm
> sorry that I didn't have time to do this earlier.  Idiosyncratic
> excuse: I gain understanding while nit-picking (see
> a5c5d5ca7828f3b3aa0384b22cc9154acd23f985)]
>
> The loop structure of score_ends_seclabel doesn't look right to me.
> But I don't actually know the logic that it is supposed to implement.
> I may also be misunderstanding the code.
>
> for each tsi
> 	if there's a match with the spd "this"
> 		remember the match_i index
>
> 		for each tsr
> 			if there is a match with the spd "this"
> 				remember the match-r index
>
> What's suspicious:
>
> - There is no early-out for either loop for a match, so the last match
>  will be remembered.  Not the best match.  If any match will do, surely
>  we should stop the loop at the first.

We only need to verify there is a match. We don't need to remember a
best one, since the actual sub-selection is based on the content of
the ACQUIRE.

Also, we don't actually return a Traffic Selector from a "selection".
What we basically do for all Traffic Selector types on the incoming side
is see if we find anything that is configured. If it is, we are good and
once it is time to generate outgoing Traffic Selectors, we generate
them based on the configuration - not on what we received.

We could break out earlier through if we see a right one.

> - Both loops are matching against spd "this".  Surely "that" should be
>  involved in one or the other

That was done on purpose. The Security Label is really a label that
applies to both ends as is configured as a symmetric value
"policy-label". But since we don't want to pass along the connection
to a bunch of functions that take Traffic Selectors (really c->spd)
on loading a connection we take the policy label from whack and we
copy it as Traffic Selector to "this" and "that". But we know they
are always the same (as long as it is based on configuration, not
based on ACQUIRES). But I agree that the code is a bit strange. I
decided not to touch it further because the entire TS matching is
getting a rework to allow matching more than one pair of TS to
generate one IPsec SA with a set of multiple TS's.

> - the variables changed by the outer loop do not affect the inner
>  loop.  It makes no sense to nest them.  My guess: there is a reason
>  in the specs but it isn't in the code.
>
> - match_r is initialized to false before the outer loop.  Perhaps it
>  should also be initialized before the inner loop.  This depends on
>  there being a reason to nest the loops.
>
> - consider the code:
> 			if (cur_i->sec_label.len == 0) {
> 			        /* ??? complain loudly */
> 		        	continue;
>                        }
>  (There is a related test in the inner loop too.)

The code really does need rewriting, I agree. I tried to touch it the
least amount. It is unfortunately re-used for initiator and responder
and it is a bit hard to tell which is which based on the calls. So I
share your discomfort with the code, and hope it will soon all go away.

>  I seem to remember being told that the labels, even though they are
>  a sequence of bytes with a length, are also NUL-terminated strings.

We are trying to be opaque about these, as the draft RFC tells us that
these are opaque payloads. It just happens that the only type of
security labels we support are based on the SElinux policy SAM, and
those happen to use NUL terminated strings.

>  If so
> 	1. this is dumb design: it is a redundant representation and
> 	   pointlessly introduces the ability to have invalid representations
> 	   (the NUL might be missing.)

We should be rejecting any non-NULL terminated security label before we
ever start comparing it. But defense in depth would indeed be better.

> 	2. the test, if it is meant to check for this problem, is only
> 	   partial.  Something like this should be used.
>
> 			if (cur_i->sec_label.len == 0 ||
> 			    cur_i->sec_label.ptr[cur_i->sec_label.len-1] != '\0')
> 			{
> 			        /* ??? complain loudly */
> 		        	continue;
>                        }

Similarly, the draft RFC and our code treat a single NULL as a misformed
label when receiving these.

> 	3. but better: the test should be done when the data-structure is ingested,
> 	   as part of input validation.  Then the rest of Pluto need not deal with
> 	   this.

I believe it does. But the code here is supposed to check that:
- If we don't want security labels as per config, we shouldn't get any
- If we do want security labels as per config, we must get at least one

>        4. this requirement should be documented in the RFC and in the code.
> 	   (Perhaps it is.)

It is?

As I said, I find the entire TS selection code rather scary. And the
current code does not remember valid selections, it only comes out with
a "score" which is ignored because we only really look at "is what we
received compatible with our configuration. If so, just generate
outgoing TS from our configuration". This entire concept has to change,
since we want to remember which of multiple TS selections that we can
match to our configuration will be valid, so we can create IPsec SA's
with more than one TS on it. This work to do this is about to start,
so I would not bother improving this code.

Paul