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
>> ---
>> 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 = l
