Re: [PATCH] misc-progs: getipstat: Extend functionality

[Michael Tremer <michael.tremer@ipfire.org>]

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

Hello Robin,

> On 23 Apr 2021, at 17:22, Robin Roevens <robin.roevens(a)disroot.org> wrote:
> 
> * bugfix: Make sure outputfiles are removed beforehand
> to prevent permission errors writing to them.
> * Add optional parameter "-x" to have iptables report exact numbers
> * Add optional parameter "-f <output_filename>" to save iptables
> filters table output to an alternate filename
> * Add optional parameter "-n <output_filename>" to save iptables
> nat table output to an alternate filename
> * Add optional parameter "-m <output_filename>" to save iptables
> mangle table output to an alternate filename
> 
> Signed-off-by: Robin Roevens <robin.roevens(a)disroot.org>
> ---
> src/misc-progs/getipstat.c | 74 +++++++++++++++++++++++++++++++++++---
> 1 file changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/src/misc-progs/getipstat.c b/src/misc-progs/getipstat.c
> index c806d54a9..57ad81d46 100644
> --- a/src/misc-progs/getipstat.c
> +++ b/src/misc-progs/getipstat.c
> @@ -2,6 +2,15 @@
>  *
>  * Get the list from IPTABLES -L
>  * 
> + * Optional commandline parameters:
> + *  -x 
> + *   instruct iptables to expand numbers

I would say that we should always call iptables with “-x” (I have no idea why this isn’t the default). There is no point to have this configurable because this command is not being called by a user and so we should make a decision either way.

> + *  -f <filter_rules_output_filename>
> + *   output filter table to alternative filename in /var/tmp/

This is adding a shell command line injection vulnerability into the code which allows an unprivileged user to execute commands as root. More below.

> + *  -n <nat_rules_output_filename>
> + *   output nat table to alternative filename in /var/tmp/
> + *  -m <mangle_rules_output_filename>
> + *   output mangle table to alternative filename in /var/tmp/
>  */
> 
> #include <stdio.h>
> @@ -12,16 +21,71 @@
> #include <fcntl.h>
> #include "setuid.h"
> 
> +int cmdOutputToFile(char *cmd, char *filename) {
> +	FILE *file;
> +	char command[STRING_SIZE];
> 
> -int main(void)
> +	// remove file if it already exist to prevent permission denied errors
> +	// if we have no explicit write permission on it.
> +	if ((file = fopen(filename, "r"))) {
> +		fclose(file);
> +		if (remove(filename) != 0) {
> +			fprintf(stderr, "\n%s could not be overwritten.\n", filename);
> +			return 1;
> +		}
> +	}

You can simply use access() to check if a file already exists. That avoids actually allocating a whole file descriptor.

> +	// Execute command and redirect output to file
> +	snprintf(command, STRING_SIZE - 1, "%s > %s", cmd, filename);
> +	return safe_system(command);

Here is the bit that introduces the vulnerability by passing through “filename” without any sanitisation.

Since safe_system() is calling a shell, a filename like “/dev/null; reboot” would reboot the system. Trying to open the file before will work fine, because almost anything is a legal filename.

So this solution is not acceptable as it pretty much allows anyone to run anything.

> +}
> +
> +int main(int argc, char** argv)
> {
> +	// Set defaults
> +	char params[STRING_SIZE] = "-L -v -n";
> +	char out_file_filter[STRING_SIZE] = "/var/tmp/iptables.txt";
> +	char out_file_nat[STRING_SIZE] = "/var/tmp/iptablesnat.txt";
> +	char out_file_mangle[STRING_SIZE] = "/var/tmp/iptablesmangle.txt";
> +
> +	int opt;
> +	char command[STRING_SIZE];
> +	
> 	if (!(initsetuid()))
> 		exit(1);
> 
> -	safe_system("/sbin/iptables -L -v -n > /var/tmp/iptables.txt");
> -	safe_system("/sbin/iptables -L -v -n -t nat > /var/tmp/iptablesnat.txt");
> -	safe_system("/sbin/iptables -t mangle -L -v -n > /var/tmp/iptablesmangle.txt");
> -	safe_system("chown nobody.nobody /var/tmp/iptables.txt /var/tmp/iptablesnat.txt /var/tmp/iptablesmangle.txt");
> +	// Parse command line params
> +	if (argc > 1) {
> +		while ((opt = getopt(argc, argv, "xf:n:m:")) != -1) {
> +			switch(opt) {
> +				case 'x':
> +					strcat(params, " -x");
> +					break;

See above.

> +				case 'f':
> +					snprintf(out_file_filter, STRING_SIZE - 1, "/var/tmp/%s", optarg);
> +					break;
> +				case 'n':
> +					snprintf(out_file_nat, STRING_SIZE - 1, "/var/tmp/%s", optarg);
> +					break;
> +				case 'm':
> +					snprintf(out_file_mangle, STRING_SIZE - 1, "/var/tmp/%s", optarg);
> +					break;

I agree with the argument parsing. We should simply expect one of -f, -n or -m being passed and then only run the one command.

A much better solution would be to not write the output to disk at all (saves the whole sanitisation headache), but just output it to the console. That way, we can parse it very easily in the existing perl CGI scripts because we do not do anything else but reading the file back again; and you can do the same in Zabbix.

Is that an acceptable solution for you?

Best,
-Michael

> +				default:
> +					fprintf(stderr, "\nBad argument given.\n\ngetipstat [-x][-f <filter_rules_output_filename>][-n <nat_rules_output_filename>][-m <mangle_rules_output_filename>]\n");
> +					exit(1);
> +			}
> +		}
> +	}
> +
> +	// Generate ipstat files
> +	snprintf(command, STRING_SIZE - 1, "/sbin/iptables %s", params);
> +	cmdOutputToFile(command, out_file_filter);
> +	snprintf(command, STRING_SIZE - 1, "/sbin/iptables -t nat %s", params);
> +	cmdOutputToFile(command, out_file_nat);
> +	snprintf(command, STRING_SIZE - 1, "/sbin/iptables -t mangle %s", params);
> +	cmdOutputToFile(command, out_file_mangle);
> +	snprintf(command, STRING_SIZE - 1, "chown nobody.nobody %s %s %s", out_file_filter, out_file_nat, out_file_mangle);
> +	safe_system(command);
> 	
> 	return 0;
> }
> -- 
> 2.31.1
> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
>