Arnaud Ebalard | 17 Jan 12:52 2008

Missing scope id when creating sockaddr from ID

Hi,

When constructing a sockaddr from an ID using ipsecdoi_id2sockaddr(),
the passed sockaddr_storage considered as a sockaddr_in6 does not have
its scope_id set (IPv6 only).

In a recent patch commited to HEAD, the code below (isakmp_quick.c)
triggers the bug by providing uninitialized struct sockaddr_storage to
the function. 

  /* identity check */
  if (idci != NULL) {
          struct sockaddr_storage proposed_addr, got_addr;
          u_int8_t proposed_prefix, got_prefix;
          u_int16_t proposed_ulproto, got_ulproto;

          error = ipsecdoi_id2sockaddr(iph2->id,
                                  (struct sockaddr *) &proposed_addr,
                                  &proposed_prefix, &proposed_ulproto);
          if (error)
                  goto end;

          error = ipsecdoi_id2sockaddr(idci,
                                  (struct sockaddr *) &got_addr,
                                  &got_prefix, &got_ulproto);

Then, the test between the two sockaddr fails in cmpsaddrstrict()
because the scope id is compared and it is filled with garbage.

    if (cmpsaddrstrict((struct sockaddr *) &proposed_addr,
                       (struct sockaddr *) &got_addr) == 0) {
            plog(LLV_DEBUG, LOCATION, NULL,
                    "IDci matches proposal.\n");

The patch attached to this email is against today's CVS version. It
forces the scope id to 0 *by default* in ipsecdoi_id2sockaddr() when
filling a sockaddr_in6 from an ID that contain an IPv6 address. With
that, providing 2 identical ID to that function makes the comparison
between the resulting sockaddr ok.

AFAICS, in the rest of racoon code, calls to ipsecdoi_id2sockaddr() are
usually  followed by a setscopeid() call to explicitly set the scope id
to a meaningful value (the one from the SA address). Here, I don't think
there is any useful value and the bug is in ipsecdoif_id2sockaddr(). 

IMHO, it would also be a good idea to nullify the sockaddr_storage
structures before they are used. The patch does not include such code.

Cheers,

a+

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Gmane