[PATCH 2/2] ext_password: Implement new file-based backend

[Jouni Malinen]

On Sun, Feb 07, 2021 at 06:48:36PM +0100, Patrick Steinhardt wrote:
> It's currently not easily possible to separate configuration of an
> interface and credentials. This makes it hard to distribute
> configuration across a set of nodes which use wpa_supplicant without
> also having to store credentials in the same file. While this can be
> solved via scripting, having a native way to achieve this would be
> preferable.
> 
> Turns out there already is a framework to have external password
> storages. It only has a single "test" backend though, which is kind of
> an in-memory store which gets initialized with all passwords up front.
> This isn't really suitable for above usecase: the backend cannot be
> initialized as part of the central configuration given that it needs the
> credentials, and we want to avoid scripting.
> 
> This commit thus extends the infrastructure to implement a new backend,
> which instead uses a simple configuration file containing key-value
> pairs. The file follows the format which wpa_supplicant.conf(5) uses:
> empty lines and comments are ignored, while passwords can be specified
> with simple `password-name=password-value` assignments.

This sounds like a reasonable addition.

> diff --git a/src/utils/ext_password_file.c b/src/utils/ext_password_file.c
> +#include "common.h"
> +#include "ext_password_i.h"
> +#include "utils/config.h"

I'd prefer to list the src/*/*.h before wpa_supplicant/*.h in include
statements.

> +static void * ext_password_file_init(const char *params)
> +{
> +	struct ext_password_file_data *data;
> +
> +	if (!params) {
> +		wpa_printf(MSG_ERROR, "EXT PW FILE: no path given");
> +		return NULL;
> +	}
> +
> +	data = os_zalloc(sizeof(*data));
> +	if (data == NULL)
> +		return NULL;
> +	data->path = os_strdup(params);
> +
> +	return data;

That os_strdup() might fail. It would be better check for that here and
return NULL instead of assuming everything is fine and later crashing
due to NULL pointer dereferencing..

> +static void ext_password_file_deinit(void *ctx)
> +{
> +	struct ext_password_file_data *data = ctx;
> +
> +	str_clear_free(data->path);

str_clear_free() sounds a bit heavy for a path name, but well, if that
contains some secure information.. However:

> +static struct wpabuf * ext_password_file_get(void *ctx, const char *name)
> +{
> +	struct ext_password_file_data *data = ctx;
> +	struct wpabuf *password = NULL;
> +	char buf[512], *pos;

This buf[] is used to read the actual passwords, so it would be more
useful to explicitly clear that memory after use here.. And probably not
the best design to use wpa_printf(MSG_ERROR, "stuff with the raw line
from the password file") to get passwords exposed in debug logs and/or
stdout. Maybe just print the line number without any of the payload.

> +	while (wpa_config_get_line(buf, sizeof(buf), f, &line, &pos)) {
> +done:
> +	fclose(f);
> +	return password;

In other words, forced_memzero(buf, sizeof(buf)) before returning from
the function.

> diff --git a/wpa_supplicant/Makefile b/wpa_supplicant/Makefile

And also similar changes for wpa_supplicant/Android.mk.

-- 
Jouni Malinen                                            PGP id EFC895FA