From 32654a316b5925914f5e5f481971d47f9728edab Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 1 Oct 2014 23:21:56 +0100 Subject: [PATCH] fds etc.: Support non-forking persistent children Polypath is are going to want to spawn a persistent child process, which will not exec. This child ought not to hold onto the various important fds. Otherwise, if the main secnet process dies but the child does not (for some reason), the network interfaces, udp sockets, etc., set up by the old secnet will remain owned by the child. Introduce a new PHASE for this purpose (currently never entered). Provide a convenient common hook function for closing a single fd. Add phase hooks to: * Close udp sockets (in the udp and polypath comm modules); * Close the pipes to userv-ipif (slip netlink module); * Close the tun device (tun netlink module); * Zero out data transport keys, to improve forward secrecy in case the subprocess leaks somehow. (Sadly we can't conveniently find the asymmmetric crypto session key negotiation state to wipe it.) Signed-off-by: Ian Jackson --- comm-common.h | 1 + polypath.c | 10 ++++++++++ process.c | 10 ++++++++++ secnet.h | 14 ++++++++++++++ site.c | 18 ++++++++++++++++++ slip.c | 3 +++ tun.c | 2 ++ udp.c | 15 +++++++++++++++ util.c | 3 ++- 9 files changed, 75 insertions(+), 1 deletion(-) diff --git a/comm-common.h b/comm-common.h index 13709c7..aa757e1 100644 --- a/comm-common.h +++ b/comm-common.h @@ -102,6 +102,7 @@ void udp_sock_experienced(struct log_if *lg, struct udpcommon *uc, void udp_socks_register(struct udpcommon *uc, struct udpsocks *socks); void udp_socks_deregister(struct udpcommon *uc, struct udpsocks *socks); +void udp_socks_childpersist(struct udpcommon *uc, struct udpsocks *socks); #define UDP_APPLY_STANDARD(st,uc,desc) \ (uc)->use_proxy=False; \ diff --git a/polypath.c b/polypath.c index 58a89ee..b867632 100644 --- a/polypath.c +++ b/polypath.c @@ -506,6 +506,15 @@ static void polypath_phase_shutdown(void *sst, uint32_t newphase) } } +static void polypath_phase_childpersist(void *sst, uint32_t newphase) +{ + struct polypath *st=sst; + struct interf *interf; + + LIST_FOREACH(interf,&st->interfs,entry) + udp_socks_childpersist(&st->uc,&interf->socks); +} + #undef BAD #undef BADE @@ -546,6 +555,7 @@ static list_t *polypath_apply(closure_t *self, struct cloc loc, add_hook(PHASE_RUN, polypath_phase_startmonitor,st); add_hook(PHASE_SHUTDOWN, polypath_phase_shutdown, st); + add_hook(PHASE_CHILDPERSIST,polypath_phase_childpersist,st); return new_closure(&cc->cl); } diff --git a/process.c b/process.c index 77fe38e..c14dd69 100644 --- a/process.c +++ b/process.c @@ -236,6 +236,16 @@ void afterfork(void) sigprocmask(SIG_SETMASK,&emptyset,NULL); } +void childpersist_closefd_hook(void *fd_vp, uint32_t newphase) +{ + int *fd_p=fd_vp; + int fd=*fd_p; + if (fd<0) return; + *fd_p=-1; + setnonblock(fd); /* in case close() might block */ + close(fd); /* discard errors - we don't care, in the child */ +} + static void signal_handler(int signum) { int saved_errno; diff --git a/secnet.h b/secnet.h index 76db603..eb250a4 100644 --- a/secnet.h +++ b/secnet.h @@ -64,6 +64,12 @@ void afterfork(void); /* Must be called before exec in every child made after start_signal_handling. Safe to call in earlier children too. */ +void childpersist_closefd_hook(void *fd_p, uint32_t newphase); +/* Convenience hook function for use with add_hook PHASE_CHILDPERSIST. + With `int fd' in your state struct, pass fd_p=&fd. The hook checks + whether fd>=0, so you can use it for an fd which is only sometimes + open. This function will set fd to -1, so it is idempotent. */ + /***** CONFIGURATION support *****/ extern bool_t just_check_config; /* If True then we're going to exit after @@ -258,10 +264,18 @@ enum phase { PHASE_DROPPRIV, /* Last chance for privileged operations */ PHASE_RUN, PHASE_SHUTDOWN, /* About to die; delete key material, etc. */ + PHASE_CHILDPERSIST, /* Forked long-term child: close fds, etc. */ /* Keep this last: */ NR_PHASES, }; +/* Each module should, in its CHILDPERSIST hooks, close all fds which + constitute ownership of important operating system resources, or + which are used for IPC with other processes who want to get the + usual disconnection effects if the main secnet process dies. + CHILDPERSIST hooks are not run if the child is going to exec; + so fds such as described above should be CLOEXEC too. */ + typedef void hook_fn(void *self, uint32_t newphase); bool_t add_hook(uint32_t phase, hook_fn *f, void *state); bool_t remove_hook(uint32_t phase, hook_fn *f, void *state); diff --git a/site.c b/site.c index 935bf38..1c57a8a 100644 --- a/site.c +++ b/site.c @@ -1901,6 +1901,23 @@ static void site_phase_hook(void *sst, uint32_t newphase) send_msg7(st,"shutting down"); } +static void site_childpersist_clearkeys(void *sst, uint32_t newphase) +{ + struct site *st=sst; + dispose_transform(&st->current.transform); + dispose_transform(&st->auxiliary_key.transform); + dispose_transform(&st->new_transform); + /* Not much point overwiting the signing key, since we loaded it + from disk, and it is only valid prospectively if at all, + anyway. */ + /* XXX it would be best to overwrite the DH state, because that + _is_ relevant to forward secrecy. However we have no + convenient interface for doing that and in practice gmp has + probably dribbled droppings all over the malloc arena. A good + way to fix this would be to have a privsep child for asymmetric + crypto operations, but that's a task for another day. */ +} + static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context, list_t *args) { @@ -2090,6 +2107,7 @@ static list_t *site_apply(closure_t *self, struct cloc loc, dict_t *context, enter_state_stop(st); add_hook(PHASE_SHUTDOWN,site_phase_hook,st); + add_hook(PHASE_CHILDPERSIST,site_childpersist_clearkeys,st); return new_closure(&st->cl); } diff --git a/slip.c b/slip.c index e555f47..fad0a7e 100644 --- a/slip.c +++ b/slip.c @@ -383,6 +383,9 @@ static void userv_invoke_userv(struct userv *st) } setnonblock(st->txfd); setnonblock(st->rxfd); + + add_hook(PHASE_CHILDPERSIST,childpersist_closefd_hook,&st->txfd); + add_hook(PHASE_CHILDPERSIST,childpersist_closefd_hook,&st->rxfd); } static void userv_kill_userv(struct userv *st) diff --git a/tun.c b/tun.c index 2c84ed0..b3c69a2 100644 --- a/tun.c +++ b/tun.c @@ -442,6 +442,8 @@ static void tun_phase_hook(void *sst, uint32_t newphase) tun_set_route(st,r); } + add_hook(PHASE_CHILDPERSIST,childpersist_closefd_hook,&st->fd); + /* Register for poll() */ register_for_poll(st, tun_beforepoll, tun_afterpoll, st->nl.name); } diff --git a/udp.c b/udp.c index 706e077..e62f5c1 100644 --- a/udp.c +++ b/udp.c @@ -340,6 +340,19 @@ void udp_socks_deregister(struct udpcommon *uc, struct udpsocks *socks) deregister_for_poll(socks->interest); } +void udp_socks_childpersist(struct udpcommon *uc, struct udpsocks *socks) +{ + int i; + for (i=0; in_socks; i++) + udp_destroy_socket(uc,&socks->socks[i]); +} + +static void udp_childpersist_hook(void *sst, uint32_t new_phase) +{ + struct udp *st=sst; + udp_socks_childpersist(&st->uc,&st->socks); +} + static void udp_phase_hook(void *sst, uint32_t new_phase) { struct udp *st=sst; @@ -350,6 +363,8 @@ static void udp_phase_hook(void *sst, uint32_t new_phase) udp_make_socket(uc,&socks->socks[i],M_FATAL); udp_socks_register(uc,socks); + + add_hook(PHASE_CHILDPERSIST,udp_childpersist_hook,st); } static list_t *udp_apply(closure_t *self, struct cloc loc, dict_t *context, diff --git a/util.c b/util.c index 12e5208..d843859 100644 --- a/util.c +++ b/util.c @@ -204,7 +204,8 @@ static const char *phases[NR_PHASES]={ "PHASE_GETRESOURCES", "PHASE_DROPPRIV", "PHASE_RUN", - "PHASE_SHUTDOWN" + "PHASE_SHUTDOWN", + "PHASE_CHILDPERSIST" }; void enter_phase(uint32_t new_phase) -- 2.30.2