[DMD] [PATCH] service: Change gid before uid when dropping privileges.

[DMD] [PATCH] service: Change gid before uid when dropping privileges.

[David Thompson]

[-- Attachment #1: Type: text/plain, Size: 96 bytes --]

Found this little order of operations issue when trying to button up the
Transmission service.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-service-Change-gid-before-uid-when-dropping-privileg.patch --]
[-- Type: text/x-patch, Size: 1650 bytes --]

From 552eafe882e3c059525e79c0f222becb9d73eb93 Mon Sep 17 00:00:00 2001
From: David Thompson <dthompson2@worcester.edu>
Date: Thu, 3 Dec 2015 10:09:34 -0500
Subject: [PATCH] service: Change gid before uid when dropping privileges.

---
 modules/dmd/service.scm | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/modules/dmd/service.scm b/modules/dmd/service.scm
index aece069..34826b1 100644
--- a/modules/dmd/service.scm
+++ b/modules/dmd/service.scm
@@ -614,23 +614,25 @@ false."
            (catch-system-error (close-fdes i))
            (loop (+ i 1)))))
 
-     (when user
+     ;; setgid must be done *before* setuid, otherwise the user will
+     ;; likely no longer have permissions to setgid.
+     (when group
        (catch #t
          (lambda ()
-           (setuid (passwd:uid (getpw user))))
+           (setgid (group:gid (getgr group))))
          (lambda (key . args)
            (format (current-error-port)
-                   "failed to change to user ~s:~%" user)
+                   "failed to change to group ~s:~%" group)
            (print-exception (current-error-port) #f key args)
            (primitive-exit 1))))
 
-     (when group
+     (when user
        (catch #t
          (lambda ()
-           (setgid (group:gid (getgr group))))
+           (setuid (passwd:uid (getpw user))))
          (lambda (key . args)
            (format (current-error-port)
-                   "failed to change to group ~s:~%" group)
+                   "failed to change to user ~s:~%" user)
            (print-exception (current-error-port) #f key args)
            (primitive-exit 1))))
 
-- 
2.5.0


[-- Attachment #3: Type: text/plain, Size: 38 bytes --]


-- 
David Thompson
GPG Key: 0FF1D807

Re: [DMD] [PATCH] service: Change gid before uid when dropping privileges.

[Ricardo Wurmus]

David Thompson <dthompson2@worcester.edu> writes:

> Found this little order of operations issue when trying to button up the
> Transmission service.

Looks fine to me.

~~ Ricardo

Re: [DMD] [PATCH] service: Change gid before uid when dropping privileges.

[Ludovic Courtès]

David Thompson <dthompson2@worcester.edu> skribis:

> Found this little order of operations issue when trying to button up the
> Transmission service.

Indeed, good catch!

> From 552eafe882e3c059525e79c0f222becb9d73eb93 Mon Sep 17 00:00:00 2001
> From: David Thompson <dthompson2@worcester.edu>
> Date: Thu, 3 Dec 2015 10:09:34 -0500
> Subject: [PATCH] service: Change gid before uid when dropping privileges.
>

Commit log please.  :-)

Could you send an updated patch?

I think we should also do:

  (setgroups #())

before calling ‘setgid’, and probably factorize exception handling.

Would you like to do that?

Thank you,
Ludo’.

Re: [DMD] [PATCH] service: Change gid before uid when dropping privileges.

[Thompson, David]

On Fri, Dec 4, 2015 at 3:00 AM, Ricardo Wurmus
<ricardo.wurmus@mdc-berlin.de> wrote:
>
> David Thompson <dthompson2@worcester.edu> writes:
>
>> Found this little order of operations issue when trying to button up the
>> Transmission service.
>
> Looks fine to me.

Thanks.  Now I need someone with commit access to dmd to apply it. :)

- Dave

Re: [DMD] [PATCH] service: Change gid before uid when dropping privileges.

[Ludovic Courtès]

"Thompson, David" <dthompson2@worcester.edu> skribis:

> On Fri, Dec 4, 2015 at 3:00 AM, Ricardo Wurmus
> <ricardo.wurmus@mdc-berlin.de> wrote:
>>
>> David Thompson <dthompson2@worcester.edu> writes:
>>
>>> Found this little order of operations issue when trying to button up the
>>> Transmission service.
>>
>> Looks fine to me.
>
> Thanks.  Now I need someone with commit access to dmd to apply it. :)

Good point, should be fixed now!  (With the admin bit, even.  ;-))

Please take a look at the comments I made earlier.

Thanks,
Ludo’.

Re: [DMD] [PATCH] service: Change gid before uid when dropping privileges.

[Thompson, David]

On Sun, Dec 6, 2015 at 4:56 PM, Ludovic Courtès <ludo@gnu.org> wrote:
> "Thompson, David" <dthompson2@worcester.edu> skribis:
>
>> On Fri, Dec 4, 2015 at 3:00 AM, Ricardo Wurmus
>> <ricardo.wurmus@mdc-berlin.de> wrote:
>>>
>>> David Thompson <dthompson2@worcester.edu> writes:
>>>
>>>> Found this little order of operations issue when trying to button up the
>>>> Transmission service.
>>>
>>> Looks fine to me.
>>
>> Thanks.  Now I need someone with commit access to dmd to apply it. :)
>
> Good point, should be fixed now!  (With the admin bit, even.  ;-))

Thank you, Ludo.

> Please take a look at the comments I made earlier.

Noted.  You just beat me to addressing them. :)

- Dave

Re: [DMD] [PATCH] service: Change gid before uid when dropping privileges.

[Thompson, David]

On Mon, Dec 7, 2015 at 8:22 AM, Thompson, David
<dthompson2@worcester.edu> wrote:
> On Sun, Dec 6, 2015 at 4:56 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>> "Thompson, David" <dthompson2@worcester.edu> skribis:
>>
>>> On Fri, Dec 4, 2015 at 3:00 AM, Ricardo Wurmus
>>> <ricardo.wurmus@mdc-berlin.de> wrote:
>>>>
>>>> David Thompson <dthompson2@worcester.edu> writes:
>>>>
>>>>> Found this little order of operations issue when trying to button up the
>>>>> Transmission service.
>>>>
>>>> Looks fine to me.
>>>
>>> Thanks.  Now I need someone with commit access to dmd to apply it. :)
>>
>> Good point, should be fixed now!  (With the admin bit, even.  ;-))
>
> Thank you, Ludo.
>
>> Please take a look at the comments I made earlier.
>
> Noted.  You just beat me to addressing them. :)

Oh, nevermind.  I thought that you fixed my commit for me, but you
just gave me commit access.  ;)

Thanks!

- Dave

Re: [DMD] [PATCH] service: Change gid before uid when dropping privileges.

[Ludovic Courtès]

"Thompson, David" <dthompson2@worcester.edu> skribis:

> On Mon, Dec 7, 2015 at 8:22 AM, Thompson, David
> <dthompson2@worcester.edu> wrote:
>> On Sun, Dec 6, 2015 at 4:56 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>>> "Thompson, David" <dthompson2@worcester.edu> skribis:
>>>
>>>> On Fri, Dec 4, 2015 at 3:00 AM, Ricardo Wurmus
>>>> <ricardo.wurmus@mdc-berlin.de> wrote:
>>>>>
>>>>> David Thompson <dthompson2@worcester.edu> writes:
>>>>>
>>>>>> Found this little order of operations issue when trying to button up the
>>>>>> Transmission service.
>>>>>
>>>>> Looks fine to me.
>>>>
>>>> Thanks.  Now I need someone with commit access to dmd to apply it. :)
>>>
>>> Good point, should be fixed now!  (With the admin bit, even.  ;-))
>>
>> Thank you, Ludo.
>>
>>> Please take a look at the comments I made earlier.
>>
>> Noted.  You just beat me to addressing them. :)
>
> Oh, nevermind.  I thought that you fixed my commit for me, but you
> just gave me commit access.  ;)

There’s no free lunch.  :-)

Ludo’.

Re: [DMD] [PATCH] service: Change gid before uid when dropping privileges.

[Ludovic Courtès]

David Thompson <dthompson2@worcester.edu> skribis:

> From 552eafe882e3c059525e79c0f222becb9d73eb93 Mon Sep 17 00:00:00 2001
> From: David Thompson <dthompson2@worcester.edu>
> Date: Thu, 3 Dec 2015 10:09:34 -0500
> Subject: [PATCH] service: Change gid before uid when dropping privileges.

Pushed with a commit log.

Thanks,
Ludo’.