[RFC PATCH 07/13] discover/discover-server: Restrict clients based on uid
Samuel Mendoza-Jonas
sam at mendozajonas.com
Mon Jul 9 11:38:50 AEST 2018
On Tue, 2018-07-03 at 15:51 +1000, Brett Grandbois wrote:
>
> On 03/07/18 13:33, Samuel Mendoza-Jonas wrote:
> > 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
> >
>
> That covers platform-powerpc, but what about a default case where no
> platform is detected, for example an environment that isn't using
> device-tree like x86? The default case for platform_restrict_clients()
> is to return false if no platform->restrict_clients is defined, which
> then looks like can flow down to a client able to crypt_set_password
> without the corresponding crypt_check_password. Hmm, maybe all that's
> needed is to default platform_restrict_clients() to return true in the
> absence of any platform definition? That would then enable the
> can_modify checks in discover_server_process_connection, but is that a
> bad thing? Having restrict_clients true by default might be the safer
> option overall.
>
My concern here is that on such a platform (or on ppc without a password
configured) there is then no way to perform any changes via the client
since there is no mechanism to authorise with the server. For example the
user won't be able to configure the network or change the autoboot order.
If you're just plugging in a DVD that will work to get into into an
installer, but for anything that requires some configuration you're a bit
stuck.
My intent at least initially was to provide a method to configure
password-based access, but not unconditionally lock down any machine
which was built with --enable-crypt.
> > > >
> > > > > ________________________________________
> > > > > 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