From: Bob G. <bob...@gm...> - 2008-03-05 23:15:32
|
First question: Why does the racoon client ignore CERT_REQ payloads in phase 1 of the exchange? v0.7 of racoon sends out an initial packet with an SA and Vendor ID; the ISAKMP server I'm contacting responds with an SA, CERT_REQ, and VENDOR_ID payloads, and racoon ignores the CERT_REQ despite assertions in RFC 2408 that an ISAKMP client MUST respond to a CERT or CERT_REQ payload at any point during the exchange (sections 5.9/5.10). Secondly, I have a patch for ipsec_doi.c that implements robust transform comparison processing. According to the RFC, the returned transforms SHOULD be numbered the same as the transforms that are sent in order to speed up the exchange, but this is not necessary. Since I have an ISAKMP server that doesn't number the returned transforms the same way, the original comparison method used by racoon to just compare transform numbers fails. For maximum interoperability, racoon should be falling back and comparing the transform parameters directly. The diff below to ipsec_doi.c does exactly that. Comments on either subject, including the suitability of inclusion of the patch into ipsec-tools, are welcome. -Bob diff -ur ipsec-tools-0.7/src/racoon/ipsec_doi.c ipsec-tools-0.7.1/src/racoon/ipsec_doi.c --- ipsec-tools-0.7/src/racoon/ipsec_doi.c 2007-08-01 06:52:20.000000000 -0500 +++ ipsec-tools-0.7.1/src/racoon/ipsec_doi.c 2008-03-04 15:45:05.000000000 -0600 @@ -926,7 +926,7 @@ { struct prop_pair **rpair = NULL, **spair = NULL; struct prop_pair *p; - int i, n, num; + int i, n, num, smax, matched; int error = -1; vchar_t *sa_ret = NULL; @@ -951,11 +951,14 @@ /* check proposal is only one ? */ n = 0; num = 0; + smax = 0; for (i = 0; i < MAXPROPPAIRLEN; i++) { if (rpair[i]) { n = i; num++; } + if (spair[i]) + smax = i; } if (num == 0) { plog(LLV_ERROR, LOCATION, NULL, @@ -982,8 +985,24 @@ if (cmp_aproppair_i(rpair[n], spair[n])) { plog(LLV_ERROR, LOCATION, NULL, - "proposal mismathed.\n"); - goto end; + "proposal mismatched: using fallback check\n"); + + matched = 0; + for (i = 0; i < MAXPROPPAIRLEN; i++) + { + if (spair[i]) + { + if (!cmp_aproppair_i(rpair[n], spair[i])) + matched = 1; + } + } + + if(!matched) + { + plog(LLV_ERROR, LOCATION, NULL, + "All propoosals mismatched in fallback.\n"); + goto end; + } } /* @@ -1015,6 +1034,134 @@ return error; } +static void +cmp_aproppair_i_log(m1, m2, m1len, m2len) + void *m1, *m2; + int m1len, m2len; +{ + int i, j; + char sbuf[2048]; + char xbuf[32]; + unsigned char *x; + + plog(LLV_DEBUG, LOCATION, NULL, "P Transform:\n"); + for(i = 0, x = (unsigned char *)m1; i < m1len; i += 8) + { + sprintf(sbuf, "%04x: ", i); + for(j = i; j < i + 8 && j < m1len; j++) + { + sprintf(xbuf, "%02x ", x[j]); + strcat(sbuf, xbuf); + } + strcat(sbuf, "\n"); + plog(LLV_DEBUG, LOCATION, NULL, sbuf); + } + + plog(LLV_DEBUG, LOCATION, NULL, "R Transform:\n"); + for(i = 0, x = (unsigned char *)m2; i < m2len; i += 8) + { + sprintf(sbuf, "%04x: ", i); + for(j = i; j < i + 8 && j < m2len; j++) + { + sprintf(xbuf, "%02x ", x[j]); + strcat(sbuf, xbuf); + } + strcat(sbuf, "\n"); + plog(LLV_DEBUG, LOCATION, NULL, sbuf); + } +} + +static int +subcmp_aproppair_i(a, b) + struct prop_pair *a, *b; +{ + struct isakmp_data *pa, *pb; + int amax, bmax; + unsigned int key, value, i, j, len, bkey, bvalue; + unsigned int a_types_seen = 0, b_types_seen = 0; + + plog(LLV_DEBUG, LOCATION, NULL, "- Detailed subcomparison in progress.\n"); + + amax = ntohs(a->trns->h.len - sizeof(*a->trns)) / sizeof(struct isakmp_data); + bmax = ntohs(b->trns->h.len - sizeof(*b->trns)) / sizeof(struct isakmp_data); + + pa = (struct isakmp_data *)(a->trns + 1); + pb = (struct isakmp_data *)(b->trns + 1); + + /* for all original attributes */ + for(i = 0; i < amax; i++) + { + /* retrieve the key and value for this attribute */ + key = ntohs(pa[i].type) & (~ISAKMP_GEN_TV); + a_types_seen |= (1 << key); + + /* Type/Value pair */ + if(ntohs(pa[i].type) & ISAKMP_GEN_TV) + value = ntohs(pa[i].lorv); + else + { + /* Type/Length/Value triple, only 4-octet supported */ + if(ntohs(pa[i].lorv) != 4) + { + plog(LLV_ERROR, LOCATION, NULL, "SA attribute length of %d octets not supported.", ntohs(pa[i].lorv)); + return -1; + } + value = *(int *)(pa + i + 1); + /* manually advance counter beyond the value */ + i++; + } + plog(LLV_DEBUG, LOCATION, NULL, "- Key %u, value %u\n", key, value); + + /* check against returned attributes */ + for(j = 0, bkey = 0, bvalue = 0; j < bmax; j++) + { + /* retrieve key/value for this attribute */ + bkey = ntohs(pb[j].type) & (~ISAKMP_GEN_TV); + b_types_seen |= (1 << key); + + /* Type/Value pair */ + if(ntohs(pb[j].type) & ISAKMP_GEN_TV) + bvalue = ntohs(pb[j].lorv); + else + { + /* Type/Length/Value triple, only 4-octet supported */ + if(ntohs(pb[j].lorv) != 4) + { + plog(LLV_ERROR, LOCATION, NULL, "SA attribute length of %d octets not supported.", ntohs(pb[j].lorv)); + return -1; + } + bvalue = *(int *)(pb + j + 1); + /* manually advance counter beyond the value */ + j++; + } + + plog(LLV_DEBUG, LOCATION, NULL, "- CKey %u, Cvalue %u\n", bkey, bvalue); + + /* value present but mismatched */ + if(bkey == key && bvalue == value) + break; + } + + /* value not present */ + if(bkey != key || bvalue != value) + { + plog(LLV_DEBUG, LOCATION, NULL, "- Transform SA attribute mismatch (or not found).\n"); + return -1; + } + } + + /* b had extra attributes not present in a? */ + if(b_types_seen != a_types_seen) + { + plog(LLV_DEBUG, LOCATION, NULL, "- Extra transform SA attributes present.\n"); + return -1; + } + + /* all attributes matched, no extra attributes */ + plog(LLV_DEBUG, LOCATION, NULL, "- SA attributes and values matched.\n"); + return 0; +} + /* * compare two prop_pair which is assumed to have same proposal number. * the case of bundle or single SA, NOT multi transforms. @@ -1035,9 +1182,25 @@ for (p = a, q = b; p && q; p = p->next, q = q->next) { for (r = q; r; r = r->tnext) { - /* compare trns */ + /* compare trns number */ + plog(LLV_DEBUG, LOCATION, NULL, + "transform comparison numbers: %d, %d.\n", p->trns->t_no, r->trns->t_no); if (p->trns->t_no == r->trns->t_no) break; + /* compare transform payload */ + if (p->trns->t_id == r->trns->t_id && !subcmp_aproppair_i(p, r)) + { + plog(LLV_INFO, LOCATION, NULL, "backup comparison via properties succeeded!\n"); + break; + } + else + { + plog(LLV_DEBUG, LOCATION, NULL, "-- Subcomparison failed (transform IDs: %d, %d)\n", p->trns->t_id, r->trns->t_id); + plog(LLV_DEBUG, LOCATION, NULL, "-- Packet dump:\n"); + cmp_aproppair_i_log(p->trns + 1, r->trns + 1, + ntohs(p->trns->h.len) - sizeof(*p->trns), + ntohs(r->trns->h.len) - sizeof(*r->trns)); + } } if (!r) { /* no suitable transform found */ |
From: VANHULLEBUS Y. <va...@fr...> - 2008-03-06 09:00:48
|
Hi. On Wed, Mar 05, 2008 at 03:47:39PM -0600, Bob Glamm wrote: [...] > Secondly, I have a patch for ipsec_doi.c that implements robust > transform comparison > processing. According to the RFC, the returned transforms SHOULD be numbered > the same as the transforms that are sent in order to speed up the > exchange, but this > is not necessary. Since I have an ISAKMP server that doesn't number > the returned > transforms the same way, the original comparison method used by racoon > to just compare > transform numbers fails. For maximum interoperability, racoon should > be falling back > and comparing the transform parameters directly. The diff below to > ipsec_doi.c does > exactly that. Thanks for the patch. I'm having a look at it, but as it is not trivial, I guess we'll report it to HEAD first, then, after some tests, we will see if we can report it in 0.7 branch. Yvan. |
From: Bob G. <bob...@gm...> - 2008-03-07 16:52:02
|
Aha, good catch on the smax - it was supposed to be an optimization to limit the number of comparisons but I used MAXPROPPAIRLEN instead. I don't really care if it's applied to 0.7, as long as the fix gets into the software. How about this - I'll download the HEAD branch and revise both this patch and the CERT_REQ patch for isakmp_ident.c and resubmit it as a single patch? -Bob On Thu, Mar 6, 2008 at 3:00 AM, VANHULLEBUS Yvan <va...@fr...> wrote: > > Hi. > > On Wed, Mar 05, 2008 at 03:47:39PM -0600, Bob Glamm wrote: > [...] > > > Secondly, I have a patch for ipsec_doi.c that implements robust > > transform comparison > > processing. According to the RFC, the returned transforms SHOULD be numbered > > the same as the transforms that are sent in order to speed up the > > exchange, but this > > is not necessary. Since I have an ISAKMP server that doesn't number > > the returned > > transforms the same way, the original comparison method used by racoon > > to just compare > > transform numbers fails. For maximum interoperability, racoon should > > be falling back > > and comparing the transform parameters directly. The diff below to > > ipsec_doi.c does > > exactly that. > > > Thanks for the patch. > > I'm having a look at it, but as it is not trivial, I guess we'll > report it to HEAD first, then, after some tests, we will see if we can > report it in 0.7 branch. > > > > Yvan. > |
From: VANHULLEBUS Y. <va...@fr...> - 2008-03-08 16:47:19
|
On Fri, Mar 07, 2008 at 10:52:04AM -0600, Bob Glamm wrote: > Aha, good catch on the smax - it was supposed to be an optimization to limit the > number of comparisons but I used MAXPROPPAIRLEN instead. Ok :-) > I don't really care > if it's applied to 0.7, as long as the fix gets into the software. Well, it *is* interesting for 0.7 branch, but we also want to ensure we won't beak anything in the "production" branch.... > How about this - I'll > download the HEAD branch and revise both this patch and the CERT_REQ patch > for isakmp_ident.c and resubmit it as a single patch? No, please keep patches separated if possible, as we may handle them separately, and as we'll at least have separate commit logs for files. Yvan. |