#273 Typos in db_dir may delete too much (or: system() is evil)

[Ludovico Gardenghi]

Hi,

clearing the cache files is done using these two lines:

snprintf(buf, sizeof(buf), "rm -rf %s/files.db %s/art_cache", db_path, db_path);
system(buf);  // This is actually wrapped in an if().

This is bad. The arguments are not properly escaped/quoted, and some of them may be interpreted by the shell. For instance, if a copy/paste typo made this line appear in the configuration file:

db_dir=/var/cache/minidlna/foo / bar

a call to minidlna -R would try to delete all files in the filesystem (I tried). Hopefully, the process should not be running as root or anybody with write access to many things, and this would not delete too much data, but especially on small/embedded devices this may not be the case.

I would either walk the filesystem in the code (using nftw(3) or similar and calls to unlink(2) and rmdir(2)), or keep calling /bin/rm using fork(2)+execv(3) or similar combinations instead of the more dangerous system(3).

Links to where this happens in the code:

https://sourceforge.net/p/minidlna/git/ci/v1_1_0/tree/minidlna.c#l393
https://sourceforge.net/p/minidlna/git/ci/v1_1_0/tree/minidlna.c#l888

[Carlos Pereira]

Here's a patch for that.

Instead of using ntfw(), which is a clean way to get around but seems Linux-specific, I've prohibited the occurance of single quotes in db_path and log_path and replaced the forementioned system() call to snprintf(buf, sizeof(buf), "rm -rf '%s/files.db' '%s/art_cache'", db_path, db_path);, so now it's impossible to inject commands in system() and/or accidentally create filesystem mayhem.

[Shrimpkin]

I needed to address this issue in one of my feature patches. Simply escapes all characters in db_path. Idea taken from stackoverflow.com.