| Message ID | 1442271254-27897-6-git-send-email-azhou@nicira.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, Sep 14, 2015 at 3:54 PM, Andy Zhou <azhou@nicira.com> wrote: > All daemons launched by root can drop their privilege using s/privilege/privileges > --user option. See man page update form more details. s/form/for > > Signed-off-by: Andy Zhou <azhou@nicira.com> > --- > NEWS | 1 + > lib/daemon.man | 10 ++++++++++ > ovn/controller-vtep/ovn-controller-vtep.c | 1 + > ovn/northd/ovn-northd.c | 1 + > ovsdb/ovsdb-client.c | 4 +++- > ovsdb/ovsdb-server.c | 1 + > tests/test-jsonrpc.c | 3 ++- > tests/test-netflow.c | 4 ++-- > tests/test-sflow.c | 1 + > utilities/ovs-ofctl.c | 2 ++ > utilities/ovs-testcontroller.c | 4 +++- > vswitchd/ovs-vswitchd.c | 15 +++++++++++++++ > 12 files changed, 42 insertions(+), 5 deletions(-) > > diff --git a/NEWS b/NEWS > index ca22c8e..ee4dc2a 100644 > --- a/NEWS > +++ b/NEWS > @@ -21,6 +21,7 @@ Post-v2.4.0 > targets to run a new system testsuite. These tests can be run inside > a Vagrant box. See INSTALL.md for details > - Dropped support for GRE64 tunnel. > + - Added --user option to all daemons > > > v2.4.0 - 20 Aug 2015 > diff --git a/lib/daemon.man b/lib/daemon.man > index 4ab9823..093b08a 100644 > --- a/lib/daemon.man > +++ b/lib/daemon.man > @@ -50,3 +50,13 @@ core dumps into the current working directory and the root directory > is not a good directory to use. > .IP > This option has no effect when \fB\-\-detach\fR is not specified. > +. > +.TP > +\fB\-\-user\fR > +Causes \fB\*(PN\fR to run as a non root user specified in "user:group", thus > +dropping all root privileges. Short forms "user" and ":group" are also > +allowed, with current user or group are assumed respectively. Only daemons > +started by the root user accepts this argument. > +.IP > +On Linux, daemons need to access kernel datapath, such as ovs-vswitchd, > +will be granted CAP_NET_ADMIN capability before dropping root privileges. > diff --git a/ovn/controller-vtep/ovn-controller-vtep.c b/ovn/controller-vtep/ovn-controller-vtep.c > index b54b29d..013dc08 100644 > --- a/ovn/controller-vtep/ovn-controller-vtep.c > +++ b/ovn/controller-vtep/ovn-controller-vtep.c > @@ -63,6 +63,7 @@ main(int argc, char *argv[]) > parse_options(argc, argv); > fatal_ignore_sigpipe(); > > + daemon_become_new_user(false); > daemonize_start(); > > retval = unixctl_server_create(NULL, &unixctl); > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index 0f57e58..8e16e0d 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -1167,6 +1167,7 @@ main(int argc, char *argv[]) > vlog_set_levels(&VLM_reconnect, VLF_ANY_DESTINATION, VLL_WARN); > parse_options(argc, argv); > > + daemon_become_new_user(false); > daemonize_start(); > > retval = unixctl_server_create(NULL, &unixctl); > diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c > index 575cf91..8bf7ce7 100644 > --- a/ovsdb/ovsdb-client.c > +++ b/ovsdb/ovsdb-client.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc. > + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -95,6 +95,8 @@ main(int argc, char *argv[]) > ovs_fatal(0, "missing command name; use --help for help"); > } > > + daemon_become_new_user(false); > + > for (command = get_all_commands(); ; command++) { > if (!command->name) { > VLOG_FATAL("unknown command '%s'; use --help for help", > diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c > index 4088d85..91750a7 100644 > --- a/ovsdb/ovsdb-server.c > +++ b/ovsdb/ovsdb-server.c > @@ -221,6 +221,7 @@ main(int argc, char *argv[]) > process_init(); > > parse_options(&argc, &argv, &remotes, &unixctl_path, &run_command); > + daemon_become_new_user(false); > > /* Create and initialize 'config_tmpfile' as a temporary file to hold > * ovsdb-server's most basic configuration, and then save our initial > diff --git a/tests/test-jsonrpc.c b/tests/test-jsonrpc.c > index f66dc65..f1da341 100644 > --- a/tests/test-jsonrpc.c > +++ b/tests/test-jsonrpc.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc. > + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -182,6 +182,7 @@ do_listen(struct ovs_cmdl_context *ctx) > ovs_fatal(error, "could not listen on \"%s\"", ctx->argv[1]); > } > > + daemon_become_new_user(false); > daemonize(); > > rpcs = NULL; > diff --git a/tests/test-netflow.c b/tests/test-netflow.c > index 631d7a2..9b4fbc9 100644 > --- a/tests/test-netflow.c > +++ b/tests/test-netflow.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2011, 2012, 2013, 2014 Nicira, Inc. > + * Copyright (c) 2011, 2012, 2013, 2014, 2015 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -188,7 +188,7 @@ test_netflow_main(int argc, char *argv[]) > "(use --help for help)"); > } > target = argv[optind]; > - > + daemon_become_new_user(false); > sock = inet_open_passive(SOCK_DGRAM, target, 0, NULL, 0, true); > if (sock < 0) { > ovs_fatal(0, "%s: failed to open (%s)", argv[1], ovs_strerror(-sock)); > diff --git a/tests/test-sflow.c b/tests/test-sflow.c > index 6f90ebf..dd21af8 100644 > --- a/tests/test-sflow.c > +++ b/tests/test-sflow.c > @@ -677,6 +677,7 @@ test_sflow_main(int argc, char *argv[]) > } > target = argv[optind]; > > + daemon_become_new_user(false); > sock = inet_open_passive(SOCK_DGRAM, target, 0, NULL, 0, true); > if (sock < 0) { > ovs_fatal(0, "%s: failed to open (%s)", target, ovs_strerror(-sock)); > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c > index 75e84e2..f7e4acb 100644 > --- a/utilities/ovs-ofctl.c > +++ b/utilities/ovs-ofctl.c > @@ -130,6 +130,8 @@ main(int argc, char *argv[]) > fatal_ignore_sigpipe(); > ctx.argc = argc - optind; > ctx.argv = argv + optind; > + > + daemon_become_new_user(false); > ovs_cmdl_run_command(&ctx, get_all_commands()); > return 0; > } > diff --git a/utilities/ovs-testcontroller.c b/utilities/ovs-testcontroller.c > index 3d59adb..55942f0 100644 > --- a/utilities/ovs-testcontroller.c > +++ b/utilities/ovs-testcontroller.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. > + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2015 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -113,6 +113,8 @@ main(int argc, char *argv[]) > "use --help for usage"); > } > > + daemon_become_new_user(false); > + > n_switches = n_listeners = 0; > for (i = optind; i < argc; i++) { > const char *name = argv[i]; > diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c > index 4fe479e..5099806 100644 > --- a/vswitchd/ovs-vswitchd.c > +++ b/vswitchd/ovs-vswitchd.c > @@ -69,6 +69,7 @@ main(int argc, char *argv[]) > char *remote; > bool exiting; > int retval; > + bool access_kernel_datapath = false; > > set_program_name(argv[0]); > retval = dpdk_init(argc,argv); > @@ -82,6 +83,20 @@ main(int argc, char *argv[]) > ovs_cmdl_proctitle_init(argc, argv); > service_start(&argc, &argv); > remote = parse_options(argc, argv, &unixctl_path); > + /* Drop root privileges if --user option is specifed. > + * > + * On linux platforms that runs kernel datapath, we will s/platforms/platform or s/runs/run > + * drop all privileges, execpt CAP_NET_ADMIN capabilities. s/execpt/except > + * > + * The goal is to increase system security that in the event > + * ovs-vswitchd is somehow compromised, the process itself > + * does not have all root capabilities. > + */ > +#if __linux__ || defined _WIN32 > + access_kernel_datapath = true; Shouldn't you simply check for __linux__ here without || _WIN32? Perhaps I am simply missing something. > +#endif > + daemon_become_new_user(access_kernel_datapath); > + > fatal_ignore_sigpipe(); > ovsrec_init(); > > -- > 1.9.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Mon, Sep 14, 2015 at 03:54:09PM -0700, Andy Zhou wrote: > All daemons launched by root can drop their privilege using > --user option. See man page update form more details. > > Signed-off-by: Andy Zhou <azhou@nicira.com> What worries me about this is that it relies on developers to remember to add a call to daemon_become_new_user() to every daemon. If we forget one, it's a security hole: --user will be silently ignored. Is it possible to integrate daemon_become_new_user() into some other function that has to be called for daemonization to work? For example, can we integrate it into daemon_start()? Or can we at least avoid the security hole by, say, aborting in daemonize_complete() if daemon_become_new_user() hasn't been called?
On Fri, Sep 18, 2015 at 12:53 PM, Ben Pfaff <blp@nicira.com> wrote: > On Mon, Sep 14, 2015 at 03:54:09PM -0700, Andy Zhou wrote: >> All daemons launched by root can drop their privilege using >> --user option. See man page update form more details. >> >> Signed-off-by: Andy Zhou <azhou@nicira.com> > > What worries me about this is that it relies on developers to remember > to add a call to daemon_become_new_user() to every daemon. If we forget > one, it's a security hole: --user will be silently ignored. > > Is it possible to integrate daemon_become_new_user() into some other > function that has to be called for daemonization to work? For example, > can we integrate it into daemon_start()? Or can we at least avoid the > security hole by, say, aborting in daemonize_complete() if > daemon_become_new_user() hasn't been called? Integrating this function into daemon_start() seems to be an attractive option. Let me try to implement this. If not, I will fall back to abort in daemonize_complete().
diff --git a/NEWS b/NEWS index ca22c8e..ee4dc2a 100644 --- a/NEWS +++ b/NEWS @@ -21,6 +21,7 @@ Post-v2.4.0 targets to run a new system testsuite. These tests can be run inside a Vagrant box. See INSTALL.md for details - Dropped support for GRE64 tunnel. + - Added --user option to all daemons v2.4.0 - 20 Aug 2015 diff --git a/lib/daemon.man b/lib/daemon.man index 4ab9823..093b08a 100644 --- a/lib/daemon.man +++ b/lib/daemon.man @@ -50,3 +50,13 @@ core dumps into the current working directory and the root directory is not a good directory to use. .IP This option has no effect when \fB\-\-detach\fR is not specified. +. +.TP +\fB\-\-user\fR +Causes \fB\*(PN\fR to run as a non root user specified in "user:group", thus +dropping all root privileges. Short forms "user" and ":group" are also +allowed, with current user or group are assumed respectively. Only daemons +started by the root user accepts this argument. +.IP +On Linux, daemons need to access kernel datapath, such as ovs-vswitchd, +will be granted CAP_NET_ADMIN capability before dropping root privileges. diff --git a/ovn/controller-vtep/ovn-controller-vtep.c b/ovn/controller-vtep/ovn-controller-vtep.c index b54b29d..013dc08 100644 --- a/ovn/controller-vtep/ovn-controller-vtep.c +++ b/ovn/controller-vtep/ovn-controller-vtep.c @@ -63,6 +63,7 @@ main(int argc, char *argv[]) parse_options(argc, argv); fatal_ignore_sigpipe(); + daemon_become_new_user(false); daemonize_start(); retval = unixctl_server_create(NULL, &unixctl); diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 0f57e58..8e16e0d 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -1167,6 +1167,7 @@ main(int argc, char *argv[]) vlog_set_levels(&VLM_reconnect, VLF_ANY_DESTINATION, VLL_WARN); parse_options(argc, argv); + daemon_become_new_user(false); daemonize_start(); retval = unixctl_server_create(NULL, &unixctl); diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c index 575cf91..8bf7ce7 100644 --- a/ovsdb/ovsdb-client.c +++ b/ovsdb/ovsdb-client.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc. + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -95,6 +95,8 @@ main(int argc, char *argv[]) ovs_fatal(0, "missing command name; use --help for help"); } + daemon_become_new_user(false); + for (command = get_all_commands(); ; command++) { if (!command->name) { VLOG_FATAL("unknown command '%s'; use --help for help", diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c index 4088d85..91750a7 100644 --- a/ovsdb/ovsdb-server.c +++ b/ovsdb/ovsdb-server.c @@ -221,6 +221,7 @@ main(int argc, char *argv[]) process_init(); parse_options(&argc, &argv, &remotes, &unixctl_path, &run_command); + daemon_become_new_user(false); /* Create and initialize 'config_tmpfile' as a temporary file to hold * ovsdb-server's most basic configuration, and then save our initial diff --git a/tests/test-jsonrpc.c b/tests/test-jsonrpc.c index f66dc65..f1da341 100644 --- a/tests/test-jsonrpc.c +++ b/tests/test-jsonrpc.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc. + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -182,6 +182,7 @@ do_listen(struct ovs_cmdl_context *ctx) ovs_fatal(error, "could not listen on \"%s\"", ctx->argv[1]); } + daemon_become_new_user(false); daemonize(); rpcs = NULL; diff --git a/tests/test-netflow.c b/tests/test-netflow.c index 631d7a2..9b4fbc9 100644 --- a/tests/test-netflow.c +++ b/tests/test-netflow.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011, 2012, 2013, 2014 Nicira, Inc. + * Copyright (c) 2011, 2012, 2013, 2014, 2015 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -188,7 +188,7 @@ test_netflow_main(int argc, char *argv[]) "(use --help for help)"); } target = argv[optind]; - + daemon_become_new_user(false); sock = inet_open_passive(SOCK_DGRAM, target, 0, NULL, 0, true); if (sock < 0) { ovs_fatal(0, "%s: failed to open (%s)", argv[1], ovs_strerror(-sock)); diff --git a/tests/test-sflow.c b/tests/test-sflow.c index 6f90ebf..dd21af8 100644 --- a/tests/test-sflow.c +++ b/tests/test-sflow.c @@ -677,6 +677,7 @@ test_sflow_main(int argc, char *argv[]) } target = argv[optind]; + daemon_become_new_user(false); sock = inet_open_passive(SOCK_DGRAM, target, 0, NULL, 0, true); if (sock < 0) { ovs_fatal(0, "%s: failed to open (%s)", target, ovs_strerror(-sock)); diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index 75e84e2..f7e4acb 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -130,6 +130,8 @@ main(int argc, char *argv[]) fatal_ignore_sigpipe(); ctx.argc = argc - optind; ctx.argv = argv + optind; + + daemon_become_new_user(false); ovs_cmdl_run_command(&ctx, get_all_commands()); return 0; } diff --git a/utilities/ovs-testcontroller.c b/utilities/ovs-testcontroller.c index 3d59adb..55942f0 100644 --- a/utilities/ovs-testcontroller.c +++ b/utilities/ovs-testcontroller.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2015 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -113,6 +113,8 @@ main(int argc, char *argv[]) "use --help for usage"); } + daemon_become_new_user(false); + n_switches = n_listeners = 0; for (i = optind; i < argc; i++) { const char *name = argv[i]; diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c index 4fe479e..5099806 100644 --- a/vswitchd/ovs-vswitchd.c +++ b/vswitchd/ovs-vswitchd.c @@ -69,6 +69,7 @@ main(int argc, char *argv[]) char *remote; bool exiting; int retval; + bool access_kernel_datapath = false; set_program_name(argv[0]); retval = dpdk_init(argc,argv); @@ -82,6 +83,20 @@ main(int argc, char *argv[]) ovs_cmdl_proctitle_init(argc, argv); service_start(&argc, &argv); remote = parse_options(argc, argv, &unixctl_path); + /* Drop root privileges if --user option is specifed. + * + * On linux platforms that runs kernel datapath, we will + * drop all privileges, execpt CAP_NET_ADMIN capabilities. + * + * The goal is to increase system security that in the event + * ovs-vswitchd is somehow compromised, the process itself + * does not have all root capabilities. + */ +#if __linux__ || defined _WIN32 + access_kernel_datapath = true; +#endif + daemon_become_new_user(access_kernel_datapath); + fatal_ignore_sigpipe(); ovsrec_init();
All daemons launched by root can drop their privilege using --user option. See man page update form more details. Signed-off-by: Andy Zhou <azhou@nicira.com> --- NEWS | 1 + lib/daemon.man | 10 ++++++++++ ovn/controller-vtep/ovn-controller-vtep.c | 1 + ovn/northd/ovn-northd.c | 1 + ovsdb/ovsdb-client.c | 4 +++- ovsdb/ovsdb-server.c | 1 + tests/test-jsonrpc.c | 3 ++- tests/test-netflow.c | 4 ++-- tests/test-sflow.c | 1 + utilities/ovs-ofctl.c | 2 ++ utilities/ovs-testcontroller.c | 4 +++- vswitchd/ovs-vswitchd.c | 15 +++++++++++++++ 12 files changed, 42 insertions(+), 5 deletions(-)