[PATCH] support: Don't fail on fchown when spawning sgid processes

[PATCH] support: Don't fail on fchown when spawning sgid processes

[Siddhesh Poyarekar]

In some cases (e.g. when podman creates user containers), the only other
group assigned to the executing user is nobody and fchown fails with it
because the group is not mapped.  Do not fail the test in this case,
instead exit as unsupported.

Reported-by: Frederic Berat <fberat@redhat.com>
Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 support/support_capture_subprocess.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
index bae7d5fb20..ad1da9fd97 100644
--- a/support/support_capture_subprocess.c
+++ b/support/support_capture_subprocess.c
@@ -153,9 +153,15 @@ copy_and_spawn_sgid (char *child_id, gid_t gid)
 	  p += wrcount;
 	}
     }
-  TEST_VERIFY (fchown (outfd, getuid (), gid) == 0);
+
+  int chowned = 0;
+  TEST_VERIFY ((chowned = fchown (outfd, getuid (), gid)) == 0
+	       || errno == EPERM);
   if (support_record_failure_is_failed ())
     goto err;
+  else if (chowned != 0)
+    ret = 77;
+
   TEST_VERIFY (fchmod (outfd, 02750) == 0);
   if (support_record_failure_is_failed ())
     goto err;
@@ -192,8 +198,10 @@ err:
       free (dirname);
     }
 
-  if (ret != 0)
-    FAIL_EXIT1("Failed to make sgid executable for test\n");
+  if (ret == 77)
+    FAIL_UNSUPPORTED ("Failed to make sgid executable for test\n");
+  else if (ret != 0)
+    FAIL_EXIT1 ("Failed to make sgid executable for test\n");
 
   return status;
 }
-- 
2.40.1

Re: [PATCH] support: Don't fail on fchown when spawning sgid processes

[Frederic Berat]

On Wed, May 31, 2023 at 3:34 PM Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
>
> In some cases (e.g. when podman creates user containers), the only other
> group assigned to the executing user is nobody and fchown fails with it
> because the group is not mapped.  Do not fail the test in this case,
> instead exit as unsupported.
>
> Reported-by: Frederic Berat <fberat@redhat.com>
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> ---
>  support/support_capture_subprocess.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
> index bae7d5fb20..ad1da9fd97 100644
> --- a/support/support_capture_subprocess.c
> +++ b/support/support_capture_subprocess.c
> @@ -153,9 +153,15 @@ copy_and_spawn_sgid (char *child_id, gid_t gid)
>           p += wrcount;
>         }
>      }
> -  TEST_VERIFY (fchown (outfd, getuid (), gid) == 0);
> +
> +  int chowned = 0;
> +  TEST_VERIFY ((chowned = fchown (outfd, getuid (), gid)) == 0
> +              || errno == EPERM);
>    if (support_record_failure_is_failed ())
>      goto err;
> +  else if (chowned != 0)
> +    ret = 77;

Shouldn't you "goto err" here instead of continuing the execution ?
During my test the reported reason for UNSUPPORTED is:

"error: tst-secure-getenv.c:78: SGID failed: GID and EGID match (1000)"
instead of the expected "Failed to make sgid executable for test" below.

I assume that is because of the missing goto here.

> +
>    TEST_VERIFY (fchmod (outfd, 02750) == 0);
>    if (support_record_failure_is_failed ())
>      goto err;
> @@ -192,8 +198,10 @@ err:
>        free (dirname);
>      }
>
> -  if (ret != 0)
> -    FAIL_EXIT1("Failed to make sgid executable for test\n");
> +  if (ret == 77)
> +    FAIL_UNSUPPORTED ("Failed to make sgid executable for test\n");
> +  else if (ret != 0)
> +    FAIL_EXIT1 ("Failed to make sgid executable for test\n");

Minor comment: Since FAIL_UNSUPPORTED exits, the "else" seems superfluous.

>
>    return status;
>  }
> --
> 2.40.1
>

Re: [PATCH] support: Don't fail on fchown when spawning sgid processes

[Siddhesh Poyarekar]

On 2023-05-31 10:32, Frederic Berat wrote:
> On Wed, May 31, 2023 at 3:34 PM Siddhesh Poyarekar
> <siddhesh@sourceware.org> wrote:
>>
>> In some cases (e.g. when podman creates user containers), the only other
>> group assigned to the executing user is nobody and fchown fails with it
>> because the group is not mapped.  Do not fail the test in this case,
>> instead exit as unsupported.
>>
>> Reported-by: Frederic Berat <fberat@redhat.com>
>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
>> ---
>>   support/support_capture_subprocess.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
>> index bae7d5fb20..ad1da9fd97 100644
>> --- a/support/support_capture_subprocess.c
>> +++ b/support/support_capture_subprocess.c
>> @@ -153,9 +153,15 @@ copy_and_spawn_sgid (char *child_id, gid_t gid)
>>            p += wrcount;
>>          }
>>       }
>> -  TEST_VERIFY (fchown (outfd, getuid (), gid) == 0);
>> +
>> +  int chowned = 0;
>> +  TEST_VERIFY ((chowned = fchown (outfd, getuid (), gid)) == 0
>> +              || errno == EPERM);
>>     if (support_record_failure_is_failed ())
>>       goto err;
>> +  else if (chowned != 0)
>> +    ret = 77;
> 
> Shouldn't you "goto err" here instead of continuing the execution ?

Oops, yes.

> During my test the reported reason for UNSUPPORTED is:
> 
> "error: tst-secure-getenv.c:78: SGID failed: GID and EGID match (1000)"
> instead of the expected "Failed to make sgid executable for test" below.
> 
> I assume that is because of the missing goto here.
> 
>> +
>>     TEST_VERIFY (fchmod (outfd, 02750) == 0);
>>     if (support_record_failure_is_failed ())
>>       goto err;
>> @@ -192,8 +198,10 @@ err:
>>         free (dirname);
>>       }
>>
>> -  if (ret != 0)
>> -    FAIL_EXIT1("Failed to make sgid executable for test\n");
>> +  if (ret == 77)
>> +    FAIL_UNSUPPORTED ("Failed to make sgid executable for test\n");
>> +  else if (ret != 0)
>> +    FAIL_EXIT1 ("Failed to make sgid executable for test\n");
> 
> Minor comment: Since FAIL_UNSUPPORTED exits, the "else" seems superfluous.

Ack, fixed.  v2 coming up.

Thanks,
Sid

> 
>>
>>     return status;
>>   }
>> --
>> 2.40.1
>>
> 

[PATCH v2] support: Don't fail on fchown when spawning sgid processes

[Siddhesh Poyarekar]

In some cases (e.g. when podman creates user containers), the only other
group assigned to the executing user is nobody and fchown fails with it
because the group is not mapped.  Do not fail the test in this case,
instead exit as unsupported.

Reported-by: Frederic Berat <fberat@redhat.com>
Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 support/support_capture_subprocess.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
index bae7d5fb20..3881e3610a 100644
--- a/support/support_capture_subprocess.c
+++ b/support/support_capture_subprocess.c
@@ -153,9 +153,18 @@ copy_and_spawn_sgid (char *child_id, gid_t gid)
 	  p += wrcount;
 	}
     }
-  TEST_VERIFY (fchown (outfd, getuid (), gid) == 0);
+
+  int chowned = 0;
+  TEST_VERIFY ((chowned = fchown (outfd, getuid (), gid)) == 0
+	       || errno == EPERM);
   if (support_record_failure_is_failed ())
     goto err;
+  else if (chowned != 0)
+    {
+      ret = 77;
+      goto err;
+    }
+
   TEST_VERIFY (fchmod (outfd, 02750) == 0);
   if (support_record_failure_is_failed ())
     goto err;
@@ -192,8 +201,10 @@ err:
       free (dirname);
     }
 
+  if (ret == 77)
+    FAIL_UNSUPPORTED ("Failed to make sgid executable for test\n");
   if (ret != 0)
-    FAIL_EXIT1("Failed to make sgid executable for test\n");
+    FAIL_EXIT1 ("Failed to make sgid executable for test\n");
 
   return status;
 }
-- 
2.40.1

Re: [PATCH v2] support: Don't fail on fchown when spawning sgid processes

[Andreas Schwab]

On Mai 31 2023, Siddhesh Poyarekar via Libc-alpha wrote:

> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
> index bae7d5fb20..3881e3610a 100644
> --- a/support/support_capture_subprocess.c
> +++ b/support/support_capture_subprocess.c
> @@ -153,9 +153,18 @@ copy_and_spawn_sgid (char *child_id, gid_t gid)
>  	  p += wrcount;
>  	}
>      }
> -  TEST_VERIFY (fchown (outfd, getuid (), gid) == 0);
> +
> +  int chowned = 0;

I think that can be bool.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v2] support: Don't fail on fchown when spawning sgid processes

[Frederic Berat]

On Wed, May 31, 2023 at 5:00 PM Siddhesh Poyarekar
<siddhesh@sourceware.org> wrote:
>
> In some cases (e.g. when podman creates user containers), the only other
> group assigned to the executing user is nobody and fchown fails with it
> because the group is not mapped.  Do not fail the test in this case,
> instead exit as unsupported.
>
> Reported-by: Frederic Berat <fberat@redhat.com>
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> ---
>  support/support_capture_subprocess.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
> index bae7d5fb20..3881e3610a 100644
> --- a/support/support_capture_subprocess.c
> +++ b/support/support_capture_subprocess.c
> @@ -153,9 +153,18 @@ copy_and_spawn_sgid (char *child_id, gid_t gid)
>           p += wrcount;
>         }
>      }
> -  TEST_VERIFY (fchown (outfd, getuid (), gid) == 0);
> +
> +  int chowned = 0;
> +  TEST_VERIFY ((chowned = fchown (outfd, getuid (), gid)) == 0
> +              || errno == EPERM);
>    if (support_record_failure_is_failed ())
>      goto err;
> +  else if (chowned != 0)
> +    {
> +      ret = 77;
> +      goto err;
> +    }
> +
>    TEST_VERIFY (fchmod (outfd, 02750) == 0);
>    if (support_record_failure_is_failed ())
>      goto err;
> @@ -192,8 +201,10 @@ err:
>        free (dirname);
>      }
>
> +  if (ret == 77)
> +    FAIL_UNSUPPORTED ("Failed to make sgid executable for test\n");
>    if (ret != 0)
> -    FAIL_EXIT1("Failed to make sgid executable for test\n");
> +    FAIL_EXIT1 ("Failed to make sgid executable for test\n");
>
>    return status;
>  }
> --
> 2.40.1
>

Looks good to me:
$> cat stdlib/tst-secure-getenv.test-result && cat stdlib/tst-secure-getenv.out
UNSUPPORTED: stdlib/tst-secure-getenv
original exit status 77
error: support_capture_subprocess.c:205: Failed to make sgid executable for test

Tested-by: Frédéric Bérat <fberat@redhat.com>

[PATCH v3] support: Don't fail on fchown when spawning sgid processes

[Siddhesh Poyarekar]

In some cases (e.g. when podman creates user containers), the only other
group assigned to the executing user is nobody and fchown fails with it
because the group is not mapped.  Do not fail the test in this case,
instead exit as unsupported.

Reported-by: Frédéric Bérat <fberat@redhat.com>
Tested-by: Frédéric Bérat <fberat@redhat.com>
Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---
 support/support_capture_subprocess.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
index bae7d5fb20..2a8d37b284 100644
--- a/support/support_capture_subprocess.c
+++ b/support/support_capture_subprocess.c
@@ -153,9 +153,18 @@ copy_and_spawn_sgid (char *child_id, gid_t gid)
 	  p += wrcount;
 	}
     }
-  TEST_VERIFY (fchown (outfd, getuid (), gid) == 0);
+
+  bool chowned = false;
+  TEST_VERIFY ((chowned = fchown (outfd, getuid (), gid) == 0)
+	       || errno == EPERM);
   if (support_record_failure_is_failed ())
     goto err;
+  else if (!chowned)
+    {
+      ret = 77;
+      goto err;
+    }
+
   TEST_VERIFY (fchmod (outfd, 02750) == 0);
   if (support_record_failure_is_failed ())
     goto err;
@@ -192,8 +201,10 @@ err:
       free (dirname);
     }
 
+  if (ret == 77)
+    FAIL_UNSUPPORTED ("Failed to make sgid executable for test\n");
   if (ret != 0)
-    FAIL_EXIT1("Failed to make sgid executable for test\n");
+    FAIL_EXIT1 ("Failed to make sgid executable for test\n");
 
   return status;
 }
-- 
2.40.1

Re: [PATCH v2] support: Don't fail on fchown when spawning sgid processes

[Siddhesh Poyarekar]

On 2023-05-31 11:13, Andreas Schwab wrote:
> On Mai 31 2023, Siddhesh Poyarekar via Libc-alpha wrote:
> 
>> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
>> index bae7d5fb20..3881e3610a 100644
>> --- a/support/support_capture_subprocess.c
>> +++ b/support/support_capture_subprocess.c
>> @@ -153,9 +153,18 @@ copy_and_spawn_sgid (char *child_id, gid_t gid)
>>   	  p += wrcount;
>>   	}
>>       }
>> -  TEST_VERIFY (fchown (outfd, getuid (), gid) == 0);
>> +
>> +  int chowned = 0;
> 
> I think that can be bool.
> 

Thanks, fixed in v3.

Sid

Re: [PATCH v3] support: Don't fail on fchown when spawning sgid processes

[Carlos O'Donell]

On 5/31/23 12:07, Siddhesh Poyarekar wrote:
> In some cases (e.g. when podman creates user containers), the only other
> group assigned to the executing user is nobody and fchown fails with it
> because the group is not mapped.  Do not fail the test in this case,
> instead exit as unsupported.
> 
> Reported-by: Frédéric Bérat <fberat@redhat.com>
> Tested-by: Frédéric Bérat <fberat@redhat.com>
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  support/support_capture_subprocess.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
> index bae7d5fb20..2a8d37b284 100644
> --- a/support/support_capture_subprocess.c
> +++ b/support/support_capture_subprocess.c
> @@ -153,9 +153,18 @@ copy_and_spawn_sgid (char *child_id, gid_t gid)
>  	  p += wrcount;
>  	}
>      }
> -  TEST_VERIFY (fchown (outfd, getuid (), gid) == 0);
> +
> +  bool chowned = false;
> +  TEST_VERIFY ((chowned = fchown (outfd, getuid (), gid) == 0)
> +	       || errno == EPERM);
>    if (support_record_failure_is_failed ())
>      goto err;
> +  else if (!chowned)
> +    {
> +      ret = 77;
> +      goto err;
> +    }
> +
>    TEST_VERIFY (fchmod (outfd, 02750) == 0);
>    if (support_record_failure_is_failed ())
>      goto err;
> @@ -192,8 +201,10 @@ err:
>        free (dirname);
>      }
>  
> +  if (ret == 77)
> +    FAIL_UNSUPPORTED ("Failed to make sgid executable for test\n");
>    if (ret != 0)
> -    FAIL_EXIT1("Failed to make sgid executable for test\n");
> +    FAIL_EXIT1 ("Failed to make sgid executable for test\n");
>  
>    return status;
>  }

-- 
Cheers,
Carlos.

Re: [PATCH v3] support: Don't fail on fchown when spawning sgid processes

[Carlos O'Donell]

On 6/1/23 07:01, Carlos O'Donell wrote:
> On 5/31/23 12:07, Siddhesh Poyarekar wrote:
>> In some cases (e.g. when podman creates user containers), the only other
>> group assigned to the executing user is nobody and fchown fails with it
>> because the group is not mapped.  Do not fail the test in this case,
>> instead exit as unsupported.
>>
>> Reported-by: Frédéric Bérat <fberat@redhat.com>
>> Tested-by: Frédéric Bérat <fberat@redhat.com>
>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> 
> LGTM.
> 
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>

I have filed an RFE with distrobox upstream. The podman wrapper tools can and should
be able to map all supplementary gid's into the container so that filesystem access
works correctly. In some cases supplementary groups could be critical to user access
of files on disk.

Map all supplementary groups into the distro container.
https://github.com/89luca89/distrobox/issues/777

>> ---
>>  support/support_capture_subprocess.c | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
>> index bae7d5fb20..2a8d37b284 100644
>> --- a/support/support_capture_subprocess.c
>> +++ b/support/support_capture_subprocess.c
>> @@ -153,9 +153,18 @@ copy_and_spawn_sgid (char *child_id, gid_t gid)
>>  	  p += wrcount;
>>  	}
>>      }
>> -  TEST_VERIFY (fchown (outfd, getuid (), gid) == 0);
>> +
>> +  bool chowned = false;
>> +  TEST_VERIFY ((chowned = fchown (outfd, getuid (), gid) == 0)
>> +	       || errno == EPERM);
>>    if (support_record_failure_is_failed ())
>>      goto err;
>> +  else if (!chowned)
>> +    {
>> +      ret = 77;
>> +      goto err;
>> +    }
>> +
>>    TEST_VERIFY (fchmod (outfd, 02750) == 0);
>>    if (support_record_failure_is_failed ())
>>      goto err;
>> @@ -192,8 +201,10 @@ err:
>>        free (dirname);
>>      }
>>  
>> +  if (ret == 77)
>> +    FAIL_UNSUPPORTED ("Failed to make sgid executable for test\n");
>>    if (ret != 0)
>> -    FAIL_EXIT1("Failed to make sgid executable for test\n");
>> +    FAIL_EXIT1 ("Failed to make sgid executable for test\n");
>>  
>>    return status;
>>  }
> 

-- 
Cheers,
Carlos.

Re: [PATCH v3] support: Don't fail on fchown when spawning sgid processes

[Carlos O'Donell]

On 6/1/23 07:22, Carlos O'Donell wrote:
> On 6/1/23 07:01, Carlos O'Donell wrote:
>> On 5/31/23 12:07, Siddhesh Poyarekar wrote:
>>> In some cases (e.g. when podman creates user containers), the only other
>>> group assigned to the executing user is nobody and fchown fails with it
>>> because the group is not mapped.  Do not fail the test in this case,
>>> instead exit as unsupported.
>>>
>>> Reported-by: Frédéric Bérat <fberat@redhat.com>
>>> Tested-by: Frédéric Bérat <fberat@redhat.com>
>>> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
>>
>> LGTM.
>>
>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 
> I have filed an RFE with distrobox upstream. The podman wrapper tools can and should
> be able to map all supplementary gid's into the container so that filesystem access
> works correctly. In some cases supplementary groups could be critical to user access
> of files on disk.
> 
> Map all supplementary groups into the distro container.
> https://github.com/89luca89/distrobox/issues/777

For reference here, toolbx works because they map 'wheel' as the secondary group in the container
rather than nobody.


>>> ---
>>>  support/support_capture_subprocess.c | 15 +++++++++++++--
>>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
>>> index bae7d5fb20..2a8d37b284 100644
>>> --- a/support/support_capture_subprocess.c
>>> +++ b/support/support_capture_subprocess.c
>>> @@ -153,9 +153,18 @@ copy_and_spawn_sgid (char *child_id, gid_t gid)
>>>  	  p += wrcount;
>>>  	}
>>>      }
>>> -  TEST_VERIFY (fchown (outfd, getuid (), gid) == 0);
>>> +
>>> +  bool chowned = false;
>>> +  TEST_VERIFY ((chowned = fchown (outfd, getuid (), gid) == 0)
>>> +	       || errno == EPERM);
>>>    if (support_record_failure_is_failed ())
>>>      goto err;
>>> +  else if (!chowned)
>>> +    {
>>> +      ret = 77;
>>> +      goto err;
>>> +    }
>>> +
>>>    TEST_VERIFY (fchmod (outfd, 02750) == 0);
>>>    if (support_record_failure_is_failed ())
>>>      goto err;
>>> @@ -192,8 +201,10 @@ err:
>>>        free (dirname);
>>>      }
>>>  
>>> +  if (ret == 77)
>>> +    FAIL_UNSUPPORTED ("Failed to make sgid executable for test\n");
>>>    if (ret != 0)
>>> -    FAIL_EXIT1("Failed to make sgid executable for test\n");
>>> +    FAIL_EXIT1 ("Failed to make sgid executable for test\n");
>>>  
>>>    return status;
>>>  }
>>
> 

-- 
Cheers,
Carlos.

Re: [PATCH v3] support: Don't fail on fchown when spawning sgid processes

[Siddhesh Poyarekar]

On 2023-06-01 07:33, Carlos O'Donell wrote:
>> I have filed an RFE with distrobox upstream. The podman wrapper tools can and should
>> be able to map all supplementary gid's into the container so that filesystem access
>> works correctly. In some cases supplementary groups could be critical to user access
>> of files on disk.
>>
>> Map all supplementary groups into the distro container.
>> https://github.com/89luca89/distrobox/issues/777
> 
> For reference here, toolbx works because they map 'wheel' as the secondary group in the container
> rather than nobody.
> 

This is interesting because wheel would allow the container user to do 
things like package installation.  I reckon podman (under the distrobox 
layer) adds the user to the sudo group to allow it to do various tasks 
using sudo...

Sid