Re: [PATCH] tools/xenstore: fix get_spec_node()

[Juergen Gross]

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



OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH] tools/xenstore: fix get_spec_node()

[Julien Grall]

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

[PATCH] tools/xenstore: fix get_spec_node()

[Juergen Gross]

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