[Swan-dev] Question on get_cookie() code

[Andrew Cagney]

On 5 January 2016 at 18:29, Paul Wouters <paul at nohats.ca> wrote:
>
> I'm looking at get_cookie() which is used to generate SPI's for the IKE
> SA. The function has basically been the same since the old freeswan
> days:
>
> void get_cookie(bool initiator, u_int8_t *cookie, int length,
>                 const ip_address *addr)
> {
>         u_char buffer[SHA1_DIGEST_SIZE];
>         SHA1_CTX ctx;
>
>         do {
>                 if (initiator) {
>                         get_rnd_bytes(cookie, length);
>                 } else {
>                         /* Responder cookie */
>                         /* This looks as good as any way */
>                         size_t addr_length;
>                         static u_int32_t counter = 0;
>                         unsigned char addr_buff[
>                                 sizeof(union { struct in_addr A;
>                                                struct in6_addr B;
>                                        })];
>
>                         addr_length =
>                                 addrbytesof(addr, addr_buff,
>                                             sizeof(addr_buff));
>                         SHA1Init(&ctx);
>                         SHA1Update(&ctx, addr_buff, addr_length);
>                         SHA1Update(&ctx, secret_of_the_day,
>                                    sizeof(secret_of_the_day));
>                         counter++;
>                         SHA1Update(&ctx, (const void *) &counter,
>                                    sizeof(counter));
>                         SHA1Final(buffer, &ctx);
>                         memcpy(cookie, buffer, length);
>                 }
>         } while (is_zero_cookie(cookie)); /* probably never loops */
> }

It seems to be feeding SHA1 crud found on the stack?

For EH/ESP, there is also:

ipsec_spi_t get_ipsec_spi(ipsec_spi_t avoid, int proto, const struct
spd_route *sr,
              bool tunnel)
{
    static ipsec_spi_t spi = 0; /* host order, so not returned directly! */
    char text_said[SATOT_BUF];

    passert(proto == IPPROTO_AH || proto == IPPROTO_ESP);
    set_text_said(text_said, &sr->this.host_addr, 0, proto);

    if (kernel_ops->get_spi != NULL) {
        return kernel_ops->get_spi(&sr->that.host_addr,
                       &sr->this.host_addr, proto, tunnel,
                       get_proto_reqid(sr->reqid, proto),
                       IPSEC_DOI_SPI_OUR_MIN, 0xffffffff,
                       text_said);
    }

    spi++;
    while (spi < IPSEC_DOI_SPI_OUR_MIN || spi == ntohl(avoid))
        get_rnd_bytes((u_char *)&spi, sizeof(spi));

    DBG(DBG_CONTROL,
        {
            ipsec_spi_t spi_net = htonl(spi);

            DBG_dump("generate SPI:", (u_char *)&spi_net,
                 sizeof(spi_net));
        });

    return htonl(spi);
}

i.e.,:
  length = 4
  if klips(I'm guessing):
     generate something within some arbitrary bounds
  else:
     ask the kernel, kind of, maybe?

so again, get_rnd_bytes, with a non-zero check seems sufficient for
the klips case.

>
> My question is, why is there a different process for generating the
> initiator and responder SPI? Both just need to be very random.

For IKEv2, they need to be non-zero, given the odds of that are low,
I've been using an initial hack:

> And since we are doing a SHA1 on this, we are still basically returning
> random but just in a more convulated way.
>
> Additionally, since the SPI in IKEv1 and IKEv2 is always 8 octets, all
> callers to this function pass a length of 8, so we might as well phase
> out the argument.
>
> The while() seems to cover an extremely extremely rare case, which is
> just as rare as generating the same SPI twice, a condition that is never
> checked for.
>
> However, if we remove the unused length argument, the useless bool for
> initiator/responder when using random in all cases, there isn't really
> much left in this function. Should we just kill it? There are 6 callers
> to this function, two each (initiator/responder) for aggressive mode,
> main mode and ikev2.
>
> If there is still a reason to keep it, I would like to rename it to
> get_rnd_ikespi() or something, as "cookie" is a misleading term.

I've been working on the assumption that IKEv2 will have its own SPI
generators (at least for AH/ESP / CHILD_SA).

And please consider going further, and s/icookie/initiator_spi/ et.al.
so that it makes reading the code from an IKEv2 much easier.

   "This is not the cookie you are looking for"

> Paul
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev