On 19/08/2015 15:29, Edward Bartolo wrote:
> This is the completed C backend with all functions tested to work. Any
> suggestions as to modifications are welcome.

OK, someone has to be the bad guy. Let it be me.

First, please note that what I'm saying is not meant to discourage you.
I appreciate your enthusiasm and willingness to contribute open source
software. What I'm saying is meant to make you realize that writing
secure software is difficult, especially in C/Unix, which is full of
pitfalls. As long as you're unfamiliar with the C/Unix API and all its
standard traps, I would advise you to refrain from writing code that
is going to be run as root; if you want to be operational right away
and contribute system software right now, it's probably easier to stick
to higher-level languages, such as Perl, Python, or whatever the FotM
interpreted language is at this time. It won't be as satisfying, and the
programs won't be as efficient, but it will be safer.

On to the code review.


> //using namespace std;

This is technically a nit, but philosophically important:
despite of what they've told you, C and C++ are not the same
language at all, and when you're writing C, you're not writing
C++. So, if you're going to write a C program, forget your C++
habits, don't even put them in comments.

> #define opSave                0
> (...)
> #define opLoadExisting            7

As you've been suggested: use an enum.

> const
>     char* path_to_interfaces_files = "/etc/network/wifi";

Aesthetics: avoid global variables if you can. If your variable
needs to be used in several functions in your module, declare it
static: "static const char *path_..." so at least it won't be
exported to other modules if you add some one day.


> 1) Glib::spawn_sync instead of a pipe stream, provides a slot.

I don't understand this comment. It's C++ syntax again, and about
the Glib module. Please don't try to explain what you're doing with
analogies to other modules in other languages.

> 2) cmd trying to call an inexistent command still returns a valid pointer!
>     verify cmd exists before calling exec

... and don't do that, because it creates what is known as a TOCTOU
race condition: you check that the cmd exists, then you use it, but
in the meantime, it could have disappeared. Or the cmd might not
exist when you check it, but would have appeared before you used it.
So, in essence, the check is useless. What you need is a proper error
code when you try to execute a command that does not exist; if
necessary, change your API.

> inline int file_exists(char* name) {
>    return (access(name, F_OK) != -1);
> }

And the function isn't actually used in your code either, so you
should simply remove its definition.

>         if(fgets(buffer, buf_size, pipe) != NULL)

If a line is longer than 127 bytes, it will be silently truncated to
127. Is that what you want? If yes, it's fine, but you should document
the behaviour.

>             if (out != NULL)
>                 strcat(out, buffer);
>                 else strcpy(out, buffer);

This makes no sense. strcpy() can't be called with a NULL argument
any more than strcat can. What are you trying to accomplish here?

>     return pclose(pipe);

This is a bit more advanced. When you design a function, or an
interface in general, you want to make it as opaque as possible: you
want to report high-level status to the caller, and hide low-level
details.
Here, the caller does not know you used popen(), and does not want
to know. The caller only wants the result in the "out" buffer, and
maybe a 0 or 1 return code (possibly with errno set) to know whether
things went OK or not. Returning the result of pclose() leaks your
internal shenanigans to the caller: don't do that. Instead, design
a proper interface (e.g. OK -> nonzero, NOK -> 0 with errno set) and
write your function so that it implements exactly that interface.

>     int saveFile(char* essid, char* pw) //argv[2], argv[3]
>     {
>         char ifilename[1024];
>         strcpy(ifilename, path_to_interfaces_files);

>         

>         strcat(ifilename, "/");
>         strcat(ifilename, essid);

Boom. You're dead.

I'm stopping here, but you're making this mistake in all the rest
of your code, and this is the *single worst mistake* to make as a
C programmer: a buffer overflow. A buffer overflow is the main cause
of security issues all over the computer world, and you cannot, you
ABSOLUTELY CANNOT, publish a piece of code that contains one, especially
when said piece of code is supposed to run as root.

ifilename is 1024 bytes long. You are assuming that essid, and
whatever comes afterwards, will fit into 1024 bytes. This is true
for normal inputs, which is certainly what you tested your program
against, but the input is given as a command line argument to your
program: you do not control the input. *The user* controls the input.
And a malicious user could very well give an essid argument that is
longer than 1024 bytes. Your program would then either crash (in the
best case), or fly demons through your nose, or silently do very evil
things that the malicious user intended.

Never, ever use fixed-size buffers to handle input you do not have
control over.

You have two choices here. The simplest choice is to sanitize your
input: before calling the functions, you make sure that your arguments
are not longer than a given maximum. You adjust the size of your buffer
so that the biggest possible data fits.
The other choice is to use a variable length buffer: you malloc()
a string, then copy your data into it, but if the data is bigger than
your allocated pool, you realloc() it, and so on, so you never write
data to memory you haven't allocated. It's a bit painful to do that
in pure C without any external help, but there are a lot of libraries
that provide interfaces to help you do that. My own library has such
an interface, "stralloc", and I use it all the time. It's invaluable.

So there you go. C/Unix is hard; I love it, but it requires a certain
frame of mind, and paying attention to a lot of details.
Please fix your buffer overflow NOW, before enthusiasts copy the code
and use it as is. Please fix the other, smaller things at your
convenience. I'll be available if you have questions.

--
Laurent