| Message ID | 20220527044700.3666830-6-dominique.martinet@atmark-techno.com |
|---|---|
| State | Rejected |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | Avoid leaking fd to child processes: use CLOEXEC | expand |
Hi Dominique, On 27.05.22 06:46, Dominique Martinet wrote: > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > So this is the most controvertial one: What if some application uses > prepare_ipc() and expects the fd to be preserved throught an exec? > This is already a use case, because the fd is exposed to Lua and then everybody can do whatever it wants. I have also no way to check what users are doing with the fd, and if they exec / fork / whatever. Someone can even do require ("swupdate") conn = connect() And then os.execute() This cannot be merged. > In my case this was leaked for -i through the async thread, so these fd > are internal to that thread and have no business leaking out, but it's > not necessarily general. > > swupdate_async_start and ipc_wait_for_complete() could mark the fd > received from ipc_inst_start_ext/prepare_ipc as CLOEXEC, but that > doesn't feel pretty... > > I'd personally think it's better to break the behaviour here, but > ultimately this is just cleanup and I don't really care. > We can: > - leave it as is with fd leaked, Yes > - add a couple of fcntl for callers we're sure about (e.g. fd doesn't > leave the function; for example swupdate_async_start provides a > callback that could theorically fork/exec if they really wanted to so if > we want to be safe we shouldn't change it, but ipc_wait_for_complete > should be safe). > That adds complexity and doesn't cover all cases so it's probably not > worth doing. > - add a new prepare_ipc_cloexec and use it when we want to, but that > has the same drawback as above that we couldn't use it for async start. > - Do this > > I'll leave that one to you... > > ipc/network_ipc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/ipc/network_ipc.c b/ipc/network_ipc.c > index 6c8c03ae4617..429c556ddb85 100644 > --- a/ipc/network_ipc.c > +++ b/ipc/network_ipc.c > @@ -11,6 +11,7 @@ > #include <string.h> > #include <strings.h> > #include <unistd.h> > +#include <fcntl.h> > #include <errno.h> > #include <sys/socket.h> > #include <sys/select.h> > @@ -47,6 +48,8 @@ static int prepare_ipc(void) { > if (connfd < 0) > return -1; > > + fcntl(connfd, F_SETFD, FD_CLOEXEC); > + > bzero(&servaddr, sizeof(servaddr)); > servaddr.sun_family = AF_LOCAL; > Best regards, Stefano
Stefano Babic wrote on Sun, Jun 05, 2022 at 04:21:50PM +0200: > This is already a use case, because the fd is exposed to Lua and then > everybody can do whatever it wants. I have also no way to check what users > are doing with the fd, and if they exec / fork / whatever. > > Someone can even do > > require ("swupdate") > > conn = connect() > > And then os.execute() > > This cannot be merged. Ok, let's keep this one out. Thanks for merging the other patches, I'll look at the coverity defect you mentioned in the other thread.
diff --git a/ipc/network_ipc.c b/ipc/network_ipc.c index 6c8c03ae4617..429c556ddb85 100644 --- a/ipc/network_ipc.c +++ b/ipc/network_ipc.c @@ -11,6 +11,7 @@ #include <string.h> #include <strings.h> #include <unistd.h> +#include <fcntl.h> #include <errno.h> #include <sys/socket.h> #include <sys/select.h> @@ -47,6 +48,8 @@ static int prepare_ipc(void) { if (connfd < 0) return -1; + fcntl(connfd, F_SETFD, FD_CLOEXEC); + bzero(&servaddr, sizeof(servaddr)); servaddr.sun_family = AF_LOCAL;
Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- So this is the most controvertial one: What if some application uses prepare_ipc() and expects the fd to be preserved throught an exec? In my case this was leaked for -i through the async thread, so these fd are internal to that thread and have no business leaking out, but it's not necessarily general. swupdate_async_start and ipc_wait_for_complete() could mark the fd received from ipc_inst_start_ext/prepare_ipc as CLOEXEC, but that doesn't feel pretty... I'd personally think it's better to break the behaviour here, but ultimately this is just cleanup and I don't really care. We can: - leave it as is with fd leaked, - add a couple of fcntl for callers we're sure about (e.g. fd doesn't leave the function; for example swupdate_async_start provides a callback that could theorically fork/exec if they really wanted to so if we want to be safe we shouldn't change it, but ipc_wait_for_complete should be safe). That adds complexity and doesn't cover all cases so it's probably not worth doing. - add a new prepare_ipc_cloexec and use it when we want to, but that has the same drawback as above that we couldn't use it for async start. - Do this I'll leave that one to you... ipc/network_ipc.c | 3 +++ 1 file changed, 3 insertions(+)