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>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
V2:
- avoid casting const away (Julien Grall)
---
 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 6d27b2dd7f..9f1328a31f 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1253,8 +1253,11 @@ static struct node *get_spec_node(struct connection *conn, const void *ctx,
 				  const char *name, const char **canonical_name,
 				  unsigned int perm)
 {
-	if (name[0] == '@')
+	if (name[0] == '@') {
+		if (canonical_name)
+			*canonical_name = name;
 		return get_node(conn, ctx, name, perm);
+	}
 
 	return get_node_canonicalized(conn, ctx, name, canonical_name, perm);
 }
-- 
2.35.3

On 24.07.2023 12:33, 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>
> Reviewed-by: Julien Grall <jgrall@amazon.com>

I've committed the two patches, and I've queued this one for backporting.
Can at least one of you please confirm that the earlier patch is not
intended to be backported, and that instead a cast will need adding in
the backport of the one here?

Jan

On 27.07.23 09:53, Jan Beulich wrote:
> On 24.07.2023 12:33, 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>
>> Reviewed-by: Julien Grall <jgrall@amazon.com>
> 
> I've committed the two patches, and I've queued this one for backporting.

Thanks.

> Can at least one of you please confirm that the earlier patch is not
> intended to be backported, and that instead a cast will need adding in
> the backport of the one here?

Yes, that was the plan.


Juergen

On 27.07.2023 09:55, Juergen Gross wrote:
> On 27.07.23 09:53, Jan Beulich wrote:
>> On 24.07.2023 12:33, 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>
>>> Reviewed-by: Julien Grall <jgrall@amazon.com>
>>
>> I've committed the two patches, and I've queued this one for backporting.
> 
> Thanks.
> 
>> Can at least one of you please confirm that the earlier patch is not
>> intended to be backported, and that instead a cast will need adding in
>> the backport of the one here?
> 
> Yes, that was the plan.

Hmm, looks like the offending patch exists only on the master branch.

Jan

On 03.08.23 11:53, Jan Beulich wrote:
> On 27.07.2023 09:55, Juergen Gross wrote:
>> On 27.07.23 09:53, Jan Beulich wrote:
>>> On 24.07.2023 12:33, 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>
>>>> Reviewed-by: Julien Grall <jgrall@amazon.com>
>>>
>>> I've committed the two patches, and I've queued this one for backporting.
>>
>> Thanks.
>>
>>> Can at least one of you please confirm that the earlier patch is not
>>> intended to be backported, and that instead a cast will need adding in
>>> the backport of the one here?
>>
>> Yes, that was the plan.
> 
> Hmm, looks like the offending patch exists only on the master branch.

Oh, indeed. It seemed to be so long ago that I changed this ...


Juergen