[RFC PATCH 07/13] discover/discover-server: Restrict clients based on uid
Grandbois, Brett
brett.grandbois at opengear.com
Fri Jun 29 12:49:03 AEST 2018
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?
________________________________________
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