[monit-dev] "check program" change uid preliminary patch

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[monit-dev] "check program" change uid preliminary patch

Leif Gustafson-2
Hello,
  Following up on the conversation in monit-general about adding an option
to "check program" to drop root privileges, I took a look at the source code
and tried to see how difficult it would be to add it.  I'm not extremely
familiar with the source tree nor do I usually use C for my day job so be
gentle, but I was able to mock up a preliminary working patch (against
5.3.2).  I think this is a pretty important security feature for "check
program" because if the user isn't careful a malicious trojan could be
executed as root.

  I had to make a couple of workarounds to get this to work.  The "check"
statements appear to be designed to only take one parameter (in this case,
the path to the program), so I had to modify the grammar of "check program"
a bit.  But also, the order in which the statement is being parsed means the
Service_t hasn't been created yet when uid/gid are parsed.  So, I have to
save those in static global variables and then set them in the instance of
Service_t once it has been created.

  So, testing this with something like "check program test-script with path
'/home/lgustafson/test-uid.sh' as uid 1000 as gid 1000 if status != 0 then
alert" worked for me.  The script was indeed run as my uid/gid.  The caveat
is the supplemental group list still contains root, so the script could
still modify writeable files owned by gid 0.  I'm not certain that there is
a portable solution for that, but it could be resolved on some of the
platforms you support.

  Like I said, this is just a preliminary patch/brainstorming exercise.

_______________________________________________
monit-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/monit-dev

check-program-set-uid.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [monit-dev] "check program" change uid preliminary patch

Jan-Henrik Haukeland
Hi

Looks like a fine start to me. Thanks for the work. It should be possible to avoid the global stuff and use the local struct myprogram programset object. I'll take a look and add the refactored patch.


On Mar 16, 2012, at 5:11 AM, Leif Gustafson wrote:

> Hello,
>  Following up on the conversation in monit-general about adding an option
> to "check program" to drop root privileges, I took a look at the source code
> and tried to see how difficult it would be to add it.  I'm not extremely
> familiar with the source tree nor do I usually use C for my day job so be
> gentle, but I was able to mock up a preliminary working patch (against
> 5.3.2).  I think this is a pretty important security feature for "check
> program" because if the user isn't careful a malicious trojan could be
> executed as root.
>
>  I had to make a couple of workarounds to get this to work.  The "check"
> statements appear to be designed to only take one parameter (in this case,
> the path to the program), so I had to modify the grammar of "check program"
> a bit.  But also, the order in which the statement is being parsed means the
> Service_t hasn't been created yet when uid/gid are parsed.  So, I have to
> save those in static global variables and then set them in the instance of
> Service_t once it has been created.
>
>  So, testing this with something like "check program test-script with path
> '/home/lgustafson/test-uid.sh' as uid 1000 as gid 1000 if status != 0 then
> alert" worked for me.  The script was indeed run as my uid/gid.  The caveat
> is the supplemental group list still contains root, so the script could
> still modify writeable files owned by gid 0.  I'm not certain that there is
> a portable solution for that, but it could be resolved on some of the
> platforms you support.
>
>  Like I said, this is just a preliminary patch/brainstorming exercise.
> <check-program-set-uid.patch>_______________________________________________
> monit-dev mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/monit-dev


_______________________________________________
monit-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/monit-dev
Reply | Threaded
Open this post in threaded view
|

Re: [monit-dev] "check program" change uid preliminary patch

Michael Shigorin
In reply to this post by Leif Gustafson-2
On Thu, Mar 15, 2012 at 08:11:26PM -0800, Leif Gustafson wrote:
> Like I said, this is just a preliminary patch/brainstorming exercise.

I might ask Openwall folks what they think of monit in general,
and they have a habit for privsep.

--
 ---- WBR, Michael Shigorin <[hidden email]>
  ------ Linux.Kiev http://www.linux.kiev.ua/

_______________________________________________
monit-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/monit-dev
Reply | Threaded
Open this post in threaded view
|

Re: [monit-dev] "check program" change uid preliminary patch

Igor Homyakov-2
In reply to this post by Jan-Henrik Haukeland
Hi guys,

Is it really necessary to re-implement su/sudo functionality ?  I
don't think so.  From my point of view we've got over-engineering
here.  I believe that su developers are smart enough to deal with
security/race issues.

So "check program su username -c command" is more secure, more
flexible and more simple way  to do that.

Kind regards
Igor Homyakov

On Fri, Mar 16, 2012 at 11:11, Jan-Henrik Haukeland <[hidden email]> wrote:

> Hi
>
> Looks like a fine start to me. Thanks for the work. It should be possible to avoid the global stuff and use the local struct myprogram programset object. I'll take a look and add the refactored patch.
>
>
> On Mar 16, 2012, at 5:11 AM, Leif Gustafson wrote:
>
>> Hello,
>>  Following up on the conversation in monit-general about adding an option
>> to "check program" to drop root privileges, I took a look at the source code
>> and tried to see how difficult it would be to add it.  I'm not extremely
>> familiar with the source tree nor do I usually use C for my day job so be
>> gentle, but I was able to mock up a preliminary working patch (against
>> 5.3.2).  I think this is a pretty important security feature for "check
>> program" because if the user isn't careful a malicious trojan could be
>> executed as root.
>>
>>  I had to make a couple of workarounds to get this to work.  The "check"
>> statements appear to be designed to only take one parameter (in this case,
>> the path to the program), so I had to modify the grammar of "check program"
>> a bit.  But also, the order in which the statement is being parsed means the
>> Service_t hasn't been created yet when uid/gid are parsed.  So, I have to
>> save those in static global variables and then set them in the instance of
>> Service_t once it has been created.
>>
>>  So, testing this with something like "check program test-script with path
>> '/home/lgustafson/test-uid.sh' as uid 1000 as gid 1000 if status != 0 then
>> alert" worked for me.  The script was indeed run as my uid/gid.  The caveat
>> is the supplemental group list still contains root, so the script could
>> still modify writeable files owned by gid 0.  I'm not certain that there is
>> a portable solution for that, but it could be resolved on some of the
>> platforms you support.
>>
>>  Like I said, this is just a preliminary patch/brainstorming exercise.
>> <check-program-set-uid.patch>_______________________________________________
>> monit-dev mailing list
>> [hidden email]
>> https://lists.nongnu.org/mailman/listinfo/monit-dev
>
>
> _______________________________________________
> monit-dev mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/monit-dev

_______________________________________________
monit-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/monit-dev
Reply | Threaded
Open this post in threaded view
|

Re: [monit-dev] "check program" change uid preliminary patch

Leif Gustafson-2
That's a very good point.  The problem now is that the argument to
"check program" is only allowed to be a single executable file.  That's
already an annoying limitation, and if it were fixed we'd be able to
just use su directly as you suggested.


On 03/16/2012 4:14 AM, Igor Homyakov wrote:

> Hi guys,
>
> Is it really necessary to re-implement su/sudo functionality ?  I
> don't think so.  From my point of view we've got over-engineering
> here.  I believe that su developers are smart enough to deal with
> security/race issues.
>
> So "check program su username -c command" is more secure, more
> flexible and more simple way  to do that.
>
> Kind regards
> Igor Homyakov
>
> On Fri, Mar 16, 2012 at 11:11, Jan-Henrik Haukeland<[hidden email]>  wrote:
>> Hi
>>
>> Looks like a fine start to me. Thanks for the work. It should be possible to avoid the global stuff and use the local struct myprogram programset object. I'll take a look and add the refactored patch.
>>
>>
>> On Mar 16, 2012, at 5:11 AM, Leif Gustafson wrote:
>>
>>> Hello,
>>>   Following up on the conversation in monit-general about adding an option
>>> to "check program" to drop root privileges, I took a look at the source code
>>> and tried to see how difficult it would be to add it.  I'm not extremely
>>> familiar with the source tree nor do I usually use C for my day job so be
>>> gentle, but I was able to mock up a preliminary working patch (against
>>> 5.3.2).  I think this is a pretty important security feature for "check
>>> program" because if the user isn't careful a malicious trojan could be
>>> executed as root.
>>>
>>>   I had to make a couple of workarounds to get this to work.  The "check"
>>> statements appear to be designed to only take one parameter (in this case,
>>> the path to the program), so I had to modify the grammar of "check program"
>>> a bit.  But also, the order in which the statement is being parsed means the
>>> Service_t hasn't been created yet when uid/gid are parsed.  So, I have to
>>> save those in static global variables and then set them in the instance of
>>> Service_t once it has been created.
>>>
>>>   So, testing this with something like "check program test-script with path
>>> '/home/lgustafson/test-uid.sh' as uid 1000 as gid 1000 if status != 0 then
>>> alert" worked for me.  The script was indeed run as my uid/gid.  The caveat
>>> is the supplemental group list still contains root, so the script could
>>> still modify writeable files owned by gid 0.  I'm not certain that there is
>>> a portable solution for that, but it could be resolved on some of the
>>> platforms you support.
>>>
>>>   Like I said, this is just a preliminary patch/brainstorming exercise.
>>> <check-program-set-uid.patch>_______________________________________________
>>> monit-dev mailing list
>>> [hidden email]
>>> https://lists.nongnu.org/mailman/listinfo/monit-dev
>>
>> _______________________________________________
>> monit-dev mailing list
>> [hidden email]
>> https://lists.nongnu.org/mailman/listinfo/monit-dev
> _______________________________________________
> monit-dev mailing list
> [hidden email]
> https://lists.nongnu.org/mailman/listinfo/monit-dev
>

_______________________________________________
monit-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/monit-dev