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@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@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 :-)



+ *  -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 :-).


+}
+
+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 will try to rework this when I have clarity about '-x' (see above) and adapt any cgi that is using it.

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.