From 3abd18e85781e00e2b7fc641f29c99e130238abf Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 25 Jul 2013 18:30:53 +0100 Subject: [PATCH] max_start_pad: calculate globally, not via client graph There is quite a lot of plumbing of max_start_pad values from one place to another. Sadly this plumbing is not complete, which can lead to crashes as the start padding is exhausted. And completing it is a lot of work which would be difficult to get correct, even if that's possible in principle. Instead, we take a different approach. We calculate a single global max_pad_start value that can be used everywhere. It is the sum of: * Headers that "site" instances might need add to the start of packets (source and destination site indices, message type both inside and outside the encryption; * Anything that "transform" instances might need to add to the start of packets. This depends on the transforms, but since it isn't a priori predictable which path any particular incoming packet might take, we have to take the worst case. These transform instances are applied only by "site" and each packet goes through at most on "forward" transform instance. * Anything that "comm" instances might need to add. This is currently only needed for the proxy feature. This is based on the assumption that a packet may follow one of these paths: comm -->- site_incoming --. ,-- site_outgoing -->- comm \ / netlink / \ tun/slip ----->-----------' `------>---------- tun/slip On the inbound side, tun and slip have to set up the buffer with the necessary start padding. site_incoming only removes stuff from the beginning (since it uses transform->reverse). netlink doesn't add or remove anything. It is site_outgoing and comm which may need to prepend things. site additionally calls transform->forwards. If in the future a module appears which can take packets out of the RHS of this diagram and feed them back into the left, it may have to do something about the buffer. Signed-off-by: Ian Jackson --- netlink.c | 7 ++----- netlink.h | 1 - secnet.h | 20 +++++++++++++------- site.c | 23 ++++++----------------- slip.c | 4 ++-- transform-cbcmac.c | 4 ++-- transform-common.h | 1 - transform-eax.c | 2 +- tun.c | 4 ++-- udp.c | 7 ++++--- util.c | 18 +++++++++++++++++- 11 files changed, 49 insertions(+), 42 deletions(-) diff --git a/netlink.c b/netlink.c index 5d586d0..173d412 100644 --- a/netlink.c +++ b/netlink.c @@ -238,7 +238,7 @@ static struct icmphdr *netlink_icmp_tmpl(struct netlink *st, struct icmphdr *h; BUF_ALLOC(&st->icmp,"netlink_icmp_tmpl"); - buffer_init(&st->icmp,st->max_start_pad); + buffer_init(&st->icmp,calculate_max_start_pad()); h=buf_append(&st->icmp,sizeof(*h)); h->iph.version=4; @@ -808,12 +808,10 @@ static void netlink_inst_set_mtu(void *sst, int32_t new_mtu) } static void netlink_inst_reg(void *sst, netlink_deliver_fn *deliver, - void *dst, int32_t max_start_pad) + void *dst) { struct netlink_client *c=sst; - struct netlink *st=c->nst; - if (max_start_pad > st->max_start_pad) st->max_start_pad=max_start_pad; c->deliver=deliver; c->dst=dst; } @@ -941,7 +939,6 @@ netlink_deliver_fn *netlink_init(struct netlink *st, st->cl.type=CL_PURE; st->cl.apply=netlink_inst_apply; st->cl.interface=st; - st->max_start_pad=0; st->clients=NULL; st->routes=NULL; st->n_clients=0; diff --git a/netlink.h b/netlink.h index 5e6ad49..7c42716 100644 --- a/netlink.h +++ b/netlink.h @@ -45,7 +45,6 @@ struct netlink { closure_t cl; void *dst; /* Pointer to host interface state */ cstring_t name; - int32_t max_start_pad; struct ipset *networks; /* Local networks */ struct subnet_list *subnets; /* Same as networks, for display */ struct ipset *remote_networks; /* Allowable remote networks */ diff --git a/secnet.h b/secnet.h index 98a3ae3..9bec310 100644 --- a/secnet.h +++ b/secnet.h @@ -146,6 +146,16 @@ static const struct timeval *const tv_now = &tv_now_global; /***** END of utility functions *****/ +/***** START of max_start_pad handling *****/ + +extern int32_t site_max_start_pad, transform_max_start_pad, + comm_max_start_pad; + +void update_max_start_pad(int32_t *our_module_global, int32_t our_instance); +int32_t calculate_max_start_pad(void); + +/***** END of max_start_pad handling *****/ + /***** SCHEDULING support */ /* If nfds_io is insufficient for your needs, set it to the required @@ -325,7 +335,6 @@ typedef const char *comm_addr_to_string_fn(void *commst, /* Returned string is in a static buffer. */ struct comm_if { void *st; - int32_t min_start_pad; comm_request_notify_fn *request_notify; comm_release_notify_fn *release_notify; comm_sendmsg_fn *sendmsg; @@ -363,8 +372,7 @@ struct site_if { /* TRANSFORM interface */ /* A reversable transformation. Transforms buffer in-place; may add - data to start or end. Maximum amount of data to be added before - the packet specified in max_start_pad. (Reverse transformations decrease + data to start or end. (Reverse transformations decrease length, of course.) Transformations may be key-dependent, in which case key material is passed in at initialisation time. They may also depend on internal factors (eg. time) and keep internal @@ -396,14 +404,12 @@ struct transform_inst_if { transform_apply_fn *forwards; transform_apply_fn *reverse; transform_destroyinstance_fn *destroy; - int32_t max_start_pad; /* same as from transform_if */ }; struct transform_if { void *st; - int32_t max_start_pad; /* these two are both <<< INT_MAX */ - int32_t keylen; /* 0 means give the transform exactly as much as there is */ int capab_transformnum; + int32_t keylen; /* <<< INT_MAX */ transform_createinstance_fn *create; }; @@ -425,7 +431,7 @@ typedef void netlink_deliver_fn(void *st, struct buffer_if *buf); #define MAXIMUM_LINK_QUALITY 3 typedef void netlink_link_quality_fn(void *st, uint32_t quality); typedef void netlink_register_fn(void *st, netlink_deliver_fn *deliver, - void *dst, int32_t max_start_pad); + void *dst); typedef void netlink_output_config_fn(void *st, struct buffer_if *buf); typedef bool_t netlink_check_config_fn(void *st, struct buffer_if *buf); typedef void netlink_set_mtu_fn(void *st, int32_t new_mtu); diff --git a/site.c b/site.c index 20f9507..909e8e9 100644 --- a/site.c +++ b/site.c @@ -89,6 +89,8 @@ #define SITE_SENTMSG5 7 #define SITE_WAIT 8 +int32_t site_max_start_pad = 4*4; + static cstring_t state_name(uint32_t state) { switch (state) { @@ -826,7 +828,7 @@ static bool_t generate_msg5(struct site *st) BUF_ALLOC(&st->buffer,"site:MSG5"); /* We are going to add four words to the message */ - buffer_init(&st->buffer,st->new_transform->max_start_pad+(4*4)); + buffer_init(&st->buffer,calculate_max_start_pad()); /* Give the netlink code an opportunity to put its own stuff in the message (configuration information, etc.) */ buf_prepend_uint32(&st->buffer,LABEL_MSG5); @@ -873,7 +875,7 @@ static void create_msg6(struct site *st, struct transform_inst_if *transform, BUF_ALLOC(&st->buffer,"site:MSG6"); /* We are going to add four words to the message */ - buffer_init(&st->buffer,transform->max_start_pad+(4*4)); + buffer_init(&st->buffer,calculate_max_start_pad()); /* Give the netlink code an opportunity to put its own stuff in the message (configuration information, etc.) */ buf_prepend_uint32(&st->buffer,LABEL_MSG6); @@ -1294,7 +1296,7 @@ static bool_t send_msg7(struct site *st, cstring_t reason) if (current_valid(st) && st->buffer.free && transport_peers_valid(&st->peers)) { BUF_ALLOC(&st->buffer,"site:MSG7"); - buffer_init(&st->buffer,st->current.transform->max_start_pad+(4*3)); + buffer_init(&st->buffer,calculate_max_start_pad()); buf_append_uint32(&st->buffer,LABEL_MSG7); buf_append_string(&st->buffer,reason); if (call_transform_forwards(st, st->current.transform, @@ -1770,17 +1772,6 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context, st->sharedsecretlen=st->sharedsecretallocd=0; st->sharedsecret=0; - /* We need to compute some properties of our comms and transports */ -#define COMPUTE_WORST(things,pad) \ - int things##_worst_##pad=0; \ - for (i=0; in##things; i++) { \ - int thispad=st->things[i]->pad; \ - if (thispad > things##_worst_##pad) \ - things##_worst_##pad=thispad; \ - } - COMPUTE_WORST(comms,min_start_pad) - COMPUTE_WORST(transforms,max_start_pad) - for (i=0; intransforms; i++) { struct transform_if *ti=st->transforms[i]; uint32_t capbit = 1UL << ti->capab_transformnum; @@ -1791,9 +1782,7 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context, } /* We need to register the remote networks with the netlink device */ - st->netlink->reg(st->netlink->st, site_outgoing, st, - transforms_worst_max_start_pad+(4*4)+ - comms_worst_min_start_pad); + st->netlink->reg(st->netlink->st, site_outgoing, st); for (i=0; incomms; i++) st->comms[i]->request_notify(st->comms[i]->st, st, site_incoming); diff --git a/slip.c b/slip.c index 0f62a69..5eb8dbd 100644 --- a/slip.c +++ b/slip.c @@ -115,7 +115,7 @@ static void slip_unstuff(struct slip *st, uint8_t *buf, uint32_t l) if (st->ignoring_packet) { if (outputchr == OUTPUT_END) { st->ignoring_packet=False; - buffer_init(st->buff,st->nl.max_start_pad); + buffer_init(st->buff,calculate_max_start_pad()); } } else { if (outputchr == OUTPUT_END) { @@ -123,7 +123,7 @@ static void slip_unstuff(struct slip *st, uint8_t *buf, uint32_t l) st->netlink_to_tunnel(&st->nl,st->buff); BUF_ALLOC(st->buff,"userv_afterpoll"); } - buffer_init(st->buff,st->nl.max_start_pad); + buffer_init(st->buff,calculate_max_start_pad()); } else if (outputchr != OUTPUT_NOTHING) { if (st->buff->size < st->buff->len) { buf_append_uint8(st->buff,outputchr); diff --git a/transform-cbcmac.c b/transform-cbcmac.c index 95c64e8..26e0a12 100644 --- a/transform-cbcmac.c +++ b/transform-cbcmac.c @@ -261,8 +261,8 @@ static list_t *transform_apply(closure_t *self, struct cloc loc, st->cl.apply=NULL; st->cl.interface=&st->ops; st->ops.st=st; - st->ops.max_start_pad=28; /* 4byte seqnum, 16byte pad, 4byte MACIV, - 4byte IV */ + update_max_start_pad(&transform_max_start_pad, 28); + /* 4byte seqnum, 16byte pad, 4byte MACIV, 4byte IV */ /* We need 256*2 bits for serpent keys, 32 bits for CBC-IV and 32 bits for CBCMAC-IV, and 32 bits for init sequence number */ diff --git a/transform-common.h b/transform-common.h index 52f6067..24ab8dc 100644 --- a/transform-common.h +++ b/transform-common.h @@ -61,7 +61,6 @@ ti->ops.forwards=transform_forward; \ ti->ops.reverse=transform_reverse; \ ti->ops.destroy=transform_destroy; \ - ti->ops.max_start_pad=st->ops.max_start_pad; \ ti->keyed=False; #endif /*TRANSFORM_COMMON_H*/ diff --git a/transform-eax.c b/transform-eax.c index 31d8171..f881abb 100644 --- a/transform-eax.c +++ b/transform-eax.c @@ -294,7 +294,7 @@ static list_t *transform_apply(closure_t *self, struct cloc loc, padding_round = 1; st->p.padding_mask = padding_round-1; - st->ops.max_start_pad=0; + update_max_start_pad(&transform_max_start_pad, 0); st->ops.keylen=0; st->ops.create=transform_create; diff --git a/tun.c b/tun.c index b0bde38..40bf6dd 100644 --- a/tun.c +++ b/tun.c @@ -116,8 +116,8 @@ static void tun_afterpoll(void *sst, struct pollfd *fds, int nfds) } if (fds[0].revents&POLLIN) { BUF_ALLOC(st->buff,"tun_afterpoll"); - buffer_init(st->buff,st->nl.max_start_pad); - l=read(st->fd,st->buff->start,st->buff->len-st->nl.max_start_pad); + buffer_init(st->buff,calculate_max_start_pad()); + l=read(st->fd,st->buff->start,st->buff->len-calculate_max_start_pad()); if (l<0) { fatal_perror("tun_afterpoll: read()"); } diff --git a/udp.c b/udp.c index 12fcfe8..42fbb1f 100644 --- a/udp.c +++ b/udp.c @@ -193,12 +193,13 @@ static bool_t udp_sendmsg(void *commst, struct buffer_if *buf, uint8_t *sa; if (st->use_proxy) { - sa=buf->start-8; + sa=buf_prepend(buf,8); memcpy(sa,&dest->sin.sin_addr,4); memset(sa+4,0,4); memcpy(sa+6,&dest->sin.sin_port,2); sendto(st->fd,sa,buf->size+8,0,(struct sockaddr *)&st->proxy, sizeof(st->proxy)); + buf_unprepend(buf,8); } else { sendto(st->fd, buf->start, buf->size, 0, (struct sockaddr *)&dest->sin, sizeof(dest->sin)); @@ -288,7 +289,6 @@ static list_t *udp_apply(closure_t *self, struct cloc loc, dict_t *context, st->cl.apply=NULL; st->cl.interface=&st->ops; st->ops.st=st; - st->ops.min_start_pad=0; st->ops.request_notify=request_notify; st->ops.release_notify=release_notify; st->ops.sendmsg=udp_sendmsg; @@ -323,9 +323,10 @@ static list_t *udp_apply(closure_t *self, struct cloc loc, dict_t *context, cfgfatal(st->loc,"udp","proxy must supply ""addr"",port\n"); } st->proxy.sin_port=htons(i->data.number); - st->ops.min_start_pad=8; } + update_max_start_pad(&comm_max_start_pad, st->use_proxy ? 8 : 0); + add_hook(PHASE_GETRESOURCES,udp_phase_hook,st); return new_closure(&st->cl); diff --git a/util.c b/util.c index 24dd9a3..8c23485 100644 --- a/util.c +++ b/util.c @@ -387,7 +387,7 @@ void send_nak(const struct comm_addr *dest, uint32_t our_index, uint32_t their_index, uint32_t msgtype, struct buffer_if *buf, const char *logwhy) { - buffer_init(buf,dest->comm->min_start_pad); + buffer_init(buf,calculate_max_start_pad()); buf_append_uint32(buf,their_index); buf_append_uint32(buf,our_index); buf_append_uint32(buf,LABEL_NAK); @@ -419,3 +419,19 @@ void util_module(dict_t *dict) { add_closure(dict,"sysbuffer",buffer_apply); } + +void update_max_start_pad(int32_t *our_module_global, int32_t our_instance) +{ + if (*our_module_global < our_instance) + *our_module_global=our_instance; +} + +int32_t transform_max_start_pad, comm_max_start_pad; + +int32_t calculate_max_start_pad(void) +{ + return + site_max_start_pad + + transform_max_start_pad + + comm_max_start_pad; +} -- 2.30.2