In case get_spec_node() is being called for a special node starting
with '@' it won't set *canonical_name. This can result in a crash of
xenstored due to dereferencing the uninitialized name in
fire_watches().

This is no security issue as it requires either a privileged caller or
ownership of the special node in question by an unprivileged caller
(which is questionable, as this would make the owner privileged in some
way).

Fixes: d6bb63924fc2 ("tools/xenstore: introduce dummy nodes for special watch paths")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xenstored_core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index a1d3047e48..790c403904 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1252,8 +1252,11 @@ static struct node *get_spec_node(struct connection *conn, const void *ctx,
 				  const char *name, char **canonical_name,
 				  unsigned int perm)
 {
-	if (name[0] == '@')
+	if (name[0] == '@') {
+		if (canonical_name)
+			*canonical_name = (char *)name;
 		return get_node(conn, ctx, name, perm);
+	}
 
 	return get_node_canonicalized(conn, ctx, name, canonical_name, perm);
 }
-- 
2.35.3

Hi Juergen,

On 20/07/2023 16:08, Juergen Gross wrote:
> In case get_spec_node() is being called for a special node starting
> with '@' it won't set *canonical_name. This can result in a crash of
> xenstored due to dereferencing the uninitialized name in
> fire_watches().
> 
> This is no security issue as it requires either a privileged caller or
> ownership of the special node in question by an unprivileged caller
> (which is questionable, as this would make the owner privileged in some
> way).
> 
> Fixes: d6bb63924fc2 ("tools/xenstore: introduce dummy nodes for special watch paths")
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>   tools/xenstore/xenstored_core.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index a1d3047e48..790c403904 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1252,8 +1252,11 @@ static struct node *get_spec_node(struct connection *conn, const void *ctx,
>   				  const char *name, char **canonical_name,
>   				  unsigned int perm)
>   {
> -	if (name[0] == '@')
> +	if (name[0] == '@') {
> +		if (canonical_name)
> +			*canonical_name = (char *)name;

eww. Let's not continue the bad practice in Xenstored to cast away the 
const. I will have a look to remove the const and you can rebase your 
patch on top.

Cheers,

-- 
Julien Grall

On 21.07.23 00:45, Julien Grall wrote:
> Hi Juergen,
> 
> On 20/07/2023 16:08, Juergen Gross wrote:
>> In case get_spec_node() is being called for a special node starting
>> with '@' it won't set *canonical_name. This can result in a crash of
>> xenstored due to dereferencing the uninitialized name in
>> fire_watches().
>>
>> This is no security issue as it requires either a privileged caller or
>> ownership of the special node in question by an unprivileged caller
>> (which is questionable, as this would make the owner privileged in some
>> way).
>>
>> Fixes: d6bb63924fc2 ("tools/xenstore: introduce dummy nodes for special watch 
>> paths")
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   tools/xenstore/xenstored_core.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>> index a1d3047e48..790c403904 100644
>> --- a/tools/xenstore/xenstored_core.c
>> +++ b/tools/xenstore/xenstored_core.c
>> @@ -1252,8 +1252,11 @@ static struct node *get_spec_node(struct connection 
>> *conn, const void *ctx,
>>                     const char *name, char **canonical_name,
>>                     unsigned int perm)
>>   {
>> -    if (name[0] == '@')
>> +    if (name[0] == '@') {
>> +        if (canonical_name)
>> +            *canonical_name = (char *)name;
> 
> eww. Let's not continue the bad practice in Xenstored to cast away the const. I 
> will have a look to remove the const and you can rebase your patch on top.

I think it should be possible to make canonical_name const. I'll look into that.


Juergen