[Swan-dev] Question on get_cookie() code

[Paul Wouters]

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 */
}


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

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.

Paul