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