makedevs: Fix matching uid/gid

From: Jaeyoon Jung <jaeyoon.jung@lge.com>

Use strcmp over strncmp to exact-match uid or gids. Otherwise it may end
up with returning a wrong id that matches partially.

Signed-off-by: Jaeyoon Jung <jaeyoon.jung@lge.com>
---
 meta/recipes-devtools/makedevs/makedevs/makedevs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On Wed, 2024-08-14 at 11:55 +0900, Jaeyoon Jung (LGE) via lists.openembedded.org wrote:
> From: Jaeyoon Jung <jaeyoon.jung@lge.com>
> 
> Use strcmp over strncmp to exact-match uid or gids. Otherwise it may end
> up with returning a wrong id that matches partially.
> 
> Signed-off-by: Jaeyoon Jung <jaeyoon.jung@lge.com>
> ---
>  meta/recipes-devtools/makedevs/makedevs/makedevs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/meta/recipes-devtools/makedevs/makedevs/makedevs.c b/meta/recipes-devtools/makedevs/makedevs/makedevs.c
> index 2254b54891..84b38d2b74 100644
> --- a/meta/recipes-devtools/makedevs/makedevs/makedevs.c
> +++ b/meta/recipes-devtools/makedevs/makedevs/makedevs.c
> @@ -202,7 +202,7 @@ static unsigned long convert2guid(char *id_buf, struct name_id *search_list)
>  		// Check for bad user/group name
>  		node = search_list;
>  		while (node != NULL) {
> -			if (!strncmp(node->name, id_buf, strlen(id_buf))) {
> +			if (!strcmp(node->name, id_buf)) {
>  				fprintf(stderr, "WARNING: Bad user/group name %s detected\n", id_buf);
>  				break;
>  			}
> @@ -212,7 +212,7 @@ static unsigned long convert2guid(char *id_buf, struct name_id *search_list)
>  	} else {
>  		node = search_list;
>  		while (node != NULL) {
> -			if (!strncmp(node->name, id_buf, strlen(id_buf)))
> +			if (!strcmp(node->name, id_buf))
>  				return node->id;
>  			node = node->next;
>  		}

I'm afraid I don't understand the problem here. If partial matching is
occurring, doesn't that mean MAX_ID_LEN is wrong?

If we drop the strncmp() here, that appears to put us at risk of buffer
overflow problems, particularly if MAX_ID_LEN is too short?

Can you give an example of how this is failing and where the above
change helps?

Thanks,

Richard

On Mon, 2024-08-26 at 19:07 +0900, Jung Jaeyoon(Jay) wrote:
> On Sat, Aug 17, 2024 at 06:47 AM, Richard Purdie wrote:
> > I'm afraid I don't understand the problem here. If partial matching
> > is occurring, doesn't that mean MAX_ID_LEN is wrong?
> > 
> > If we drop the strncmp() here, that appears to put us at risk of
> > buffer overflow problems, particularly if MAX_ID_LEN is too short?
> > 
> > Can you give an example of how this is failing and where the above
> > change helps?
> 
> 
> (Sorry for being late responding on this. It has been missing in my
> INBOX somehow.)
> 
> A typical case where it is failing is when id_buf is shorter than
> node->name and they match partially, e.g, id_buf is "foo" and node-
> >name is "foobar".
> Note that it's using strlen(id_buf), not strlen(MAX_ID_LEN).

convert2guid, where the strlen is located, is called from two places
both in interpret_table_entry() and there the buffer is defined as:

	char usr_buf[MAX_ID_LEN];
	char grp_buf[MAX_ID_LEN];

The name field of node is defined as:

char name[MAX_NAME_LEN+1]

which could be a problem if they differ but:

#define MAX_ID_LEN      40
#define MAX_NAME_LEN    40

so the only issue I can see is there is perhaps an off by one on the
length definition?

> I believe using strcmp() here is safe because it is guaranteed that
> both strings cannot be longer than MAX_ID_LEN(=MAX_NAME_LEN) and are
> always NULL-terminated.

Perhaps, but I think the existing code should work and if there is a
bug, I'd like to understand what it is. Is the issue an off by one on
the length?

I'd ask again if you have an actual example of the failure? Is it only
with strings of 39/40 length?

Cheers,

Richard

On Mon, 2024-08-26 at 22:41 +0900, Jung Jaeyoon(Jay) wrote:
> As an actual example, suppose there is an entry as below in a device
> table that makedevs receives:
> 
> /dev/system        c       0660    root       system      1000     
> 0      -       -       -
> 
> then you will get "system" group for id_buf in convert2guid().
> and suppose there are two entries in /etc/group where both start with
> "system" like:
> 
> systemd-journal:x:900:
> system:x:2020:
> 
> Obviously convert2guid() should return 2020 in this case, but with
> the existing code it will return 900 because the entry of "systemd-
> journal" comes first in the loop and strncmp("systemd-journal",
> "system", strlen("system")) returns 0.

That makes sense and does indeed look like a bug. I think we should 
change the strlen to MAX_ID_LEN instead?

I'm still a little worried there is an off by one error somewhere in
there too.

Cheers,

Richard