[RFC PATCH 07/13] discover/discover-server: Restrict clients based on uid
Brett Grandbois
brett.grandbois at opengear.com
Tue Jul 3 09:24:34 AEST 2018
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.
>
>> ________________________________________
>> 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