How and what could be improved in this code?

dminican_slax · 08-03-2020, 04:00 PM

I made this sometime ago now, I use slackware, I know I could do this with a script but I wanted to practice C so my goal was to practice the use of system() making a little tool to get info and free HD space I'd like to implement some kind of automation in future versions but I'm neither a professional dev or sysadmin.

Tell me what you guys think

Code:

#include <stdio.h>
#include <stdlib.h>
#include <sys/errno.h>

int main()
{

	char lttr[2];

	puts("|************************************************************|");
	puts("|*           simple system info and cleaning tool           *|");
	puts("|*             choose the actions from the list             *|");
	puts("-************************************************************-");
	puts("* Use \"a\" for system info or \"x\" for extra info          *");
	puts("* Use \"b\" for system cleaning                              *");
	puts("* Use \"c\" to free some disk space                          *");
	puts("**************************************************************");
	lttr = getchar();

	if (lttr <= 0 || lttr >= 0)
	{
		puts("Use a,b,c or x\n");
		exit(1);
	}

	switch(lttr)
	{
		case 'a':
		case 'A':
		
		puts("System info...\n");
		puts("linux distro/kernel info:\n");
		system("uname -a");
		puts("logged users:\n");
		system("w");
		puts("system uptime\n");
		system("uptime");
		break;

		case 'x':
		case 'X':
		printf("Home directory:\n");
		system("echo ~");
		printf("Last logged users:\n");
		system("last");
		printf("Asigned shell:\n");
		system("echo $SHELL");
		printf("All user directories:\n");
		system("echo $userdirs");
		puts("That's all!");
		exit(EXIT_SUCCESS);
		break;

		
		case 'b':
		case 'B':

		puts("cleaning /tmp\n");
		system("rm -rf /tmp/*");
		puts("cleaning system logs\n");
		system("cd /var/log&&cat /dev/null > message&&cat /dev/null > wtmp");
		break;

		case 'c':
		case 'C':

		puts("deleting some unused files...");
		system("rm -rf ~/.thumbnails/large/*");
		system("rm -rf ~/.thumbnails/normal/*");
		exit(EXIT_SUCCESS);

	}
	return 0;
}

Greetings

Geist · 08-03-2020, 05:11 PM

Ehhh, "system" is 'basically' just a script in the end.

So, I would suggest, to make this code better...is to just write it as a script, perhaps using dialog or something to snazz it up.

There really isn't much to learn from these 'dumb' functions.

Why not make a small game instead. Guess the number, perhaps with "warm" and "cold" hints?

Edit:
But, to stay on topic.

You could sanitize the input a bit by making it lowercase, and then checking against the lowercase letter.
You could put those cases into their own functions and call them when the respective letter is entered.
You could refactor the logic that fills 'lttr' with a letter into its own function and then call it instead of 'inlining' the code, and then, inside that function do some better checking against the input values (you could also put the lowercasing logic there)
If you sanitize the input in its own function, and it's gonna be just one character, then you could return a single character from the function, instead of an array, even if you were to use an array internally there.

Other than that, I can't think of much.
Except perhaps rename lttr to letter, or choice, or input, or whatever.
Cause, what if you want a number at some point?

Ratamahatta · 08-03-2020, 07:14 PM

Quote:

Originally Posted by dminican_slax

Code:

...
	if (lttr <= 0 || lttr >= 0)
	{
		puts("Use a,b,c or x\n");
		exit(1);
	}
...

I firmly believe that lttr will always be either less, equal or greater than 0. ;-)

You probably meant something like this:

Code:

if (lttr < A || lttr > C && (lttr < a || lttr > c))

But as Geist suggested, convert the input to either lowercase or uppercase. This will allow to simplify that line to something like (assuming input is converted to lowercase):

Code:

if (lttr < a || lttr > c)

I don't see you use anything from sys/error.h except for EXIT_SUCCESS. - On Linux/Unix that value is 0 for any command-line tool. That include might be removed and the macros replaced. (If that is according to your taste. - That is a matter of taste, really.)

I'm with Geist on the system calls. Maybe you should use some more C-ish code instead. Will be better portable (if you want to port it) and in any case faster and reduces the memory footprint. (Because system starts a shell and hands it the command as parameters.)

rtmistler · 08-04-2020, 07:18 AM

I've classically heard the repeated declarations that the system call is a risk, and I believe I've also heard that this was fixed long ago.

Opinion is that if you are using C code, and invoking the system call to directly call a shell statement, then one of two things:

There should be a C function available which can do the same thing.
There should be enough support in bash or other scripting languages where you do not need to have a C or other language program be the host for it.

So I don't mix a program language and a shell language together, as much as possible.

There is a remove() function available in the C library.

Otherwise, code is code.

It doesn't appear to be a hot mess of syntax and formatting, granted I indent the contents for each case statement, but also my indentations are 3-4 white spaces, which you may also use, but here in the post it appears to be nearly 8 spaces, or even tabs, haven't checked.

I also haven't checked correctness of the flow, but I could. My question is why would I? This can be debugged very easily using GDB and if you had a particular problem, I'd advise you to do that, or I'd look at the specific problem area where you asked a question.

As it is, you've just asked a general question, and my answers are:

Don't mix C and bash
Test the code to your satisfaction and if it does what you want, for all test situations you can envision, then you're done.
If there's problems, then debug them.

If there are deficiencies you discover with testing, then please update.

pan64 · 08-04-2020, 07:52 AM

yes, as it was mentioned you need to remove all the system calls (if you wish to improve). You need to decide if you want to write a shell script or c code.
the imperfect if statement was already mentioned.
You cannot (and must not) remove system logs as it was implemented here.
rm -rf /tmp/* is just dangerous, this may cause problems.

rtmistler · 08-04-2020, 08:20 AM

Quote:

Originally Posted by pan64

You cannot (and must not) remove system logs as it was implemented here.
rm -rf /tmp/* is just dangerous, this may cause problems.

My laziness with not really reading the code is exposed!

I wholeheartedly agree, and recommend to never to do any system cleanup shortcuts using a script or program.

EVERY ... (SWEAR WORD) ... TIME, I have used an automated process to do something like this, the MOMENT I "let it rip" (executed it), I regretted it!

dugan · 08-04-2020, 12:29 PM

Nothing wrong with doing one puts per line, but I'm going to point out (if you don't know already) that C will automatically concatenate adjacent strings. You can, if you'd prefer, do it with:

Code:

printf("|************************************************************|\n"
       "|*           simple system info and cleaning tool           *|\n"
       "|*             choose the actions from the list             *|\n"
       "-************************************************************-\n"
       "* Use \"a\" for system info or \"x\" for extra info          *\n"
       "* Use \"b\" for system cleaning                              *\n"
       "* Use \"c\" to free some disk space                          *\n"
       "**************************************************************\n");

And as for this block (which obviously always executes, and is also wrong for other reasons):

Code:

if (lttr <= 0 || lttr >= 0)
{
	puts("Use a,b,c or x\n");
	exit(1);
}

It should be the default block in the switch statement.

Code:

// You're not doing anything else with lttr, so this will save a line.
switch(getchar())
{
	case 'a':
	case 'A':
	// Snip		
	break;

	case 'x':
	case 'X':
	// snip
	break;
	
	case 'b':
	case 'B':
	// snip
	break;

	case 'c':
	case 'C':
	// snip
	exit(EXIT_SUCCESS);

	default:
	puts("Use a,b,c or x\n");
	exit(1);
}

mina86 · 08-04-2020, 12:39 PM

Code:

	if (lttr <= 0 || lttr >= 0)
	{
		puts("Use a,b,c or x\n");
		exit(1);
	}

Get rid of this condition. Instead add a default case to the switch.

Don't use exit function. You're in the main function already; just return the value.

Make up your mind in regards to using EXIT_SUCCESS and EXIT_FAILURE or raw numbers. Use one or the other, but don’t mix both.

Code:

	switch(lttr)
	{
		case 'a':
		case 'A':
		
		puts("System info...\n");
		puts("linux distro/kernel info:\n");

Either indent case lines and then indent code under the case by two levels, or don’t indent case lines. For example:

Code:

switch (foo) {
case 'a':
        ...do stuff..
        break;
case 'A':
        ...do other stuff...
        break;
...
}

Code:

		case 'x':
		case 'X':
...
		system("echo $userdirs");
		puts("That's all!");
		exit(EXIT_SUCCESS);
		break;

No need for break after an exit since the latter terminates the program. Furthermore, there isn't need for exit at all since you have a return 0 after the switch.

Code:

		system("cd /var/log&&cat /dev/null > message&&cat /dev/null > wtmp");

This is just fopen("/var/log/message"); fopen("/var/log/wtmp"); so write a function that does that for a file and handles errors.

dugan · 08-04-2020, 04:27 PM

Quote:

Originally Posted by rtmistler

I've classically heard the repeated declarations that the system call is a risk, and I believe I've also heard that this was fixed long ago.

How would it be possible to "fix" it, exactly?

https://blog.securityinnovation.com/...injection.html

https://owasp.org/www-community/atta...mand_Injection

rtmistler · 08-04-2020, 07:50 PM

Quote:

Originally Posted by dugan

How would it be possible to "fix" it, exactly?

Beats me. I stopped paying attention to it back in the ... 90s?

You heard it, you heard it.

Eventually someone said, "Yeah ... you're not supposed to use the system call." And someone else said loudly, "That was fixed!!"

I just never use it.

You don't need it.

I honestly don't care the explanation or arguments about it, sorry to say.

pan64 · 08-05-2020, 02:26 AM

Quote:

Originally Posted by dugan

How would it be possible to "fix" it, exactly?

by analyzing/interpreting the string right before execution [system call]. One by one. (look for not acceptable chars for example).

mina86 · 08-05-2020, 04:47 AM

Quote:

Originally Posted by pan64

by analyzing/interpreting the string right before execution [system call]. One by one. (look for not acceptable chars for example).

That is a terrible way to do security. Chances are you’re gonna miss something or will whitelist the wrong set of characters. This has been tried many times in many contexts and often leads to vulnerabilities.

dugan · 08-05-2020, 05:18 AM

Quote:

Originally Posted by pan64

by analyzing/interpreting the string right before execution [system call]. One by one. (look for not acceptable chars for example).

What, like "$" and ";"? That would defeat the purpose of using system in the first place. The standard library is obviously not "sanitizing" them out before spawning a shell to run the command string (which, remember, is what "system" does).

Python and Qt both take an array of arguments when spawning a process. For C, this page recommends using execve (instead of system), which is obviously how both of them are implemented:

ENV33-C. Do not call system()

pan64 · 08-05-2020, 05:30 AM

Quote:

Originally Posted by mina86

That is a terrible way to do security. Chances are you’re gonna miss something or will whitelist the wrong set of characters. This has been tried many times in many contexts and often leads to vulnerabilities.

Obviously you are right. But anyhow, I don't know better way.

dminican_slax · 08-05-2020, 01:07 PM

Quote:

Originally Posted by Geist

You could sanitize the input a bit by making it lowercase, and then checking against the lowercase letter.
You could put those cases into their own functions and call them when the respective letter is entered.
You could refactor the logic that fills 'lttr' with a letter into its own function and then call it instead of 'inlining' the code, and then, inside that function do some better checking against the input values (you could also put the lowercasing logic there)
If you sanitize the input in its own function, and it's gonna be just one character, then you could return a single character from the function, instead of an array, even if you were to use an array internally there.

Other than that, I can't think of much.
Except perhaps rename lttr to letter, or choice, or input, or whatever.
Cause, what if you want a number at some point?

Guys thanks for your answers, as I said I was just trying to practice the use of system() this code is the version 4 of it I'll check on it and see if I can implement some of the things you've suggested. I feel like I have to reply to everyone but that'd make the thread too long. I don't know a lot about system() and it's security implications, I'm just a noob with a kinda complex hobby.

Greetings,
dminican_slax