[RFC PATCH 07/13] discover/discover-server: Restrict clients based on uid
Samuel Mendoza-Jonas
sam at mendozajonas.com
Tue Jul 3 13:33:37 AEST 2018
On Tue, 2018-07-03 at 09:24 +1000, Brett Grandbois wrote:
>
> On 02/07/18 10:59, Samuel Mendoza-Jonas wrote:
> > 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
>
> I am completely fine with allowing password set from the UI, its a nice
> admin feature. I guess where I get a bit wary is that normal password
> change convention is to require the old password when setting a new one,
> which is what would be required if a user had to drop into the shell to
> change it. But looking at the protocol handler implementation it
> appears to me that there are some situations where a client could set a
> root password via the protocol without having to supply the old one? I
> haven't gone through the full UI implementation as yet so maybe it isn't
> possible from there, but from the discover server side it looks possible
> to implement a means to do it.
>
Assuming I've covered all my bases correctly restrict_clients is only set
to false via discover_server_set_auth_mode() either in pb-discover.c when
the platform says no password is available, or in
discover_server_handle_auth_message() if the user changes the password to
nothing. Otherwise restrict_clients is true and the password check
occurs. So the only time a user should be able to change the password
without supplying the old password is when there isn't any old password.
Can you see a corner case I might have missed?
Thanks,
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