[RFC PATCH] backup: Set owner of /var/ipfire/backup/{in,ex}clude to "root"

[RFC PATCH] backup: Set owner of /var/ipfire/backup/{in,ex}clude to "root"

[Peter Müller]

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

Since these files are static, there is no legitimate reason why they
should be owned (hence writable) by "nobody".

Signed-off-by: Peter Müller <peter.mueller(a)ipfire.org>
---
 lfs/backup | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lfs/backup b/lfs/backup
index 6f686bf22..adbf16e65 100644
--- a/lfs/backup
+++ b/lfs/backup
@@ -1,7 +1,7 @@
 ###############################################################################
 #                                                                             #
 # IPFire.org - A linux based firewall                                         #
-# Copyright (C) 2007-2021  IPFire Team  <info(a)ipfire.org>                     #
+# Copyright (C) 2007-2022  IPFire Team  <info(a)ipfire.org>                     #
 #                                                                             #
 # This program is free software: you can redistribute it and/or modify        #
 # it under the terms of the GNU General Public License as published by        #
@@ -61,8 +61,8 @@ $(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects))
 	@$(PREBUILD)
 	-mkdir -p /var/ipfire/backup/bin
 	install -v -m 755 -o root $(DIR_SRC)/config/backup/backup.pl /var/ipfire/backup/bin
-	install -v -m 644 $(DIR_SRC)/config/backup/include /var/ipfire/backup/
-	install -v -m 644 $(DIR_SRC)/config/backup/exclude /var/ipfire/backup/
+	install -v -m 644 -o root $(DIR_SRC)/config/backup/include /var/ipfire/backup/
+	install -v -m 644 -o root $(DIR_SRC)/config/backup/exclude /var/ipfire/backup/
 	chown nobody:nobody -R /var/ipfire/backup/
 	chown root:root -R /var/ipfire/backup/bin/
 	-mkdir -p /var/ipfire/backup/addons
-- 
2.35.3

Re: [RFC PATCH] backup: Set owner of /var/ipfire/backup/{in, ex}clude to "root"

[Michael Tremer]

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

Hello Peter,

I agree that the files should be owned by root. However, your patch doesn’t fix that.

> On 15 Sep 2022, at 21:15, Peter Müller <peter.mueller(a)ipfire.org> wrote:
> 
> Since these files are static, there is no legitimate reason why they
> should be owned (hence writable) by "nobody".
> 
> Signed-off-by: Peter Müller <peter.mueller(a)ipfire.org>
> ---
> lfs/backup | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lfs/backup b/lfs/backup
> index 6f686bf22..adbf16e65 100644
> --- a/lfs/backup
> +++ b/lfs/backup
> @@ -1,7 +1,7 @@
> ###############################################################################
> #                                                                             #
> # IPFire.org - A linux based firewall                                         #
> -# Copyright (C) 2007-2021  IPFire Team  <info(a)ipfire.org>                     #
> +# Copyright (C) 2007-2022  IPFire Team  <info(a)ipfire.org>                     #
> #                                                                             #
> # This program is free software: you can redistribute it and/or modify        #
> # it under the terms of the GNU General Public License as published by        #
> @@ -61,8 +61,8 @@ $(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects))
> 	@$(PREBUILD)
> 	-mkdir -p /var/ipfire/backup/bin
> 	install -v -m 755 -o root $(DIR_SRC)/config/backup/backup.pl /var/ipfire/backup/bin
> -	install -v -m 644 $(DIR_SRC)/config/backup/include /var/ipfire/backup/
> -	install -v -m 644 $(DIR_SRC)/config/backup/exclude /var/ipfire/backup/
> +	install -v -m 644 -o root $(DIR_SRC)/config/backup/include /var/ipfire/backup/
> +	install -v -m 644 -o root $(DIR_SRC)/config/backup/exclude /var/ipfire/backup/

They have been created as root before. That is the default.

> 	chown nobody:nobody -R /var/ipfire/backup/

And here is where they will be changed. Still.

> 	chown root:root -R /var/ipfire/backup/bin/
> 	-mkdir -p /var/ipfire/backup/addons
> -- 
> 2.35.3

-Michael

Re: [RFC PATCH] backup: Set owner of /var/ipfire/backup/{in, ex}clude to "root"

[Peter Müller]

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

Hello Michael,

thanks for your reply. Indeed, glad you caught that.

Before I submit a second version: Shouldn't the {in,ex}clude.user files also be owned
by root? I was unable to find any instance in the source code where these are modified
by an unprivileged user.

On that note, is it intended/desired that many subfolders of /var/ipfire/ are owned
by "nobody"? While I of course see the need for "nobody" to write _files_, do not quite
get why the parent folders (such as /var/ipfire/auth/, /var/ipfire/ca/, etc. pp.) have
to be owned by that user as well.

Thanks, and best regards,
Peter Müller


> Hello Peter,
> 
> I agree that the files should be owned by root. However, your patch doesn’t fix that.
> 
>> On 15 Sep 2022, at 21:15, Peter Müller <peter.mueller(a)ipfire.org> wrote:
>>
>> Since these files are static, there is no legitimate reason why they
>> should be owned (hence writable) by "nobody".
>>
>> Signed-off-by: Peter Müller <peter.mueller(a)ipfire.org>
>> ---
>> lfs/backup | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/lfs/backup b/lfs/backup
>> index 6f686bf22..adbf16e65 100644
>> --- a/lfs/backup
>> +++ b/lfs/backup
>> @@ -1,7 +1,7 @@
>> ###############################################################################
>> #                                                                             #
>> # IPFire.org - A linux based firewall                                         #
>> -# Copyright (C) 2007-2021  IPFire Team  <info(a)ipfire.org>                     #
>> +# Copyright (C) 2007-2022  IPFire Team  <info(a)ipfire.org>                     #
>> #                                                                             #
>> # This program is free software: you can redistribute it and/or modify        #
>> # it under the terms of the GNU General Public License as published by        #
>> @@ -61,8 +61,8 @@ $(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects))
>> 	@$(PREBUILD)
>> 	-mkdir -p /var/ipfire/backup/bin
>> 	install -v -m 755 -o root $(DIR_SRC)/config/backup/backup.pl /var/ipfire/backup/bin
>> -	install -v -m 644 $(DIR_SRC)/config/backup/include /var/ipfire/backup/
>> -	install -v -m 644 $(DIR_SRC)/config/backup/exclude /var/ipfire/backup/
>> +	install -v -m 644 -o root $(DIR_SRC)/config/backup/include /var/ipfire/backup/
>> +	install -v -m 644 -o root $(DIR_SRC)/config/backup/exclude /var/ipfire/backup/
> 
> They have been created as root before. That is the default.
> 
>> 	chown nobody:nobody -R /var/ipfire/backup/
> 
> And here is where they will be changed. Still.
> 
>> 	chown root:root -R /var/ipfire/backup/bin/
>> 	-mkdir -p /var/ipfire/backup/addons
>> -- 
>> 2.35.3
> 
> -Michael

Re: [RFC PATCH] backup: Set owner of /var/ipfire/backup/{in, ex}clude to "root"

[Michael Tremer]

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

Hello,

> On 17 Sep 2022, at 12:17, Peter Müller <peter.mueller(a)ipfire.org> wrote:
> 
> Hello Michael,
> 
> thanks for your reply. Indeed, glad you caught that.
> 
> Before I submit a second version: Shouldn't the {in,ex}clude.user files also be owned
> by root? I was unable to find any instance in the source code where these are modified
> by an unprivileged user.

Yes.

> On that note, is it intended/desired that many subfolders of /var/ipfire/ are owned
> by "nobody"? While I of course see the need for "nobody" to write _files_, do not quite
> get why the parent folders (such as /var/ipfire/auth/, /var/ipfire/ca/, etc. pp.) have
> to be owned by that user as well.

Sometimes, programs create temporary files which is why those directories need to have those ownerships.

It would be much more preferable to create any temporary files in /tmp and then move then, but the code is written that way, and I would prefer to not touch it any more.

-Michael

> 
> Thanks, and best regards,
> Peter Müller
> 
> 
>> Hello Peter,
>> 
>> I agree that the files should be owned by root. However, your patch doesn’t fix that.
>> 
>>> On 15 Sep 2022, at 21:15, Peter Müller <peter.mueller(a)ipfire.org> wrote:
>>> 
>>> Since these files are static, there is no legitimate reason why they
>>> should be owned (hence writable) by "nobody".
>>> 
>>> Signed-off-by: Peter Müller <peter.mueller(a)ipfire.org>
>>> ---
>>> lfs/backup | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/lfs/backup b/lfs/backup
>>> index 6f686bf22..adbf16e65 100644
>>> --- a/lfs/backup
>>> +++ b/lfs/backup
>>> @@ -1,7 +1,7 @@
>>> ###############################################################################
>>> #                                                                             #
>>> # IPFire.org - A linux based firewall                                         #
>>> -# Copyright (C) 2007-2021  IPFire Team  <info(a)ipfire.org>                     #
>>> +# Copyright (C) 2007-2022  IPFire Team  <info(a)ipfire.org>                     #
>>> #                                                                             #
>>> # This program is free software: you can redistribute it and/or modify        #
>>> # it under the terms of the GNU General Public License as published by        #
>>> @@ -61,8 +61,8 @@ $(TARGET) : $(patsubst %,$(DIR_DL)/%,$(objects))
>>> 	@$(PREBUILD)
>>> 	-mkdir -p /var/ipfire/backup/bin
>>> 	install -v -m 755 -o root $(DIR_SRC)/config/backup/backup.pl /var/ipfire/backup/bin
>>> -	install -v -m 644 $(DIR_SRC)/config/backup/include /var/ipfire/backup/
>>> -	install -v -m 644 $(DIR_SRC)/config/backup/exclude /var/ipfire/backup/
>>> +	install -v -m 644 -o root $(DIR_SRC)/config/backup/include /var/ipfire/backup/
>>> +	install -v -m 644 -o root $(DIR_SRC)/config/backup/exclude /var/ipfire/backup/
>> 
>> They have been created as root before. That is the default.
>> 
>>> 	chown nobody:nobody -R /var/ipfire/backup/
>> 
>> And here is where they will be changed. Still.
>> 
>>> 	chown root:root -R /var/ipfire/backup/bin/
>>> 	-mkdir -p /var/ipfire/backup/addons
>>> -- 
>>> 2.35.3
>> 
>> -Michael