| Message ID | 20230724103324.4207-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/xenstore: fix get_spec_node() | expand |
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
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); }
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(-)