Regression fix (affects 4.18 and 4.17): Some empty lists have 0 elements, not 1 empty string

[alejandro-colomar]

Reported-by: @osark084

This is not a fix for the bug you reported, but it fixes the regression I found, which is very related to what you reported.

Please check if this somehow changes the reproducibility of the other bug.

---

Tested:

alx@devuan:~/src/shadow/shadow/master$ cat /etc/group | grep foo
foo:x:1004:
alx@devuan:~/src/shadow/shadow/master$ sudo ./src/groupmod -U alx foo
alx@devuan:~/src/shadow/shadow/master$ cat /etc/group | grep foo
foo:x:1004:alx
alx@devuan:~/src/shadow/shadow/master$ sudo ./src/groupmod -U "" foo
alx@devuan:~/src/shadow/shadow/master$ cat /etc/group | grep foo
foo:x:1004:

---

Revisions:

v1b

• Document which programs are affected by the bug, in which versions, and in which configuration.

$ git rd 
1:  f71b2f85b ! 1:  93d169b08 lib/, src/: Some empty lists have 0 elements, not 1 empty string
    @@ Commit message
         we switched from using strtok(3) to strsep(3), without remembering to
         special-case an empty CSV.
     
    +    The bug affected directly groupadd(8) and groupmod(8).
    +
    +    The bug also affected the library function add_groups().  In systems
    +    using PAM, that function is unused.  On systems without PAM, it is
    +    called by the library function setup_uid_gid(), with the contents of the
    +    "CONSOLE_GROUPS" configuration (login.defs) CSV string.
    +
    +    setup_uid_gid() is directly called by su(1) and login(1) on systems
    +    without PAM.
    +
    +    setup_uid_gid() is also called by the library function expire().
    +
    +    expire() is directly called by expiry(1), su(1), and login(1).
    +
    +    This bug is a regression introduced in the release 4.17.0, and present
    +    in the releases 4.17.{0..4} and 4.18.0.
    +
         Fixes: 90afe61003ef (2024-12-05; "lib/, src/: Use strsep(3) instead of strtok(3)")
         Link: <https://github.com/shadow-maint/shadow/issues/1420>
         Reported-by: Osark Vieira <https://github.com/osark084>
2:  f7aa7b40c = 2:  cbe7205fb lib/, src/: Reduce scope of local variables

v1c

• Rebase

$ git rd 
1:  93d169b08 = 1:  e5b0dcfaf lib/, src/: Some empty lists have 0 elements, not 1 empty string
2:  cbe7205fb = 2:  3c364ebb8 lib/, src/: Reduce scope of local variables

v1d

• Rebase

$ git rd 
1:  e5b0dcfaf = 1:  aca05fbad lib/, src/: Some empty lists have 0 elements, not 1 empty string
2:  3c364ebb8 = 2:  a59f6e87f lib/, src/: Reduce scope of local variables

v1e

• Rebase

$ git range-diff master..gh/regression fa889977190a..regression 
1:  aca05fbad = 1:  548cbf6a9 lib/, src/: Some empty lists have 0 elements, not 1 empty string
2:  a59f6e87f = 2:  74feae9c9 lib/, src/: Reduce scope of local variables

v2

• Rebase after Test groupadd and groupmod -U option with empty user list #1427. This shows that the tests pass.

$ git rd 
-:  --------- > 1:  ac3a2d74d tests/system/tests/test_groupmod.py: add test for groupmod -U with empty string
-:  --------- > 2:  44bba1dcd tests/system/tests/test_groupadd.py: add test for groupadd -U with empty string
1:  548cbf6a9 = 3:  0845ded0b lib/, src/: Some empty lists have 0 elements, not 1 empty string
2:  74feae9c9 = 4:  9451f9c7b lib/, src/: Reduce scope of local variables

v3

• Split patches. First apply add a block and indent code, and then fix the bugs in a minimal patch.

Interdiff:

$ git diff gh/regression..regression 
$

$ git rd 
1:  ac3a2d74d = 1:  ac3a2d74d tests/system/tests/test_groupmod.py: add test for groupmod -U with empty string
2:  44bba1dcd = 2:  44bba1dcd tests/system/tests/test_groupadd.py: add test for groupadd -U with empty string
3:  0845ded0b ! 3:  2e1296d63 lib/, src/: Some empty lists have 0 elements, not 1 empty string
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>
     
      ## Commit message ##
    -    lib/, src/: Some empty lists have 0 elements, not 1 empty string
    +    lib/, src/: Add blocks
     
    -    In general, empty fields in a CSV are errors.  However, in some cases,
    -    we want to allow passing empty lists, and the way to encode that is as
    -    an empty string.  This was accidentally broken in the last release, when
    -    we switched from using strtok(3) to strsep(3), without remembering to
    -    special-case an empty CSV.
    +    This is in preparation for the following patch.
     
    -    The bug affected directly groupadd(8) and groupmod(8).
    -
    -    The bug also affected the library function add_groups().  In systems
    -    using PAM, that function is unused.  On systems without PAM, it is
    -    called by the library function setup_uid_gid(), with the contents of the
    -    "CONSOLE_GROUPS" configuration (login.defs) CSV string.
    -
    -    setup_uid_gid() is directly called by su(1) and login(1) on systems
    -    without PAM.
    -
    -    setup_uid_gid() is also called by the library function expire().
    -
    -    expire() is directly called by expiry(1), su(1), and login(1).
    -
    -    This bug is a regression introduced in the release 4.17.0, and present
    -    in the releases 4.17.{0..4} and 4.18.0.
    -
    -    Fixes: 90afe61003ef (2024-12-05; "lib/, src/: Use strsep(3) instead of strtok(3)")
    -    Link: <https://github.com/shadow-maint/shadow/issues/1420>
    -    Reported-by: Osark Vieira <https://github.com/osark084>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/addgrps.c ##
    -@@
    - #include "shadow/grp/agetgroups.h"
    - #include "shadowlog.h"
    - #include "string/strchr/strchrscnt.h"
    -+#include "string/strcmp/streq.h"
    - #include "string/strerrno.h"
    - 
    - 
     @@ lib/addgrps.c: add_groups(const char *list)
        if (dup == NULL)
                goto free_gids;
      
     -  while (NULL != (g = strsep(&p, ",:"))) {
     -          struct group  *grp;
    -+  if (!streq(dup, "")) {
    ++  {
     +          while (NULL != (g = strsep(&p, ",:"))) {
     +                  struct group  *grp;
      
    @@ lib/addgrps.c: add_groups(const char *list)
        free(dup);
      
     
    - ## src/groupadd.c ##
    -@@
    - #include "shadow/gshadow/sgrp.h"
    - #include "shadowlog.h"
    - #include "string/memset/memzero.h"
    -+#include "string/strcmp/streq.h"
    - #include "string/strerrno.h"
    - #include "string/strtok/stpsep.h"
    - 
    -@@ src/groupadd.c: grp_update(void)
    -   }
    - #endif                            /* SHADOWGRP */
    - 
    --  if (user_list) {
    -+  if (user_list && !streq(user_list, "")) {
    -           char  *u, *ul;
    - 
    -           ul = user_list;
    -
      ## src/groupmod.c ##
     @@ src/groupmod.c: grp_update(void)
                }
    @@ src/groupmod.c: grp_update(void)
     -                          fprintf(stderr, _("Invalid member username %s\n"), u);
     -                          exit (E_GRP_UPDATE);
     -                  }
    -+          if (!streq(user_list, "")) {
    ++          {
     +                  ul = user_list;
     +                  while (NULL != (u = strsep(&ul, ","))) {
     +                          if (prefix_getpwnam(u) == NULL) {
-:  --------- > 4:  77f368c10 lib/, src/: Some empty lists have 0 elements, not 1 empty string
4:  9451f9c7b = 5:  7f77d0aaa lib/, src/: Reduce scope of local variables

v3b

• Fix commit message.

$ git rd 
1:  ac3a2d74d = 1:  ac3a2d74d tests/system/tests/test_groupmod.py: add test for groupmod -U with empty string
2:  44bba1dcd = 2:  44bba1dcd tests/system/tests/test_groupadd.py: add test for groupadd -U with empty string
3:  2e1296d63 = 3:  2e1296d63 lib/, src/: Add blocks
4:  77f368c10 ! 4:  0f31bb63f lib/, src/: Some empty lists have 0 elements, not 1 empty string
    @@ Commit message
     
         In general, empty fields in a CSV are errors.  However, in some cases,
         we want to allow passing empty lists, and the way to encode that is as
    -    an empty string.  This was accidentally broken in the last release, when
    -    we switched from using strtok(3) to strsep(3), without remembering to
    +    an empty string.  This was accidentally broken in 4.17.0, when we
    +    switched from using strtok(3) to strsep(3), without remembering to
         special-case an empty CSV.
     
         The bug affected directly groupadd(8) and groupmod(8).
5:  7f77d0aaa = 5:  a893c004d lib/, src/: Reduce scope of local variables

[hallyn]

Sounds like we should add a test to tests/usertools/usermod/

[alejandro-colomar]

Sounds like we should add a test to tests/usertools/usermod/

This actually doesn't affect usermod(8).

See the programs and configurations which are affected:

    The bug affected directly groupadd(8) and groupmod(8).
    
    The bug also affected the library function add_groups().  In systems
    using PAM, that function is unused.  On systems without PAM, it is
    called by the library function setup_uid_gid(), with the contents of the
    "CONSOLE_GROUPS" configuration (login.defs) CSV string.
    
    setup_uid_gid() is directly called by su(1) and login(1) on systems
    without PAM.
    
    setup_uid_gid() is also called by the library function expire().
    
    expire() is directly called by expiry(1), su(1), and login(1).
    
    This bug is a regression introduced in the release 4.17.0, and present
    in the releases 4.17.{0..4} and 4.18.0.

[alejandro-colomar]

Sounds like we should add a test to tests/usertools/usermod/

This actually doesn't affect usermod(8).

Or maybe it does... I originally only checked code modified by the same commit that introduced the bug in groupmod(8), but maybe there are other faulty commits. I'll revise.

[alejandro-colomar]

Sounds like we should add a test to tests/usertools/usermod/

This actually doesn't affect usermod(8).

Or maybe it does... I originally only checked code modified by the same commit that introduced the bug in groupmod(8), but maybe there are other faulty commits. I'll revise.

I can confirm that usermod(8) is bug-free.

alx@devuan:~$ sudo useradd foo
alx@devuan:~$ sudo groupadd foo1
alx@devuan:~$ sudo groupadd foo2
alx@devuan:~$ sudo groupadd foo3
alx@devuan:~$ sudo usermod -G foo1,foo2,foo3 foo
alx@devuan:~$ cat /etc/group | grep foo
foo:x:1004:
foo1:x:1005:foo
foo2:x:1006:foo
foo3:x:1007:foo
alx@devuan:~$ sudo usermod -G foo1,foo2 foo
alx@devuan:~$ cat /etc/group | grep foo
foo:x:1004:
foo1:x:1005:foo
foo2:x:1006:foo
foo3:x:1007:
alx@devuan:~$ sudo usermod -G "" foo
alx@devuan:~$ cat /etc/group | grep foo
foo:x:1004:
foo1:x:1005:
foo2:x:1006:
foo3:x:1007:

And reading the source code, I can see it has the streq(,"") check, which is consistent with that experiment:

shadow/src/usermod.c

Line 243 in f973c40

if (streq(list, "")) {

[alejandro-colomar]

Sounds like we should add a test to tests/usertools/usermod/

@hallyn

Would you mind adding a test to tests/grouptools/groupmod with the experiment in the first comment in a separate PR? I'm a bit unsure of how those tests work.

We should also test groupadd(8), which should be very similar. If you write the test for groupmod(8), I could imitate it for groupadd(8).

We should also test at least one of su(1), login(1), and/or expiry(1).

[ikerexxe]

I'm a little slow this morning, so forgive me if I ask some very basic questions.

Tested:

alx@devuan:~/src/shadow/shadow/master$ cat /etc/group | grep foo
foo:x:1004:
alx@devuan:~/src/shadow/shadow/master$ sudo ./src/groupmod -U alx foo
alx@devuan:~/src/shadow/shadow/master$ cat /etc/group | grep foo
foo:x:1004:alx
alx@devuan:~/src/shadow/shadow/master$ sudo ./src/groupmod -U "" foo
alx@devuan:~/src/shadow/shadow/master$ cat /etc/group | grep foo
foo:x:1004:

This reproduces the bug in shadow 4.17.0+ and up to this PR. But the bug only reproduces if PAM is not used. Is my understanding correct?

[alejandro-colomar]

I'm a little slow this morning, so forgive me if I ask some very basic questions.

Tested:

alx@devuan:~/src/shadow/shadow/master$ cat /etc/group | grep foo
foo:x:1004:
alx@devuan:~/src/shadow/shadow/master$ sudo ./src/groupmod -U alx foo
alx@devuan:~/src/shadow/shadow/master$ cat /etc/group | grep foo
foo:x:1004:alx
alx@devuan:~/src/shadow/shadow/master$ sudo ./src/groupmod -U "" foo
alx@devuan:~/src/shadow/shadow/master$ cat /etc/group | grep foo
foo:x:1004:

This reproduces the bug in shadow 4.17.0+ and up to this PR. But the bug only reproduces if PAM is not used. Is my understanding correct?

The bugs in groupmod(8) and groupadd(8) occur regardless of PAM. You should be able to reproduce them in PAM systems. The bugs in su(1), login(1), and expiry(1) occur only if PAM.

[ikerexxe]

I added the python tests for groupadd and groupmod in #1427

We should also test at least one of su(1), login(1), and/or expiry(1).

Unfortunately I didn't have time to implement the python wrappers for those binaries yet, so I'm unable to test those for the moment

[alejandro-colomar]

I added the python tests for groupadd and groupmod in #1427

We should also test at least one of su(1), login(1), and/or expiry(1).

Unfortunately I didn't have time to implement the python wrappers for those binaries yet, so I'm unable to test those for the moment

I think those tests should give us enough confidence that this PR is good. It would be nice to have tests for su(1) and login(1) in the future, but I'm happy with this for now.

I don't care at all about expiry(1), TBH. I would like to deprecate that program, and all features about expiration of passwords. I'll open an issue to deprecate it in 4.20, and remove it in 4.21.

[alejandro-colomar]

@ikerexxe I've included your tests in this PR, to show that they pass.

[hallyn]

I'm ok with this, but how do you want to coordinate this with PR 1427?

[alejandro-colomar]

I'm ok with this, but how do you want to coordinate this with PR 1427?

Either merge the other one and then this one immediately, or merge this one, and then the other one would need to be rebased.

[alejandro-colomar]

I'm ok with this, but how do you want to coordinate this with PR 1427?

Either merge the other one and then this one, or merge this one, and then the other one would need to be rebased.

I.e., I don't care too much. :)

[alejandro-colomar]

Actually, I think it's better to merge this first. @hallyn , @ikerexxe

[ikerexxe]

I'm fine with that, but can you remove the tests I developed? Or include all of them, since you only have a partial content of #1427

PS: I prefer the first option

[alejandro-colomar]

I'm fine with that, but can you remove the tests I developed? Or include all of them, since you only have a partial content of #1427

PS: I prefer the first option

You mean that the test tests/system/tests/test_groupmod.py: add test for groupmod -U with user list is missing. However, that test is testing something unrelated to this bug, which is why I left it out, and why I think it should go in a separate PR.

[alejandro-colomar]

I'm fine with that, but can you remove the tests I developed? Or include all of them, since you only have a partial content of #1427
PS: I prefer the first option

You mean that the test tests/system/tests/test_groupmod.py: add test for groupmod -U with user list is missing. However, that test is testing something unrelated to this bug, which is why I left it out, and why I think it should go in a separate PR.

@ikerexxe @hallyn

My ideal preference would be that you split #1472 into two PRs: one that tests this PR (so only the first two commits), and one that adds the other test (and the other "fix", which BTW would benefit from some explanation in the commit message). The unrelated PR should pass CI, since it is testing code that should be okay already. #1472 should indeed fail CI because it's fixed by this PR.

[ikerexxe]

Ok, makes sense. #1427 only contains the tests for the fix in this PR while #1450 contains the other test

[alejandro-colomar]

Ok, makes sense. #1427 only contains the tests for the fix in this PR while #1450 contains the other test

Thanks!

@hallyn please merge first #1472 (which will make CI fail precisely because of the regression we'll be testing) and immediately afterwards this one, which will fix CI by fixing the regression.