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

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

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

Hello,

> On 26 Apr 2021, at 12:04, Robin Roevens <robin.roevens(a)disroot.org> wrote:
> 
> Hi Michael
> 
> Michael Tremer schreef op ma 26-04-2021 om 11:40 [+0100]:
>> 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.
> 
> Since the output was used on webgui pages, I assumed you wanted pretty printed sizes, for readability by the user (And I think I indeed prefer that on the gui). Therefore I implemented it as an optional parameter. No problem for me to just use -x always and not make it optional but I do think it adds value to the command to the keep '-x' hence displaying pretty values on the webgui and eventually the webgui could give the choice to the user to see the pretty print or the exact print; defaulting to the pretty print, as it is pretty :-)

Oh, I must have confused this with “-w”, the locking parameter. Wherever we present this data to the user (and only god knows why we are actually doing this), we should format sizes in something human readable, indeed.

If there is a long option, we should probably use those, so that stupid people like me do not confuse them (—-wait over -w, and —-exact over -x).

>> 
>>> + *  -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.
> 
> Well.. I haven't really touched any C/C++ code since I graduated from school some 20 years ago .. so it was already a bit of a challenge for me to come up with the current code. Absolutely oblivious to possible vulnerabilities.
> Luckily you catched it :-).

This is why we are doing this review process. And although it sometimes seems painful, it is very much worth it.

> 
>> 
>>> +}
>>> +
>>> +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?
> 
> That is absolutely acceptable for me, as I already thought it to be a bit cumbersome handling this through files.

I guess this file was written by someone who couldn’t implement it in any better way. We unfortunately have loads of code that isn’t technically the best (or remotely a good) solution, so I am happy to fix that now. We all learned a lot over the years.

> I will try to rework this when I have clarity about '-x' (see above) and adapt any cgi that is using it.

So if you simply call safe_system() with string that doesn’t change, you are fine.

I would also suggest to use run() which avoids the shell (and therefore a whole class of vulnerabilities) and takes an array which makes it easier to add the filter/nat/mangle options. There are lots of examples in src/misc-progs, but feel free to ask any questions on here.

Best,
-Michael

> 
> Regards
> Robin
> 
>> 
>> 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.
>>> 
>> 
>> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke 
> inhoud door MailScanner en lijkt schoon te zijn.