[pve-devel] [PATCH v2 cluster 1/2] pmxcfs: add verify_token IPCC request

[Thomas Lamprecht]

On 11/21/19 3:43 PM, Fabian Grünbichler wrote:
> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
>  data/src/cfs-ipc-ops.h |  2 ++
>  data/src/server.c      | 58 ++++++++++++++++++++++++++++++++++++++++++
>  data/src/status.c      |  1 +
>  data/PVE/Cluster.pm    | 18 +++++++++++++
>  4 files changed, 79 insertions(+)
> 
> diff --git a/data/src/cfs-ipc-ops.h b/data/src/cfs-ipc-ops.h
> index e4026ad..9d788bc 100644
> --- a/data/src/cfs-ipc-ops.h
> +++ b/data/src/cfs-ipc-ops.h
> @@ -41,4 +41,6 @@
>  
>  #define CFS_IPC_GET_GUEST_CONFIG_PROPERTY 11
>  
> +#define CFS_IPC_VERIFY_TOKEN 12
> +
>  #endif
> diff --git a/data/src/server.c b/data/src/server.c
> index 36acc1d..6d67f6b 100644
> --- a/data/src/server.c
> +++ b/data/src/server.c
> @@ -89,6 +89,11 @@ typedef struct {
>  	char property[];
>  } cfs_guest_config_propery_get_request_header_t;
>  
> +typedef struct {
> +	struct qb_ipc_request_header req_header;
> +	char token[];
> +} cfs_verify_token_request_header_t;
> +
>  struct s1_context {
>  	int32_t client_pid;
>  	uid_t uid;
> @@ -343,6 +348,59 @@ static int32_t s1_msg_process_fn(
>  
>  			result = cfs_create_guest_conf_property_msg(outbuf, memdb, rh->property, rh->vmid);
>  		}
> +	} else if (request_id == CFS_IPC_VERIFY_TOKEN) {
> +
> +		cfs_verify_token_request_header_t *rh = (cfs_verify_token_request_header_t *) data;
> +		int tokenlen = request_size - G_STRUCT_OFFSET(cfs_verify_token_request_header_t, token);
> +
> +		if (tokenlen <= 0) {
> +			cfs_debug("tokenlen <= 0, %d", tokenlen);
> +			result = -EINVAL;
> +		} else if (memchr(rh->token, '\n', tokenlen) != NULL) {
> +			cfs_debug("token contains newline");
> +			result = -EINVAL;
> +		} else if (rh->token[tokenlen-1] != '\0') {
> +			cfs_debug("token not NULL-terminated");
> +			result = -EINVAL;
> +		} else if (strnlen(rh->token, tokenlen-1) != tokenlen - 1) {
> +			cfs_debug("token contains NULL-byte");
> +			result = -EINVAL;
> +		} else {
> +			cfs_debug("cfs_verify_token: basic validity checked, reading token.cfg");
> +			gpointer tmp = NULL;
> +			int bytes_read = memdb_read(memdb, "priv/token.cfg", &tmp);

priv/ makes this a bit namespaced already, but maybe "access-token.cfg"
could be a bit more explaining

> +			size_t remaining = bytes_read > 0 ? bytes_read : 0;
> +			if (tmp != NULL && remaining > 0) {

remaining >= tokenlen

> +				char *line = (char *) tmp;
> +				char *next_line;
> +				char *end = line + remaining;
> +				*end = '\0';

should be:
const char *const end = line + remaining;

You mustn't write at *end, that an out-of-bound access, and undefined
behavior. Check my "_get_property_value" in status.c, had similar issues
first, now it gets it right™.

> +
> +				while (line != NULL) {
> +					next_line = memchr(line, '\n', remaining);
> +					if (next_line != NULL) {
> +						*next_line = '\0';

unnecessary, just use (next_line or end) - line as boundaries and do a
strncmp below[0]

> +					}
> +					/* we ensure above that both are properly terminated */

you ain't if next_line was NULL, i.e., no \n at EOF, at least if you
do not do your out-of-bound write to *end = 0 anymore ;-)

> +					if (strcmp(line, rh->token) == 0) {

[0] ...here

effectively I'd go with:

int bytes_read = memdb_read(memdb, "priv/token.cfg", &tmp);
size_t remaining = bytes_read > 0 ? bytes_read : 0;
if (tmp != NULL && remaining >= tokenlen) {
    char *line = (char *) tmp;
    char *next_newline;
    char *const end = line + remaining;

    while (line != NULL) {
        next_newline = memchr(line, '\n', remaining);
        if (next_newline != NULL) {
            *next_newline = '\0';
        }

        if (memcmp(line, rh->token, tokenlen) == 0) {
            result = 0; // found
            break;
        }

        line = next_newline;
        if (line != NULL) {
            line += 1;
            remaining = end - line;
            if (remaining < tokenlen) {
                result = -ENOENT;
                break;
            }
        }
    }
    if (line == NULL) {
        result = -ENOENT;
    }
    g_free(tmp);
} else {
    cfs_debug("token: token.cfg does not exist - ENOENT");
    result = -ENOENT;
}

here, the actually search part could be moved out to a is_line_in_conf
function, as it's completely agnostic to tokens itself..


> +						result = 0;
> +						break;
> +					}
> +					line = next_line;
> +					if (line != NULL) {
> +						line += 1;
> +						remaining = end - line;
> +					}
> +				}
> +				if (line == NULL) {
> +					result = -ENOENT;
> +				}
> +				g_free(tmp);
> +			} else {
> +				cfs_debug("token: token.cfg does not exist - ENOENT");
> +				result = -ENOENT;
> +			}
> +		}
>  	}
>  
>  	cfs_debug("process result %d", result);
> diff --git a/data/src/status.c b/data/src/status.c
> index 62eaa76..453fcaf 100644
> --- a/data/src/status.c
> +++ b/data/src/status.c
> @@ -96,6 +96,7 @@ static memdb_change_t memdb_change_array[] = {
>  	{ .path = "ceph.conf" },
>  	{ .path = "sdn.cfg" },
>  	{ .path = "sdn.cfg.new" },
> +	{ .path = "priv/token.cfg" },
>  };
>  
>  static GMutex mutex;
> diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
> index 2057162..e888ae8 100644
> --- a/data/PVE/Cluster.pm
> +++ b/data/PVE/Cluster.pm
> @@ -182,6 +182,18 @@ my $ipcc_get_cluster_log = sub {
>      return &$ipcc_send_rec(CFS_IPC_GET_CLUSTER_LOG, $bindata);
>  };
>  
> +my $ipcc_verify_token = sub {
> +    my ($full_token) = @_;
> +
> +    my $bindata = pack "Z*", $full_token;
> +    my $res = PVE::IPCC::ipcc_send_rec(CFS_IPC_VERIFY_TOKEN, $bindata);
> +
> +    return 1 if $! == 0;
> +    return 0 if $! == ENOENT;
> +
> +    die "$!\n";
> +};
> +
>  my $ccache = {};
>  
>  sub cfs_update {
> @@ -442,6 +454,12 @@ sub get_cluster_log {
>      return &$ipcc_get_cluster_log($user, $max);
>  }
>  
> +sub verify_token {
> +    my ($userid, $token) = @_;
> +
> +    return &$ipcc_verify_token("$userid $token");
> +}
> +
>  my $file_info = {};
>  
>  sub cfs_register_file {
>