On November 23, 2019 5:21 pm, Thomas Lamprecht wrote:
> On 11/21/19 3:43 PM, Fabian Grünbichler wrote:
>> Signed-off-by: Fabian Grünbichler <f.gruenbich...@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
ACK
>
>> + 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™.
I actually used that as a starting point ;) but you are of course
correct, we can't ensure that the last line is 0-terminated this way.
>
>> +
>> + 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]
but I actually want to ensure I compare both token and the full line, no
matter which one is longer.
e.g., if the file contains (for whatever reason!)
someuser@pve!token asd213
someotheruser@pve!token asdas-asdas-dsadsa
and I compare against
someuser@pve!token asd213-dfsdasads-asdsdas-dasds
I don't want to compare across the newline! while it should never be
able to produce a false match (we reject token values as needle that
contain a newline), it seems like a good safeguard to ensure we only
match one full line at a time..
>
>> + }
>> + /* 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';
so in practice I'd add another check here that the current line
(next_newline - line) is as long as rh->token, and if it is not, proceed
directly to the next line. this might also speed it up, since we now
only do the full string/memory comparison against stored values where
the full tokenid has the same length as the one we are looking for.
if next_newline == NULL, then we must be in the last loop, and remaining
should be exactly tokenlen, so we could check that in an else branch and
bail if that check fails.
> }
>
> 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..
sounds like a good idea - but where to put it? actually create the msg
and put most of the code in status.c, like for the property getter? or
keep the token stuff inline, and just move the line-search to
cfs-utils.c?
>
>
>> + 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 {
>>
>
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
