xorl %eax, %eax

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…