[RFC PATCH 07/13] discover/discover-server: Restrict clients based on uid
Samuel Mendoza-Jonas
sam at mendozajonas.com
Mon Jul 2 10:59:46 AEST 2018
On Fri, 2018-06-29 at 02:49 +0000, Grandbois, Brett wrote:
> Hi Sam,
>
> Looks good overall so far. In the processing for AUTH_MSG_SET is it a
> security hole to allow the client to just blindly set a new root password
> without providing the old one? The password check is only done if
> restrict_clients is set so in other configurations a client is able to change
> the root password? Some other options there would be to not process
> authentication messages at all unless restrict_clients is set? Or maybe some
> sort of session token to declare that a client has previously authenticated
> to allow it?
>
Hi Brett,
It's somewhere between a security hole and a feature; the idea here is that if
no password is set (ie. found in NVRAM) then restrict_clients isn't set and
clients aren't restricted from doing anything, including setting a password.
Indeed in this case there isn't a previous password, the system is just
unconfigured in that respect.
Mainly it depends if we want to allow users a way of setting the password from
the UI. If we're happy to require users to drop to the shell to set a password
in NVRAM then yes we can just drop any authentication message if
restrict_clients is not set, but in general we've tried to avoid making that
necessary.
Regards,
Sam
> ________________________________________
> From: Petitboot <petitboot-bounces+brett.grandbois=opengear.com at lists.ozlabs.org> on behalf of Samuel Mendoza-Jonas <sam at mendozajonas.com>
> Sent: Thursday, June 28, 2018 4:41:45 PM
> To: petitboot at lists.ozlabs.org
> Cc: Samuel Mendoza-Jonas
> Subject: [RFC PATCH 07/13] discover/discover-server: Restrict clients based on uid
>
> If crypt support is enabled restrict what actions clients can perform by
> default. Initial authorisation is set at connection time; clients
> running as root are unrestricted, anything else runs as restricted until
> it makes an authentication to pb-discover.
>
> Unprivileged clients may only perform the following actions:
> - Boot the default boot option.
> - Cancel the autoboot timeout.
> - Make an authentication request.
>
> If a group named "petitgroup" exists then the socket permissions are
> also modified so that only clients running as root or in that group may
> connect to the socket.
> The user-event socket is only usable by root since the two main
> usecases are by utilities called by pb-discover or by a user in the
> shell who will need to su to root anyway.
>
> Signed-off-by: Samuel Mendoza-Jonas <sam at mendozajonas.com>
> ---
> discover/discover-server.c | 233 ++++++++++++++++++++++++++++++++++++-
> discover/discover-server.h | 3 +
> discover/pb-discover.c | 3 +
> discover/platform.c | 13 +++
> discover/platform.h | 4 +
> discover/user-event.c | 7 +-
> 6 files changed, 260 insertions(+), 3 deletions(-)
>
> diff --git a/discover/discover-server.c b/discover/discover-server.c
> index 814053d..59c22ce 100644
> --- a/discover/discover-server.c
> +++ b/discover/discover-server.c
> @@ -1,3 +1,4 @@
> +#define _GNU_SOURCE
>
> #include <unistd.h>
> #include <stdlib.h>
> @@ -10,11 +11,15 @@
> #include <sys/socket.h>
> #include <sys/un.h>
> #include <asm/byteorder.h>
> +#include <grp.h>
> +#include <sys/stat.h>
>
> #include <pb-config/pb-config.h>
> #include <talloc/talloc.h>
> #include <waiter/waiter.h>
> #include <log/log.h>
> +#include <crypt/crypt.h>
> +#include <i18n/i18n.h>
>
> #include "pb-protocol/pb-protocol.h"
> #include "list/list.h"
> @@ -31,6 +36,7 @@ struct discover_server {
> struct list clients;
> struct list status;
> struct device_handler *device_handler;
> + bool restrict_clients;
> };
>
> struct client {
> @@ -39,6 +45,8 @@ struct client {
> struct waiter *waiter;
> int fd;
> bool remote_closed;
> + bool can_modify;
> + struct waiter *auth_waiter;
> };
>
>
> @@ -245,10 +253,122 @@ static int write_config_message(struct discover_server *server,
> return client_write_message(server, client, message);
> }
>
> +static int write_authenticate_message(struct discover_server *server,
> + struct client *client)
> +{
> + struct pb_protocol_message *message;
> + struct auth_message auth_msg;
> + int len;
> +
> + auth_msg.op = AUTH_MSG_RESPONSE;
> + auth_msg.authenticated = client->can_modify;
> +
> + len = pb_protocol_authenticate_len(&auth_msg);
> +
> + message = pb_protocol_create_message(client,
> + PB_PROTOCOL_ACTION_AUTHENTICATE, len);
> + if (!message)
> + return -1;
> +
> + pb_protocol_serialise_authenticate(&auth_msg, message->payload, len);
> +
> + return client_write_message(server, client, message);
> +}
> +
> +static int client_auth_timeout(void *arg)
> +{
> + struct client *client = arg;
> + int rc;
> +
> + client->can_modify = false;
> +
> + rc = write_authenticate_message(client->server, client);
> + if (rc)
> + pb_log("failed to send client auth timeout\n");
> +
> + return 0;
> +}
> +
> +static int discover_server_handle_auth_message(struct client *client,
> + struct auth_message *auth_msg)
> +{
> + struct status *status;
> + int rc;
> +
> + status = talloc_zero(client, struct status);
> +
> + switch (auth_msg->op) {
> + case AUTH_MSG_REQUEST:
> + if (!crypt_check_password(auth_msg->password)) {
> + rc = -1;
> + pb_log("Client failed to authenticate\n");
> + status->type = STATUS_ERROR;
> + status->message = talloc_asprintf(status,
> + _("Password incorrect"));
> + } else {
> + client->can_modify = true;
> + rc = write_authenticate_message(client->server,
> + client);
> + if (client->auth_waiter)
> + waiter_remove(client->auth_waiter);
> + client->auth_waiter = waiter_register_timeout(
> + client->server->waitset,
> + 300000, /* 5 min */
> + client_auth_timeout, client);
> + pb_log("Client authenticated\n");
> + status->type = STATUS_INFO;
> + status->message = talloc_asprintf(status,
> + _("Authenticated successfully"));
> + }
> + break;
> + case AUTH_MSG_SET:
> + if (client->server->restrict_clients) {
> + if (!crypt_check_password(auth_msg->set_password.password)) {
> + rc = -1;
> + pb_log("Wrong password for set request\n");
> + status->type = STATUS_ERROR;
> + status->message = talloc_asprintf(status,
> + _("Password incorrect"));
> + break;
> + }
> + }
> +
> + rc = crypt_set_password(auth_msg,
> + auth_msg->set_password.new_password);
> + if (rc) {
> + pb_log("Failed to set password\n");
> + status->type = STATUS_ERROR;
> + status->message = talloc_asprintf(status,
> + _("Error setting password"));
> + } else {
> + platform_set_password(auth_msg->set_password.new_password);
> + discover_server_set_auth_mode(client->server,
> + auth_msg->set_password.new_password != NULL);
> + pb_log("System password changed\n");
> + status->type = STATUS_ERROR;
> + status->message = talloc_asprintf(status,
> + _("Password updated successfully"));
> +
> + }
> + break;
> + default:
> + pb_log("%s: unknown op\n", __func__);
> + rc = -1;
> + break;
> + }
> +
> + write_boot_status_message(client->server, client, status);
> + talloc_free(status);
> +
> + return rc;
> +}
> +
> static int discover_server_process_message(void *arg)
> {
> struct pb_protocol_message *message;
> struct boot_command *boot_command;
> + struct auth_message *auth_msg;
> + struct status *status;
> struct client *client = arg;
> struct config *config;
> char *url;
> @@ -261,6 +381,56 @@ static int discover_server_process_message(void *arg)
> return 0;
> }
>
> + /*
> + * If crypt support is enabled, non-authorised clients can only delay
> + * boot, not configure options or change the default boot option.
> + */
> + if (!client->can_modify) {
> + switch (message->action) {
> + case PB_PROTOCOL_ACTION_BOOT:
> + boot_command = talloc(client, struct boot_command);
> +
> + rc = pb_protocol_deserialise_boot_command(boot_command,
> + message);
> + if (rc) {
> + pb_log("%s: no boot command?", __func__);
> + return 0;
> + }
> +
> + device_handler_boot(client->server->device_handler,
> + client->can_modify, boot_command);
> + break;
> + case PB_PROTOCOL_ACTION_CANCEL_DEFAULT:
> + device_handler_cancel_default(client->server->device_handler);
> + break;
> + case PB_PROTOCOL_ACTION_AUTHENTICATE:
> + auth_msg = talloc(client, struct auth_message);
> + rc = pb_protocol_deserialise_authenticate(
> + auth_msg, message);
> + if (rc) {
> + pb_log("Couldn't parse client's auth request\n");
> + break;
> + }
> +
> + rc = discover_server_handle_auth_message(client,
> + auth_msg);
> + talloc_free(auth_msg);
> + break;
> + default:
> + pb_log("non-root client tried to perform action %d\n",
> + message->action);
> + status = talloc_zero(client, struct status);
> + if (status) {
> + status->type = STATUS_ERROR;
> + status->message = talloc_asprintf(status,
> + "Client must run as root to make changes");
> + write_boot_status_message(client->server, client,
> + status);
> + talloc_free(status);
> + }
> + }
> + return 0;
> + }
>
> switch (message->action) {
> case PB_PROTOCOL_ACTION_BOOT:
> @@ -274,7 +444,7 @@ static int discover_server_process_message(void *arg)
> }
>
> device_handler_boot(client->server->device_handler,
> - boot_command);
> + client->can_modify, boot_command);
> break;
>
> case PB_PROTOCOL_ACTION_CANCEL_DEFAULT:
> @@ -311,6 +481,19 @@ static int discover_server_process_message(void *arg)
> device_handler_install_plugin(client->server->device_handler,
> url);
> break;
> + /* For AUTH_MSG_SET */
> + case PB_PROTOCOL_ACTION_AUTHENTICATE:
> + auth_msg = talloc(client, struct auth_message);
> + rc = pb_protocol_deserialise_authenticate(
> + auth_msg, message);
> + if (rc) {
> + pb_log("Couldn't parse client's auth request\n");
> + break;
> + }
> +
> + rc = discover_server_handle_auth_message(client, auth_msg);
> + talloc_free(auth_msg);
> + break;
> default:
> pb_log("%s: invalid action %d\n", __func__, message->action);
> return 0;
> @@ -320,12 +503,27 @@ static int discover_server_process_message(void *arg)
> return 0;
> }
>
> +void discover_server_set_auth_mode(struct discover_server *server,
> + bool restrict_clients)
> +{
> + struct client *client;
> +
> + server->restrict_clients = restrict_clients;
> +
> + list_for_each_entry(&server->clients, client, list) {
> + client->can_modify = !restrict_clients;
> + write_authenticate_message(server, client);
> + }
> +}
> +
> static int discover_server_process_connection(void *arg)
> {
> struct discover_server *server = arg;
> struct statuslog_entry *entry;
> int fd, rc, i, n_devices, n_plugins;
> struct client *client;
> + struct ucred ucred;
> + socklen_t len;
>
> /* accept the incoming connection */
> fd = accept(server->socket, NULL, NULL);
> @@ -346,6 +544,30 @@ static int discover_server_process_connection(void *arg)
> WAIT_IN, discover_server_process_message,
> client);
>
> + /*
> + * get some info on the connecting process - if the client is being
> + * run as root allow them to make changes
> + */
> + if (server->restrict_clients) {
> + len = sizeof(struct ucred);
> + rc = getsockopt(client->fd, SOL_SOCKET, SO_PEERCRED, &ucred,
> + &len);
> + if (rc) {
> + pb_log("Failed to get socket info - restricting client\n");
> + client->can_modify = false;
> + } else {
> + pb_log("Client details: pid: %d, uid: %d, egid: %d\n",
> + ucred.pid, ucred.uid, ucred.gid);
> + client->can_modify = ucred.uid == 0;
> + }
> + } else
> + client->can_modify = true;
> +
> + /* send auth status to client */
> + rc = write_authenticate_message(server, client);
> + if (rc)
> + return 0;
> +
> /* send sysinfo to client */
> rc = write_system_info_message(server, client, system_info_get());
> if (rc)
> @@ -492,6 +714,7 @@ struct discover_server *discover_server_init(struct waitset *waitset)
> {
> struct discover_server *server;
> struct sockaddr_un addr;
> + struct group *group;
>
> server = talloc(NULL, struct discover_server);
> if (!server)
> @@ -511,7 +734,6 @@ struct discover_server *discover_server_init(struct waitset *waitset)
> }
>
> talloc_set_destructor(server, server_destructor);
> -
> addr.sun_family = AF_UNIX;
> strcpy(addr.sun_path, PB_SOCKET_PATH);
>
> @@ -520,6 +742,13 @@ struct discover_server *discover_server_init(struct waitset *waitset)
> goto out_err;
> }
>
> + /* Allow all clients to communicate on this socket */
> + group = getgrnam("petitgroup");
> + if (group) {
> + chown(PB_SOCKET_PATH, 0, group->gr_gid);
> + chmod(PB_SOCKET_PATH, 0660);
> + }
> +
> if (listen(server->socket, 8)) {
> pb_log("server socket listen: %s\n", strerror(errno));
> goto out_err;
> diff --git a/discover/discover-server.h b/discover/discover-server.h
> index 9f3aa62..9722e17 100644
> --- a/discover/discover-server.h
> +++ b/discover/discover-server.h
> @@ -20,6 +20,9 @@ void discover_server_destroy(struct discover_server *server);
> void discover_server_set_device_source(struct discover_server *server,
> struct device_handler *handler);
>
> +void discover_server_set_auth_mode(struct discover_server *server,
> + bool restrict_clients);
> +
> void discover_server_notify_device_add(struct discover_server *server,
> struct device *device);
> void discover_server_notify_boot_option_add(struct discover_server *server,
> diff --git a/discover/pb-discover.c b/discover/pb-discover.c
> index c494eeb..e2b36dd 100644
> --- a/discover/pb-discover.c
> +++ b/discover/pb-discover.c
> @@ -189,6 +189,9 @@ int main(int argc, char *argv[])
> if (config_get()->debug)
> pb_log_set_debug(true);
>
> + if (platform_restrict_clients())
> + discover_server_set_auth_mode(server, true);
> +
> system_info_init(server);
>
> handler = device_handler_init(server, waitset, opts.dry_run == opt_yes);
> diff --git a/discover/platform.c b/discover/platform.c
> index cc6306f..7ef0c88 100644
> --- a/discover/platform.c
> +++ b/discover/platform.c
> @@ -209,6 +209,19 @@ int platform_get_sysinfo(struct system_info *info)
> return -1;
> }
>
> +bool platform_restrict_clients(){
> + if (platform && platform->restrict_clients)
> + return platform->restrict_clients(platform);
> + return false;
> +}
> +
> +int platform_set_password(char *password)
> +{
> + if (platform && platform->set_password)
> + return platform->set_password(platform, password);
> + return -1;
> +}
> +
> int config_set(struct config *newconfig)
> {
> int rc;
> diff --git a/discover/platform.h b/discover/platform.h
> index 5aa8e3f..07e49b3 100644
> --- a/discover/platform.h
> +++ b/discover/platform.h
> @@ -11,6 +11,8 @@ struct platform {
> void (*pre_boot)(struct platform *,
> const struct config *);
> int (*get_sysinfo)(struct platform *, struct system_info *);
> + bool (*restrict_clients)(struct platform *);
> + int (*set_password)(struct platform *, char *password);
> uint16_t dhcp_arch_id;
> void *platform_data;
> };
> @@ -19,6 +21,8 @@ int platform_init(void *ctx);
> int platform_fini(void);
> const struct platform *platform_get(void);
> int platform_get_sysinfo(struct system_info *info);
> +bool platform_restrict_clients(void);
> +int platform_set_password(char *password);
> void platform_pre_boot(void);
>
> /* configuration interface */
> diff --git a/discover/user-event.c b/discover/user-event.c
> index 77d28c1..00a5160 100644
> --- a/discover/user-event.c
> +++ b/discover/user-event.c
> @@ -24,6 +24,7 @@
> #include <errno.h>
> #include <string.h>
> #include <sys/socket.h>
> +#include <sys/stat.h>
> #include <sys/types.h>
> #include <sys/un.h>
>
> @@ -469,7 +470,7 @@ static int user_event_boot(struct user_event *uev, struct event *event)
> cmd->dtb_file = talloc_strdup(cmd, event_get_param(event, "dtb"));
> cmd->boot_args = talloc_strdup(cmd, event_get_param(event, "args"));
>
> - device_handler_boot(handler, cmd);
> + device_handler_boot(handler, false, cmd);
>
> talloc_free(cmd);
>
> @@ -711,6 +712,10 @@ struct user_event *user_event_init(struct device_handler *handler,
> strerror(errno));
> }
>
> + /* Don't allow events from non-priviledged users */
> + chown(PBOOT_USER_EVENT_SOCKET, 0, 0);
> + chmod(PBOOT_USER_EVENT_SOCKET, 0660);
> +
> waiter_register_io(waitset, uev->socket, WAIT_IN,
> user_event_process, uev);
>
> --
> 2.18.0
>
> _______________________________________________
> Petitboot mailing list
> Petitboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/petitboot
More information about the Petitboot
mailing list