[Swan-dev] IKEv2: fix a crash when only one endpoint specifies policy-label=

[Wewegama, Kavinda]

Hi Hugh,

I think it makes sense to replace the final IF with just a `return`:

if (sec_label.len == 0) {
    /* This endpoint is not configured to use labeled IPsec. */
    if (!tsi->contains_sec_label && !tsr->contains_sec_label) {
        /* No sec_label was found and none was expected. */
        return NULL;
    }

    /*
     * Error: This end is *not* configured to use labeled
     * IPsec but the peer is.
     */
    return &null_shunk;
}

Thanks for taking a look at this and providing feedback!

-Kavinda

> -----Original Message-----
> From: Swan-dev <swan-dev-bounces at lists.libreswan.org> On Behalf Of D.
> Hugh Redelmeier
> Sent: Wednesday, March 10, 2021 12:09 PM
> To: swan-dev at lists.libreswan.org
> Subject: EXTERNAL: Re: [Swan-dev] IKEv2: fix a crash when only one
> endpoint specifies policy-label=
> 
> | commit 2a2376e5bfa6c19e9a334e2a651b54135e64ab21
> | Author: Kavinda Wewegama <kavinda.wewegama at forcepointgov.com>
> | Date:   Tue Mar 9 21:38:55 2021 -0500
> |
> |     IKEv2: fix a crash when only one endpoint specifies policy-label=
> |
> |     Signed-off-by: Paul Wouters <pwouters at redhat.com>
> 
> Code is extracted from score_ends_seclabel:
> 
>     if (sec_label.len == 0) {
>         /* This endpoint is not configured to use labeled IPsec. */
>         if (!tsi->contains_sec_label && !tsr->contains_sec_label) {
>             /* No sec_label was found and none was expected? */
>             return NULL;    /* success: no label, as expected */
>         }
> 
>         if (tsi->contains_sec_label || tsr->contains_sec_label) {
>             /*
>              * Error: This end is *not* configured to use labeled
>              * IPsec but the peer is.
>              */
>             return &null_shunk;
>         }
>     }
> 
>     /* This endpoint is configured to use labeled IPsec. */
> 
> This code confuses me.
> 
> The final IF condition is the complement of the previous one.
> So an ELSE would do the job.  But since the THEN clause never falls through,
> an ELSE isn't even needed.
> 
> It isn't clear, but the the THEN case of the outer IF never falls through.
> 
> I'll commit code that makes this clearer.
> 
> Is this what is intended?
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev