From: Matthew G. <mg...@sh...> - 2006-07-21 20:35:53
|
All, I was playing around with the sainfo configuration on one of my test boxes and noticed this peculiar behavior. After reviewing the man page regarding anonymous sainfo usage ... sainfo (source_id destination_id | source_id anonymous | anonymous destination_id | anonymous) ... I defined an single semi-anonymous sainfo section so that it would be selected for negotiation ... sainfo address 10.1.1.2 any anonymous { ... } ... when the source id is a clients modecfg assigned ip address and the destination id is anything. But when the client tried to initiate quick mode with the source initiator id being 10.1.1.2 and the destination responder id being 67.18.254.190, the debug log shows ... 2006-07-21 00:59:50: DEBUG: begin. 2006-07-21 00:59:50: DEBUG: seen nptype=8(hash) 2006-07-21 00:59:50: DEBUG: seen nptype=1(sa) 2006-07-21 00:59:50: DEBUG: seen nptype=10(nonce) 2006-07-21 00:59:50: DEBUG: seen nptype=5(id) 2006-07-21 00:59:50: DEBUG: seen nptype=5(id) 2006-07-21 00:59:50: DEBUG: succeed. 2006-07-21 00:59:50: DEBUG: received IDci2:2006-07-21 00:59:50: DEBUG: 01000000 0a010102 2006-07-21 00:59:50: DEBUG: received IDcr2:2006-07-21 00:59:50: DEBUG: 01000000 4312febe [trim] 2006-07-21 00:22:54: ERROR: failed to get sainfo. 2006-07-21 00:22:54: ERROR: failed to get sainfo. 2006-07-21 00:22:54: ERROR: failed to pre-process packet. 2006-07-21 00:22:54: DEBUG: IV freed ... Then I swapped the source_id and destination_id in the sainfo section ... sainfo anonymous address 10.1.1.2 any { ... } ... and the negotiation completed without error. The way I interpret the man page is that the source_id would be initiators id ( or IDci as described in RFC 2409 section 5.5 ) and destination_id would be the responders id ( or IDcr ). However, what seems to work is when the id local to racoon is defined as the source id and the remote peer id is defined as the destination id in the sainfo section. So my question is, am I misunderstanding the man page with the current behavior being the intended behavior or is there some nasty reversed logic in the code somewhere? Thanks, -Matthew |
From: Matthew G. <mg...@sh...> - 2006-07-22 00:54:46
Attachments:
idfix.diff
|
Matthew Grooms wrote: > All, > > So my question is, am I misunderstanding the man page with the > current behavior being the intended behavior or is there some nasty > reversed logic in the code somewhere? > Sorry to reply to myself, but this looks like a problem with get_sainfo_r in isakmp_quick.c. Note that idsrc is assigned iph2->id which is the responders id and ipdst is assigned iph2->id_p which is the initiators id. Whats worse is that it does this after testing that the other pointer is not NULL ... not good. Also, iph2->src ( reponders address ) is substituted for idsrc and iph2->dst ( initiator address ) for iddst as a fallback. Here is a patch. It would be great if someone else could take a look at what I am describing as I have been known to be wrong. -Matthew |
From: <ma...@ne...> - 2006-07-22 06:52:08
|
Matthew Grooms <mg...@sh...> wrote: > Here is a patch. It would be great if someone else could take a look at > what I am describing as I have been known to be wrong. Yvan, please handle that one, I'm clueless about it. -- Emmanuel Dreyfus http://hcpnet.free.fr/pubz ma...@ne... |
From: <ma...@ne...> - 2006-07-22 21:11:16
|
Matthew Grooms <mg...@sh...> wrote: > Here is a patch. It would be great if someone else could take a look at > what I am describing as I have been known to be wrong. I'm not knwoledgable. Yvan, please have a look at it. -- Emmanuel Dreyfus http://hcpnet.free.fr/pubz ma...@ne... |
From: VANHULLEBUS Y. <va...@fr...> - 2006-07-24 20:39:17
|
On Sat, Jul 22, 2006 at 11:11:14PM +0200, Emmanuel Dreyfus wrote: > Matthew Grooms <mg...@sh...> wrote: > > > Here is a patch. It would be great if someone else could take a look at > > what I am describing as I have been known to be wrong. > > I'm not knwoledgable. Yvan, please have a look at it. I've been afk last days, I'll try to handle this ASAP (tommorow ?). Yvan. |
From: VANHULLEBUS Y. <va...@fr...> - 2006-07-26 09:56:51
|
On Fri, Jul 21, 2006 at 07:56:14PM -0500, Matthew Grooms wrote: > Matthew Grooms wrote: [....] > Sorry to reply to myself, but this looks like a problem with > get_sainfo_r in isakmp_quick.c. Note that idsrc is assigned iph2->id > which is the responders id and ipdst is assigned iph2->id_p which is the > initiators id. Whats worse is that it does this after testing that the > other pointer is not NULL ... not good. Also, iph2->src ( reponders > address ) is substituted for idsrc and iph2->dst ( initiator address ) > for iddst as a fallback. Right, there are at least wrong/missing checks there. But as it may break lots of things, I'll have to go deeper in that part of the code to ensure your patch don't break "something else". I'll also have to re-re-read RFC2409 5.5...... Yvan. |
From: Matthew G. <mg...@sh...> - 2006-07-27 05:34:17
|
VANHULLEBUS Yvan wrote: > On Fri, Jul 21, 2006 at 07:56:14PM -0500, Matthew Grooms wrote: >> Matthew Grooms wrote: > [....] >> Sorry to reply to myself, but this looks like a problem with >> get_sainfo_r in isakmp_quick.c. Note that idsrc is assigned iph2->id >> which is the responders id and ipdst is assigned iph2->id_p which is the >> initiators id. Whats worse is that it does this after testing that the >> other pointer is not NULL ... not good. Also, iph2->src ( reponders >> address ) is substituted for idsrc and iph2->dst ( initiator address ) >> for iddst as a fallback. > > Right, there are at least wrong/missing checks there. > Thanks very much for taking a look at it. > But as it may break lots of things, I'll have to go deeper in that > part of the code to ensure your patch don't break "something else". > The function is only called from one place, where racoon is acting as a responder in phase2 and needs to get a matching sainfo. I don't think there is much chance of it breaking anything else. > I'll also have to re-re-read RFC2409 5.5...... > > Just for quick reference as I know you have limited time ... Initiator Responder ----------- ----------- HDR*, HASH(1), SA, Ni [, KE ] [, IDci, IDcr ] --> Racoon looks like it stores the peer id as iph2->id_p and the local id as iph2->id. --- Payload id encoding as initiator in quick_i1send --- /* IDci */ np = (idcr) ? ISAKMP_NPTYPE_ID : ISAKMP_NPTYPE_NONE; if (idci) p = set_isakmp_payload(p, iph2->id, np); /* IDcr */ if (idcr) p = set_isakmp_payload(p, iph2->id_p, ISAKMP_NPTYPE_NONE); --- Payload id decoding as responder in quick_r1recv --- if (iph2->id_p == NULL) { /* for IDci */ f_id_order++; if (isakmp_p2ph(&iph2->id_p, pa->ptr) < 0) goto end; } else if (iph2->id == NULL) { /* for IDcr */ if (f_id_order == 0) { plog(LLV_ERROR, LOCATION, NULL, "IDr2 payload is not " "immediatelly followed " "by IDi2. We allowed.\n"); /* XXX we allowed in this case. */ } if (isakmp_p2ph(&iph2->id, pa->ptr) < 0) goto end; } --- end --- Thanks again, -Matthew |
From: VANHULLEBUS Y. <va...@fr...> - 2006-07-26 15:55:48
|
Ok..... >From RFC 2409, section 5.5, IDci is Initiator's value, IDcr is Responder's value, and they are sent in that order. >From quick_i2recv() and quick_r1recv(), iph2->id is always ID from *our* side, and iph2->id_p is always ID for peer. Now, in get_sainfo_r(): If we do have a peer ID (id_p), we set the idsrc as local ID (id). This would seem more logic to test id, or at least to use id_p But if id_p is NULL, we get idsrc from local IP(iph2->src is local IP, even in responder mode). Then if we do have a local id (id), we set the iddst as peer's ID (id_p). And if id is NULL, we get iddst from peer's IP. In facts, most of the time, we'll have both IDs (Tunnel mode), or none of them (Transport mode), so it works, but not with the same order for sainfos. So only sainfo "local -> peer" should be needed, and this is the sainfo which should always be used, as initiator and as responder, in all cases. But this doesn't match the man page for tunnel mode, which says that the first address is the IDci, and the second is the IDcr. The next question is: what should we break ? My idea is to change the man page, to tell that sainfos should be "sainfo local remote". This won't break existing configurations, as this is the way it actually works ! This would be more simpler, and ensure that only one sainfo is needed for a peer, enven if both peers can initiate the tunnel. And in the source code, we would just have to reverse id / id_p checks (well, Matthew, I guess this is what your patch does, but it is simpler to just reverse the tests than reversing all the work within tests :-). Does this match your tests ? I'll have to do some real world tests to confirm my code analysis (perhaps I missed another code which also revert something....), and to check the modification. Yvan. |
From: Matthew G. <mg...@sh...> - 2006-07-26 19:38:23
|
VANHULLEBUS Yvan wrote: > > My idea is to change the man page, to tell that sainfos should be > "sainfo local remote". This won't break existing configurations, as > this is the way it actually works ! > This would have been my other suggestion. But my preference would be to change to code to match the documentation. > > This would be more simpler, and ensure that only one sainfo is needed > for a peer, enven if both peers can initiate the tunnel. > If you don't mind not being able to create a specific sainfo that only works in one direction. I suppose its one of those simplicity vs flexibility question. But lets face it, most admins just use a single anonymous remote and sainfo anyhow. Either way, I don't think there will be much grief for anyone. > > And in the source code, we would just have to reverse id / id_p > checks (well, Matthew, I guess this is what your patch does, but it is > simpler to just reverse the tests than reversing all the work within > tests :-). > Sure, sounds like you came to the same conclusions I did. My goal was to get things working like the man page states it should. > > > Does this match your tests ? > Yes. > I'll have to do some real world tests to confirm my code analysis > (perhaps I missed another code which also revert something....), and > to check the modification. > I think you will find the results consistent with what I have reported. Thanks, -Matthew |