[PATCH 09/25] site: Extra info in name fields for MSG1, clearer processing
Ian Jackson
ijackson at chiark.greenend.org.uk
Sat Jul 20 00:38:53 BST 2013
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 <ijackson at chiark.greenend.org.uk>
---
NOTES | 5 ++-
site.c | 114 +++++++++++++++++++++++++++++----------------------------------
2 files changed, 57 insertions(+), 62 deletions(-)
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 cd053e2..7857956 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;
@@ -505,19 +502,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;
}
@@ -1245,6 +1238,16 @@ 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, struct buffer_if *buf,
+ 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. */
+{
+ 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,
@@ -1255,60 +1258,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:
@@ -1557,14 +1557,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);
--
1.7.2.5
More information about the sgo-software-discuss
mailing list