From: Ian Jackson Date: Thu, 25 Jul 2013 17:30:51 +0000 (+0100) Subject: site: Extra info in name fields for MSG1, clearer processing X-Git-Tag: debian/0.3.0_beta2~15 X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~ian/git?p=secnet.git;a=commitdiff_plain;h=7e29719ec13da8db08c7aa4753d958141967bf0a;ds=sidebyside site: Extra info in name fields for MSG1, clearer processing We change the parsing of the names in MSG1 packets to align it with that used for MSG2..4, abolishing the simple memcmp-based "setupsig" arrangement. This means that MSG1 packets, like MSG2..4, can contain extra information in the name fields. This results in the following table of behaviours (where "old secnet" is the version before we introduced additional data at all): MSG Sent name Old secnet New secnet 1 expected ok ok 1 expected\0extra rejected ok, extra data 1 expected/suffix rejected rejected 1 expect rejected rejected 2-4 expected ok ok 2-4 expected\0extra ok, extra ignored ok, extra data 2-4 expected/suffix wrongly accepted rejected 2-4 expect read overrun rejected After the new secnet is widely deployed, we will be able to use the extra data field in MSG1 for public key algorithm agility and public key rollover. Also, intend to introduce a new packet which (like MSG1) is addressed by name rather than the site id; this change makes that easier too. In detail, we: * Make site_incoming extract the message type earlier, and use the message type to choose how to check the address. * Abolish st->setupsig and st->setupsiglen; * Break out a function named_for_us, which uses unpick_msg and then checks the names; * Arrange to keep the unpicked message from named_for_us and reuse it in process_msg1. * Eliminate checking dest==0 for name-addressed packets. This last point constitutes a change to the implemented protocol. The old code would treat a packet as name-addressed iff the destination "index" was zero. However, the requirement to send a zero in this field is not documented. After this patch, peers which send MSG1 with a non-zero destination index will have their messages honoured rather than discarded. But we do document the restriction. There is no other significant functional implication of this last point. Peers which send non-MSG1s with zero destination index will have their packets rejected both before and afterwards (although there may be a change in whether a NAK is sent and in logging) but anyway those peers are broken because all of our generated indexes are nonzero. A downside of this change is that we will now unpick an incoming MSG1 repeatedly until we find which site instance wants it. But this unpicking is not particularly arduous. Signed-off-by: Ian Jackson --- diff --git a/NOTES b/NOTES index ddd14a5..3e9d71d 100644 --- a/NOTES +++ b/NOTES @@ -198,7 +198,10 @@ The protocol version selection stuff is not yet implemented. Messages: -1) A->B: *,iA,msg1,A,B,nA +1) A->B: *,iA,msg1,A+,B+,nA + +i* must be encoded as 0. (However, it is permitted for a site to use +zero as its "index" for another site.) 2) B->A: iA,iB,msg2,B+,A+,nB,nA diff --git a/site.c b/site.c index 2e8335a..98bd6b6 100644 --- a/site.c +++ b/site.c @@ -253,9 +253,6 @@ struct site { after this time, initiate a new key exchange */ - uint8_t *setupsig; /* Expected signature of incoming MSG1 packets */ - int32_t setupsiglen; /* Allows us to discard packets quickly if - they are not for us */ bool_t setup_priority; /* Do we have precedence if both sites emit message 1 simultaneously? */ uint32_t log_events; @@ -506,19 +503,15 @@ static bool_t generate_msg1(struct site *st) } static bool_t process_msg1(struct site *st, struct buffer_if *msg1, - const struct comm_addr *src) + const struct comm_addr *src, struct msg *m) { - struct msg m; - /* We've already determined we're in an appropriate state to process an incoming MSG1, and that the MSG1 has correct values of A and B. */ - if (!unpick_msg(st,LABEL_MSG1,msg1,&m)) return False; - transport_record_peer(st,&st->setup_peers,src,"msg1"); - st->setup_session_id=m.source; - memcpy(st->remoteN,m.nR,NONCELEN); + st->setup_session_id=m->source; + memcpy(st->remoteN,m->nR,NONCELEN); return True; } @@ -1254,6 +1247,18 @@ static void site_outgoing(void *sst, struct buffer_if *buf) initiate_key_setup(st,"outgoing packet"); } +static bool_t named_for_us(struct site *st, const struct buffer_if *buf_in, + uint32_t type, struct msg *m) + /* For packets which are identified by the local and remote names. + * If it has our name and our peer's name in it it's for us. */ +{ + struct buffer_if buf[1]; + buffer_readonly_clone(buf,buf_in); + return unpick_msg(st,type,buf,m) + && name_matches(&m->remote,st->remotename) + && name_matches(&m->local,st->localname); +} + /* This function is called by the communication device to deliver packets from our peers. */ static bool_t site_incoming(void *sst, struct buffer_if *buf, @@ -1263,60 +1268,57 @@ static bool_t site_incoming(void *sst, struct buffer_if *buf, if (buf->size < 12) return False; + uint32_t msgtype=ntohl(get_uint32(buf->start+8)); uint32_t dest=ntohl(*(uint32_t *)buf->start); - - if (dest==0) { - /* It could be for any site - it should have LABEL_MSG1 and - might have our name and our peer's name in it */ - if (buf->size<(st->setupsiglen+8+NONCELEN)) return False; - if (memcmp(buf->start+8,st->setupsig,st->setupsiglen)==0) { - /* It's addressed to us. Decide what to do about it. */ - dump_packet(st,buf,source,True); - if (st->state==SITE_RUN || st->state==SITE_RESOLVE || - st->state==SITE_WAIT) { - /* We should definitely process it */ - if (process_msg1(st,buf,source)) { - slog(st,LOG_SETUP_INIT,"key setup initiated by peer"); + struct msg named_msg; + + if (msgtype==LABEL_MSG1) { + if (!named_for_us(st,buf,msgtype,&named_msg)) + return False; + /* It's a MSG1 addressed to us. Decide what to do about it. */ + dump_packet(st,buf,source,True); + if (st->state==SITE_RUN || st->state==SITE_RESOLVE || + st->state==SITE_WAIT) { + /* We should definitely process it */ + if (process_msg1(st,buf,source,&named_msg)) { + slog(st,LOG_SETUP_INIT,"key setup initiated by peer"); + enter_new_state(st,SITE_SENTMSG2); + } else { + slog(st,LOG_ERROR,"failed to process incoming msg1"); + } + BUF_FREE(buf); + return True; + } else if (st->state==SITE_SENTMSG1) { + /* We've just sent a message 1! They may have crossed on + the wire. If we have priority then we ignore the + incoming one, otherwise we process it as usual. */ + if (st->setup_priority) { + BUF_FREE(buf); + slog(st,LOG_DUMP,"crossed msg1s; we are higher " + "priority => ignore incoming msg1"); + return True; + } else { + slog(st,LOG_DUMP,"crossed msg1s; we are lower " + "priority => use incoming msg1"); + if (process_msg1(st,buf,source,&named_msg)) { + BUF_FREE(&st->buffer); /* Free our old message 1 */ enter_new_state(st,SITE_SENTMSG2); } else { - slog(st,LOG_ERROR,"failed to process incoming msg1"); + slog(st,LOG_ERROR,"failed to process an incoming " + "crossed msg1 (we have low priority)"); } BUF_FREE(buf); return True; - } else if (st->state==SITE_SENTMSG1) { - /* We've just sent a message 1! They may have crossed on - the wire. If we have priority then we ignore the - incoming one, otherwise we process it as usual. */ - if (st->setup_priority) { - BUF_FREE(buf); - slog(st,LOG_DUMP,"crossed msg1s; we are higher " - "priority => ignore incoming msg1"); - return True; - } else { - slog(st,LOG_DUMP,"crossed msg1s; we are lower " - "priority => use incoming msg1"); - if (process_msg1(st,buf,source)) { - BUF_FREE(&st->buffer); /* Free our old message 1 */ - enter_new_state(st,SITE_SENTMSG2); - } else { - slog(st,LOG_ERROR,"failed to process an incoming " - "crossed msg1 (we have low priority)"); - } - BUF_FREE(buf); - return True; - } } - /* The message 1 was received at an unexpected stage of the - key setup. XXX POLICY - what do we do? */ - slog(st,LOG_UNEXPECTED,"unexpected incoming message 1"); - BUF_FREE(buf); - return True; } - return False; /* Not for us. */ + /* The message 1 was received at an unexpected stage of the + key setup. XXX POLICY - what do we do? */ + slog(st,LOG_UNEXPECTED,"unexpected incoming message 1"); + BUF_FREE(buf); + return True; } if (dest==st->index) { /* Explicitly addressed to us */ - uint32_t msgtype=ntohl(get_uint32(buf->start+8)); if (msgtype!=LABEL_MSG0) dump_packet(st,buf,source,True); switch (msgtype) { case LABEL_NAK: @@ -1565,14 +1567,6 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context, /* The information we expect to see in incoming messages of type 1 */ /* fixme: lots of unchecked overflows here, but the results are only corrupted packets rather than undefined behaviour */ - st->setupsiglen=strlen(st->remotename)+strlen(st->localname)+8; - st->setupsig=safe_malloc(st->setupsiglen,"site_apply"); - put_uint32(st->setupsig+0,LABEL_MSG1); - put_uint16(st->setupsig+4,strlen(st->remotename)); - memcpy(&st->setupsig[6],st->remotename,strlen(st->remotename)); - put_uint16(st->setupsig+(6+strlen(st->remotename)),strlen(st->localname)); - memcpy(&st->setupsig[8+strlen(st->remotename)],st->localname, - strlen(st->localname)); st->setup_priority=(strcmp(st->localname,st->remotename)>0); buffer_new(&st->buffer,SETUP_BUFFER_LEN);