[Swan-dev] 3.16rc2 memory corruption in md->packet ?

Paul Wouters paul at nohats.ca
Fri Dec 4 18:51:50 UTC 2015


On Fri, 4 Dec 2015, Tuomo Soini wrote:

>> | #1  0x00007f1334390f42 in release_md (md=0x7f13376a38b0) at
>> | /usr/src/debug/libreswan-3.16rc2/programs/pluto/msgdigest.c:107
>> | #2  0x00007f1334351017 in release_fragments
>> (st=st at entry=0x7f1337687010) at
>> | /usr/src/debug/libreswan-3.16rc2/programs/pluto/state.c:659
>>
>> This is crashing in the code to delete V1 IKE fragments that might be
>> still hung on the state object.  The chance of this happening when a
>> timer goes of would seem low.  Unless some fragments were lost and the
>> state object was hanging around to get that lost fragment.
>>
>> I wonder if we've tested that case?  Probably not.
>>
>> Tuomo: were you using V1 with fragmented IKE messages?
>> Were you running with electric fence?
>
> This is ikev2 only system. So if we are in ikev1 only code we have
> quite clearly a bug.

Note that Hugh seems to be wrong about IKEv1.

The code that triggered was (timer.c):

                if (st->st_ikev2 && IS_IKE_SA(st)) {
                         /* IKEv2 parent, delete children too */
                         delete_my_family(st, FALSE);
                         /* note: no md->st to clear */

So this is IKEv2.

then checking delete_my_family()

                        if (v2_responder_state)
                                 change_state(st, STATE_CHILDSA_DEL);
                         delete_state(st);

and delete_state():

        /* from here on we are just freeing RAM */

         ikev1_clear_msgid_list(st);
         unreference_key(&st->st_peer_pubkey);
         release_fragments(st);

/*
  * Release both V1 and V2 fragments
  * (of course only one kind could appear in a state)
  */
void release_fragments(struct state *st)
{
         struct ike_frag *frag = st->ike_frags;

         while (frag != NULL) {
                 struct ike_frag *this = frag;

                 frag = this->next;
                 release_md(this->md);
                 pfree(this);
         }

         st->ike_frags = NULL;

         release_v2fragments(st);
}

Note the comment might be wrong! What happens if a conn switches from v1
to v2. I _think_ that we delete the state and create a new one, but I'm
not sure if that's a contract in our API. If we are sure then we can
protect this code using a check for st->st_ikev2 and never call
release_md()


So somehow our code set st->ike_frags and if so, this code
here is missing a check for v1 versus v2 before doing v1 code

Let's look at release_v2fragments()

void release_v2fragments(struct state *st)
{
         struct ikev2_frag *frag = st->st_tfrags;

         while (frag != NULL) {
                 struct ikev2_frag *this = frag;

                 frag = this->next;
                 freeanychunk(this->cipher);
                 pfree(this);
         }

         st->st_tfrags = NULL;
}

Note that different state structs are used, IKEv1 has st->ike_frags and
IKEv2 has st->st_tfrags. (I missed this first, we should rename these)

So the real question remains, how did the IKEv1 st->ike_frags become
non-NULL ?

I don't know why Tuomo hit the v1 code doing a release_md() instead of
getting passed to the release_v2fragments() call. I checked if the
code got confused between ike_frags and st_tfrags but that's not the
case:

paul at bofh:~/libreswan/programs/pluto (master)$ grep ike_frags ikev2*
paul at bofh:~/libreswan/programs/pluto (master)$ grep st_tfrags ikev1*
paul at bofh:~/libreswan/programs/pluto (master)$

Did the connection switch between ikev1 and ikev2? But even if it did
that, why would the pointer be rubbish?. Looking at the timer.c code,
it passes st to ipsecdoi_replace() but no one updates st->st_ikev2
and oddly it seems to call ipsecdoi_initiate() which can call
quick_outI1() albeit for only v1 states but the if/then clause seems
to have no clue about ikev2 states?

Checking who sets st_ike2 I see ikev2parent_outI1() which does so
on a newly created struct st, and ikev2parent_inI1outR1() which
also does it on a newly created struct st. So a retry that goes
from ikev2 to ikev2 i think will still have st_ikev2 = TRUE set!

What also worries me is that c->failed_ikev2 is used to flip over
connections between v1 and v2, but that's a conn variable and not
a state variable. It's kind of weird.

Paul


More information about the Swan-dev mailing list