Thank you! Comments in-line below. (I'll skip repeating David's
suggestions.)
On 2015-06-04 14:37, Matt Carr wrote:
> From: M Carr <mc...@bbn.com>
>
> ---
> bin/rpki-rtr/main.c | 28 ++++++++++++++++++++++++++++
> etc/rpstir.conf.in | 3 +++
> lib/config/config.c | 10 ++++++++++
> lib/config/config.h | 2 ++
> 4 files changed, 43 insertions(+)
>
> diff --git a/bin/rpki-rtr/main.c b/bin/rpki-rtr/main.c
> index 63c8baf..dcab66c 100644
> --- a/bin/rpki-rtr/main.c
> +++ b/bin/rpki-rtr/main.c
> @@ -11,6 +11,7 @@
> #include <stdint.h>
> #include <stdlib.h>
> #include <netdb.h>
> +#include <pwd.h>
>
> #include "util/bag.h"
> #include "util/queue.h"
> @@ -490,6 +491,33 @@ static void startup(
> pthread_exit(NULL);
> }
>
> + // Drop privileges (if run as root)
> + const char *user = CONFIG_RPSTIR_USER_get();
> + struct passwd *pwd;
> + pwd = getpwnam(user);
> + if (pwd == NULL)
> + {
> + LOG(LOG_ERR, "can't find rpstir user: %s", user);
Before erroring out, check to see if the string can be interpreted as an
integer (to allow the user to specify a UID instead of a username).
> + exit_code = EXIT_FAILURE;
> + pthread_exit(NULL);
> + }
> +
> + if (getuid() == 0 || getgid() == 0)
Would it be safer to do something like the following?
if (getgid() == 0)
{
// drop gid
}
if (getuid() == 0)
{
// drop uid
}
> + {
> + if (setgid(pwd->pw_gid) == -1)
Rather than check for error, it's safer to check for non-success:
if (setgid(pwd->pw_gid) != 0)
> + {
> + LOG(LOG_ERR, "can't drop privileges");
Please add more detail, e.g.:
LOG(LOG_ERR, "can't change group ID to %" PRIuMAX ": %s (%d)",
(uintmax_t)(pwd->pw_gid), strerror(errno), errno);
> + exit_code = EXIT_FAILURE;
> + pthread_exit(NULL);
> + }
> + if (setuid(pwd->pw_uid) == -1)
(same comment as above)
> + {
> + LOG(LOG_ERR, "can't drop privileges");
(same comment as above)
> + exit_code = EXIT_FAILURE;
> + pthread_exit(NULL);
> + }
> + }
We should also drop supplementary group memberships (before dropping
root UID). Sadly there's no POSIX API for this, only for getting the
list of supplementary groups. Perhaps we can take the following approach:
1. add an autoconf test to configure.ac to see if the system has
setgroups() or initgroups()
2. tweak the code to do the following:
* call getgroups() to get a list of supplementary groups
* if the user is in any supplementary groups:
- if setgroups() or initgroups() is available, use it to shed
the supplementary groups
- else error out
Also, please check to make sure getuid(), getgid(), and getgroups()
return the expected values after setting the UID and GIDs.
Also, please move this block of code to its own function.
> +
> block_signals();
> run_state->db_request_queue = Queue_new(true);
> if (run_state->db_request_queue == NULL)
> diff --git a/etc/rpstir.conf.in b/etc/rpstir.conf.in
> index 9eedd21..2cf2dae 100644
> --- a/etc/rpstir.conf.in
> +++ b/etc/rpstir.conf.in
> @@ -9,6 +9,9 @@
> # RPKICacheDir) can take either absolute paths or paths relative to this
> # configuration file.
>
> +# rpstir user account
> +rpstirUser rpstir
> +
> # Database to use.
> Database some_database
>
> diff --git a/lib/config/config.c b/lib/config/config.c
> index 4ee824e..9ecc0bd 100644
> --- a/lib/config/config.c
> +++ b/lib/config/config.c
> @@ -14,6 +14,16 @@
>
> /** All available config options */
> static const struct config_option config_options[] = {
> + //CONFIG_RPSTIR_USER
> + {
> + "rpstirUser",
> + false,
> + config_type_string_converter, &config_type_string_arg_mandatory,
> + config_type_string_converter_inverse, NULL,
> + free,
> + NULL, NULL,
> + NULL},
> +
Please put this new entry at the end. (Though it doesn't matter in this
case, it's good to be in the habit of preserving ABI compatibility as
much as possible.)
> // CONFIG_RPKI_PORT
> {
> "RPKIPort",
> diff --git a/lib/config/config.h b/lib/config/config.h
> index 59e1686..320142a 100644
> --- a/lib/config/config.h
> +++ b/lib/config/config.h
> @@ -60,6 +60,7 @@
>
>
> enum config_key {
> + CONFIG_RPSTIR_USER,
Please move this at the end.
> CONFIG_RPKI_PORT,
> CONFIG_DATABASE,
> CONFIG_DATABASE_USER,
> @@ -111,6 +112,7 @@ enum config_key {
> }
> */
>
> +CONFIG_GET_HELPER(CONFIG_RPSTIR_USER, char)
Also move this to the end.
Thanks,
Richard
> CONFIG_GET_HELPER_DEREFERENCE(CONFIG_RPKI_PORT, uint16_t)
> CONFIG_GET_HELPER(CONFIG_DATABASE, char)
> CONFIG_GET_HELPER(CONFIG_DATABASE_USER, char)
------------------------------------------------------------------------------
_______________________________________________
rpstir-devel mailing list
rpstir-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/rpstir-devel
