[PATCH] ipblocklist: Ensure /var/ipfire/ipblocklist is owned and writable by "nobody"

[PATCH] ipblocklist: Ensure /var/ipfire/ipblocklist is owned and writable by "nobody"

[Peter Müller]

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

Fixes: #12917
Signed-off-by: Peter Müller <peter.mueller(a)ipfire.org>
---
 config/rootfiles/core/170/update.sh | 3 +++
 lfs/ipblocklist-sources             | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/config/rootfiles/core/170/update.sh b/config/rootfiles/core/170/update.sh
index b6b66f3f1..c7dc09946 100644
--- a/config/rootfiles/core/170/update.sh
+++ b/config/rootfiles/core/170/update.sh
@@ -164,6 +164,9 @@ ldconfig
 mkdir -pv /var/lib/ipblocklist
 chown nobody:nobody /var/lib/ipblocklist
 
+# Ensure permissions for /var/ipfire/ipblocklist are set properly
+chown -Rv nobody:nobody /var/ipfire/ipblocklist
+
 # Rebuild fcrontab from scratch
 /usr/bin/fcrontab -z
 
diff --git a/lfs/ipblocklist-sources b/lfs/ipblocklist-sources
index 30b9e94a4..87bd95cca 100644
--- a/lfs/ipblocklist-sources
+++ b/lfs/ipblocklist-sources
@@ -47,7 +47,7 @@ b2 :
 
 $(TARGET) :
 	@$(PREBUILD)
-	mkdir -p /var/ipfire/ipblocklist
-	install -v -m 0644 $(DIR_SRC)/config/ipblocklist/sources /var/ipfire/ipblocklist
+	install -d -o nobody -g nobody -m 0755 /var/ipfire/ipblocklist
+	install -v -o nobody -g nobody -m 0644 $(DIR_SRC)/config/ipblocklist/sources /var/ipfire/ipblocklist
 
 	@$(POSTBUILD)
-- 
2.35.3

Re: [PATCH] ipblocklist: Ensure /var/ipfire/ipblocklist is owned and writable by "nobody"

[Bernhard Bitsch]

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

Reviewed-by: Bernhard Bitsch <bbitsch(a)ipfire.org>

Am 22.08.2022 um 08:30 schrieb Peter Müller:
> Fixes: #12917
> Signed-off-by: Peter Müller <peter.mueller(a)ipfire.org>
> ---
>   config/rootfiles/core/170/update.sh | 3 +++
>   lfs/ipblocklist-sources             | 4 ++--
>   2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/config/rootfiles/core/170/update.sh b/config/rootfiles/core/170/update.sh
> index b6b66f3f1..c7dc09946 100644
> --- a/config/rootfiles/core/170/update.sh
> +++ b/config/rootfiles/core/170/update.sh
> @@ -164,6 +164,9 @@ ldconfig
>   mkdir -pv /var/lib/ipblocklist
>   chown nobody:nobody /var/lib/ipblocklist
>   
> +# Ensure permissions for /var/ipfire/ipblocklist are set properly
> +chown -Rv nobody:nobody /var/ipfire/ipblocklist
> +
>   # Rebuild fcrontab from scratch
>   /usr/bin/fcrontab -z
>   
> diff --git a/lfs/ipblocklist-sources b/lfs/ipblocklist-sources
> index 30b9e94a4..87bd95cca 100644
> --- a/lfs/ipblocklist-sources
> +++ b/lfs/ipblocklist-sources
> @@ -47,7 +47,7 @@ b2 :
>   
>   $(TARGET) :
>   	@$(PREBUILD)
> -	mkdir -p /var/ipfire/ipblocklist
> -	install -v -m 0644 $(DIR_SRC)/config/ipblocklist/sources /var/ipfire/ipblocklist
> +	install -d -o nobody -g nobody -m 0755 /var/ipfire/ipblocklist
> +	install -v -o nobody -g nobody -m 0644 $(DIR_SRC)/config/ipblocklist/sources /var/ipfire/ipblocklist
>   
>   	@$(POSTBUILD)

Re: [PATCH] ipblocklist: Ensure /var/ipfire/ipblocklist is owned and writable by "nobody"

[Peter Müller]

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

Hello list,

today, Stefan reached out to me via phone and explained that /var/ipfire/ipblocklist/
should not be chown'ed to "nobody", since this would mean write access to the "sources"
file, a thing neither needed nor desirable.

Instead, he recommended touching a "modified" file in the same folder and granting
"nobody" write access to it. While testing, I noticed the same thing is necessary for
a "settings" file.

I will submit a second version of the patch in due course.

Best,
Peter Müller


> Fixes: #12917
> Signed-off-by: Peter Müller <peter.mueller(a)ipfire.org>
> ---
>  config/rootfiles/core/170/update.sh | 3 +++
>  lfs/ipblocklist-sources             | 4 ++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/config/rootfiles/core/170/update.sh b/config/rootfiles/core/170/update.sh
> index b6b66f3f1..c7dc09946 100644
> --- a/config/rootfiles/core/170/update.sh
> +++ b/config/rootfiles/core/170/update.sh
> @@ -164,6 +164,9 @@ ldconfig
>  mkdir -pv /var/lib/ipblocklist
>  chown nobody:nobody /var/lib/ipblocklist
>  
> +# Ensure permissions for /var/ipfire/ipblocklist are set properly
> +chown -Rv nobody:nobody /var/ipfire/ipblocklist
> +
>  # Rebuild fcrontab from scratch
>  /usr/bin/fcrontab -z
>  
> diff --git a/lfs/ipblocklist-sources b/lfs/ipblocklist-sources
> index 30b9e94a4..87bd95cca 100644
> --- a/lfs/ipblocklist-sources
> +++ b/lfs/ipblocklist-sources
> @@ -47,7 +47,7 @@ b2 :
>  
>  $(TARGET) :
>  	@$(PREBUILD)
> -	mkdir -p /var/ipfire/ipblocklist
> -	install -v -m 0644 $(DIR_SRC)/config/ipblocklist/sources /var/ipfire/ipblocklist
> +	install -d -o nobody -g nobody -m 0755 /var/ipfire/ipblocklist
> +	install -v -o nobody -g nobody -m 0644 $(DIR_SRC)/config/ipblocklist/sources /var/ipfire/ipblocklist
>  
>  	@$(POSTBUILD)

[PATCH v2] ipblocklist: Both "settings" and "modify" need to be writable for "nobody"

[Peter Müller]

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

The second version of this patch avoids being generous with file
permissions, as Stefan pointed out that /var/ipfire/ipblocklist/sources
must not be writable to "nobody".

Therefore, the needed files ("settings" and "modify") are prepared
during the Core Upgrade and LFS file, and equipped with appropriate
permissions.

Fixes: #12917
Cc: Stefan Schantl <stefan.schantl(a)ipfire.org>
Signed-off-by: Peter Müller <peter.mueller(a)ipfire.org>
---
 config/rootfiles/core/170/update.sh | 4 ++++
 lfs/ipblocklist-sources             | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/config/rootfiles/core/170/update.sh b/config/rootfiles/core/170/update.sh
index b6b66f3f1..9d16f4a32 100644
--- a/config/rootfiles/core/170/update.sh
+++ b/config/rootfiles/core/170/update.sh
@@ -164,6 +164,10 @@ ldconfig
 mkdir -pv /var/lib/ipblocklist
 chown nobody:nobody /var/lib/ipblocklist
 
+# Create necessary files for IPBlocklist and set their ownership accordingly (#12917)
+touch /var/ipfire/ipblocklist/{settings,modified}
+chown nobody:nobody /var/ipfire/ipblocklist/{settings,modified}
+
 # Rebuild fcrontab from scratch
 /usr/bin/fcrontab -z
 
diff --git a/lfs/ipblocklist-sources b/lfs/ipblocklist-sources
index 30b9e94a4..d0ce30350 100644
--- a/lfs/ipblocklist-sources
+++ b/lfs/ipblocklist-sources
@@ -49,5 +49,7 @@ $(TARGET) :
 	@$(PREBUILD)
 	mkdir -p /var/ipfire/ipblocklist
 	install -v -m 0644 $(DIR_SRC)/config/ipblocklist/sources /var/ipfire/ipblocklist
+	touch /var/ipfire/ipblocklist/{settings,modified}
+	chown nobody:nobody /var/ipfire/ipblocklist/{settings,modified}
 
 	@$(POSTBUILD)
-- 
2.35.3

Re: [PATCH v2] ipblocklist: Both "settings" and "modify" need to be writable for "nobody"

[Michael Tremer]

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

Hello,

I was told that this patch isn’t solving the problem it is supposed to solve.

However, I do not see why. Could someone explain to my little brain why?

-Michael

> On 22 Aug 2022, at 21:11, Peter Müller <peter.mueller(a)ipfire.org> wrote:
> 
> The second version of this patch avoids being generous with file
> permissions, as Stefan pointed out that /var/ipfire/ipblocklist/sources
> must not be writable to "nobody".
> 
> Therefore, the needed files ("settings" and "modify") are prepared
> during the Core Upgrade and LFS file, and equipped with appropriate
> permissions.
> 
> Fixes: #12917
> Cc: Stefan Schantl <stefan.schantl(a)ipfire.org>
> Signed-off-by: Peter Müller <peter.mueller(a)ipfire.org>
> ---
> config/rootfiles/core/170/update.sh | 4 ++++
> lfs/ipblocklist-sources             | 2 ++
> 2 files changed, 6 insertions(+)
> 
> diff --git a/config/rootfiles/core/170/update.sh b/config/rootfiles/core/170/update.sh
> index b6b66f3f1..9d16f4a32 100644
> --- a/config/rootfiles/core/170/update.sh
> +++ b/config/rootfiles/core/170/update.sh
> @@ -164,6 +164,10 @@ ldconfig
> mkdir -pv /var/lib/ipblocklist
> chown nobody:nobody /var/lib/ipblocklist
> 
> +# Create necessary files for IPBlocklist and set their ownership accordingly (#12917)
> +touch /var/ipfire/ipblocklist/{settings,modified}
> +chown nobody:nobody /var/ipfire/ipblocklist/{settings,modified}
> +
> # Rebuild fcrontab from scratch
> /usr/bin/fcrontab -z
> 
> diff --git a/lfs/ipblocklist-sources b/lfs/ipblocklist-sources
> index 30b9e94a4..d0ce30350 100644
> --- a/lfs/ipblocklist-sources
> +++ b/lfs/ipblocklist-sources
> @@ -49,5 +49,7 @@ $(TARGET) :
> 	@$(PREBUILD)
> 	mkdir -p /var/ipfire/ipblocklist
> 	install -v -m 0644 $(DIR_SRC)/config/ipblocklist/sources /var/ipfire/ipblocklist
> +	touch /var/ipfire/ipblocklist/{settings,modified}
> +	chown nobody:nobody /var/ipfire/ipblocklist/{settings,modified}
> 
> 	@$(POSTBUILD)
> -- 
> 2.35.3

Re: [PATCH v2] ipblocklist: Both "settings" and "modify" need to be writable for "nobody"

[Peter Müller]

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

Hello Michael,

thanks for your reply and apologies for my belated response.

Stefan pointed out to me that if we would create these files in ipblocklist itself,
they would have became part of the component's rootfile (which was also not updated
in the patch). This would have caused user settings for ipblocklist to be overwritten,
if ipblocklist is updated in a future Core Update.

configroot is the better place, since we must never ship this, and this is where
all the other settings files are created already. Also, file permissions are already
taken care of there.

Version 3 should _finally_ solve the issue. Please let me know if it doesn't.

All the best,
Peter Müller

> Hello,
> 
> I was told that this patch isn’t solving the problem it is supposed to solve.
> 
> However, I do not see why. Could someone explain to my little brain why?
> 
> -Michael
> 
>> On 22 Aug 2022, at 21:11, Peter Müller <peter.mueller(a)ipfire.org> wrote:
>>
>> The second version of this patch avoids being generous with file
>> permissions, as Stefan pointed out that /var/ipfire/ipblocklist/sources
>> must not be writable to "nobody".
>>
>> Therefore, the needed files ("settings" and "modify") are prepared
>> during the Core Upgrade and LFS file, and equipped with appropriate
>> permissions.
>>
>> Fixes: #12917
>> Cc: Stefan Schantl <stefan.schantl(a)ipfire.org>
>> Signed-off-by: Peter Müller <peter.mueller(a)ipfire.org>
>> ---
>> config/rootfiles/core/170/update.sh | 4 ++++
>> lfs/ipblocklist-sources             | 2 ++
>> 2 files changed, 6 insertions(+)
>>
>> diff --git a/config/rootfiles/core/170/update.sh b/config/rootfiles/core/170/update.sh
>> index b6b66f3f1..9d16f4a32 100644
>> --- a/config/rootfiles/core/170/update.sh
>> +++ b/config/rootfiles/core/170/update.sh
>> @@ -164,6 +164,10 @@ ldconfig
>> mkdir -pv /var/lib/ipblocklist
>> chown nobody:nobody /var/lib/ipblocklist
>>
>> +# Create necessary files for IPBlocklist and set their ownership accordingly (#12917)
>> +touch /var/ipfire/ipblocklist/{settings,modified}
>> +chown nobody:nobody /var/ipfire/ipblocklist/{settings,modified}
>> +
>> # Rebuild fcrontab from scratch
>> /usr/bin/fcrontab -z
>>
>> diff --git a/lfs/ipblocklist-sources b/lfs/ipblocklist-sources
>> index 30b9e94a4..d0ce30350 100644
>> --- a/lfs/ipblocklist-sources
>> +++ b/lfs/ipblocklist-sources
>> @@ -49,5 +49,7 @@ $(TARGET) :
>> 	@$(PREBUILD)
>> 	mkdir -p /var/ipfire/ipblocklist
>> 	install -v -m 0644 $(DIR_SRC)/config/ipblocklist/sources /var/ipfire/ipblocklist
>> +	touch /var/ipfire/ipblocklist/{settings,modified}
>> +	chown nobody:nobody /var/ipfire/ipblocklist/{settings,modified}
>>
>> 	@$(POSTBUILD)
>> -- 
>> 2.35.3
> 

Re: [PATCH] ipblocklist: Ensure /var/ipfire/ipblocklist is owned and writable by "nobody"

[Rob Brewer]

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

On Mon, 22 Aug 2022 20:08:00 +0000, Peter Müller wrote:

> Hello list,
> 
> today, Stefan reached out to me via phone and explained that
> /var/ipfire/ipblocklist/ should not be chown'ed to "nobody", since this
> would mean write access to the "sources"
> file, a thing neither needed nor desirable.
> 
> Instead, he recommended touching a "modified" file in the same folder
> and granting "nobody" write access to it. While testing, I noticed the
> same thing is necessary for a "settings" file.
> 
> I will submit a second version of the patch in due course.
> 
> Best,
> Peter Müller
> 
> 
If it helps I think Tim's original Ipblacklist had these permissions:

drwxr-xr-x 2 nobody nobody  4096 Feb  6  2022 ipblacklist

ls -l /var/ipfire/ipblacklist/

-rw-r--r-- 1 root   root     441 Aug 22 21:24 checked
-rw-r--r-- 1 root   root     190 Aug 22 21:24 modified
-rw-r--r-- 1 nobody nobody   305 Aug  3 10:29 settings
-rw-r--r-- 1 root   root   11443 Aug  3 09:28 sources
-rw-r--r-- 1 root   root       0 Feb  2  2022 status

So nobody.nobody would seem to be correct for the directory and is working 
OK here.


Rob