xorl %eax, %eax

IPSec-Tools Null Pointer Dereference

with 2 comments

I saw this in the NetBSD’s CVS-web a couple of minutes ago. Credits go to Neil Kettle for this bug and its fix. This bug can be found at the fragmentation routines of the ISAKMP of ipsec-tools and affects releases prior to 0.7.2. Here is the buggy routine:

176 int
177 isakmp_frag_extract(iph1, msg)
178         struct ph1handle *iph1;
179         vchar_t *msg;
180 {
181         struct isakmp *isakmp;
182         struct isakmp_frag *frag;
183         struct isakmp_frag_item *item;
184         vchar_t *buf;
 ...
190         if (msg->l < sizeof(*isakmp) + sizeof(*frag)) {
191                 plog(LLV_ERROR, LOCATION, NULL, "Message too short\n");
192                 return -1;
193         }
194
195         isakmp = (struct isakmp *)msg->v;
196         frag = (struct isakmp_frag *)(isakmp + 1);
197
198         /*
199          * frag->len is the frag payload data plus the frag payload header,
200          * whose size is sizeof(*frag)
201          */
202         if (msg->l < sizeof(*isakmp) + ntohs(frag->len)) {
203                 plog(LLV_ERROR, LOCATION, NULL, "Fragment too short\n");    
204                 return -1;
205         }
206
207         if ((buf = vmalloc(ntohs(frag->len) - sizeof(*frag))) == NULL) {    
208                 plog(LLV_ERROR, LOCATION, NULL, "Cannot allocate memory\n");
209                 return -1;
210         }
 ...
219         memcpy(buf->v, data, buf->l);
 ...
263 }


This code is located at src/crypto/dist/ipsec-tools/src/racoon/isakmp_frag.c of the NetBSD’s source code tree. The bug could appear if ntohs(frag->len) was equal to sizeof(*frag). In this case the return pointer of vmalloc() at line 207 would NOT be NULL since vmalloc() which can be seen at src/crypto/dist/ipsec-tools/src/racoon/vmbuf.c does the following:

52 vchar_t *
53 vmalloc(size)
54         size_t size;
55 {
56         vchar_t *var;
57
58         if ((var = (vchar_t *)racoon_malloc(sizeof(*var))) == NULL)
59                 return NULL;
60
61         var->l = size;
62         if (size == 0) {
63                 var->v = NULL;
64         } else {
65                 var->v = (caddr_t)racoon_calloc(1, size);
66                 if (var->v == NULL) {
67                         (void)racoon_free(var);
68                         return NULL;
69                 }
70         }
71
72         return var;
73 }


As you can see, if its argument, size, is zero (line 62) it will set var->v (or in our case this would be buf->v) to NULL and return var at line 72 which is not NULL (see line 58). This means that buf->v is NULL and a dereference will happen at line 219 when memcpy() will attempt to write to this address. This could be also triggered, if sizeof(*frag) is greater than ntohs(frag->len) which will result in an integer overflow during the substration. This will have the exact same effect because of the racoon_calloc() at line 65 since buf->v would be NULL and memcpy() at line 219 will attempt to write to it. This was patched by performing an additional check for this condition like this:

 */
-	if (msg->l < sizeof(*isakmp) + ntohs(frag->len)) {
+	if (msg->l < sizeof(*isakmp) + ntohs(frag->len) ||
+	    ntohs(frag->len) < sizeof(*frag) + 1) {
 plog(LLV_ERROR, LOCATION, NULL, "Fragment too short\n");


Really interesting bug in my opionion…

Written by xorl

May 4, 2009 at 21:42

Posted in bugs, netbsd

2 Responses

Subscribe to comments with RSS.

  1. thank you keep the good work

    security-root

    May 5, 2009 at 04:17

  2. thanks for your coverage! it was an interesting bug! I checked several ways to reach a buggy call to vmalloc, this was by far the simplest!. The actual dereference caused is in isakmp_main when the fragments are re-combined.

    mu-b

    May 5, 2009 at 20:14


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s