Buffer overflow in "safe" strncpy usage

[joshka]

@Duncaen pointed out a buffer overflow at https://lobste.rs/s/0zcez1/nnn_missing_terminal_file_browser_for_x#c_dfkmqo

/* Just a safe strncpy(3) */
static void
xstrlcpy(char *dest, const char *src, size_t n)
{
	strncpy(dest, src, n - 1);
	dest[n - 1] = '\0';
}
...
xstrlcpy(cmd + strlen(cmd), newpath, strlen(newpath) + 1);
xstrlcpy(buf + strlen(buf), fpath, strlen(fpath) + 1);
...

First example is a buffer overflow if the user sets an NNN_OPENER environment variable to > 1024 chars

Second example isn't an overflow, as buf length is fpath length + 16 and initially filled with something less than 15 chars. It would be easy to carelessly modify this line or the previous lines to cause a buffer overflow though.

[jarun]

The first one is a miss. It's fixed now.
I'm aware of and will live with the second one, don't see it changing anytime soon.

Thanks for the report! 👍

[Duncaen]

Maybe fix the way you use strncpy(3), the n parameter should be the max length for dest, not src.
And maybe escape the file names in commands executed with system(3).

[jarun]

I believe that's not an issue anymore (wrt. strncpy(3) usage). strlen(newpath) can't exceed 4096 (PATH_MAX), and I have added a check to ensure the lengths of env vars don't exit 4096. Together, the length has to be less than MAX_CMD_LEN.

[jarun]

But I see the point now. It's safer and future proof. Will make the change. 👍

[Duncaen]

This is not escaping, wouldn't files contain ' break commands now?

[jarun]

@Duncaen I am not quite clear here though I understand there are some vulnerabilities. Probably raise a PR.

[jarun]

I believe you indicated this. To avoid all the string manipulation I decided to move from system(3) to exec* calls as much as possible.

[joshka]

There's also a few places where strcat could overflow the end of string.
There's a bunch of places where a malicious environment variable could overflow buffers (everywhere you see sprintf / strcat and xgetenv)

Then there's https://github.com/jarun/nnn/blob/master/nnn.c#L1585:

xstrlcpy(cmd + 1, player, MAX_CMD_LEN); // overflow by 1

The guiding rule broken in lots of places is: Don't trust your inputs. In nnn those are:

1. filenames
2. environment variables
3. keyboard input
4. command output (maybe?)
5. possibly other things

[jarun]

xstrlcpy(cmd + 1, player, MAX_CMD_LEN); // overflow by 1

Please refer to the latest code. This is gone now.

[jarun]

Probably I'll get rid of the suspicious strcat()s as well.

[joshka]

I'm not a C programmer (my feedback on this was more out of curiosity than anything). Please forgive me if this is a stupid question ;). I wonder whether switching to C11 methods be helpful here e.g. strncpy_s / strncat_s.

There's a good article at http://www.informit.com/articles/article.aspx?p=2036582&seqNum=5 that highlights some of the potential issues with other mechanisms (like string truncation).

[jarun]

I wonder whether switching to C11 methods be helpful here e.g. strncpy_s / strncat_s.

Maybe, but there's always scope for doing better when it comes to questions on security. I believe when a system is having issues with EDITOR or PAGER, it's already compromised or the user intentionally wants to play evil. At some point we just need to put a stop to the paranoia and move ahead.

[jarun]

Another link for later reference.

[jarun]

I've got rid of all the system(3) calls (except a harmless one), an activity I wanted to start for sometime now. However, nnn is just around 1 month old and I was more involved in adding new features. Thanks for speeding up the activity. Happy to get rid of the shelluloid crap where my knowledge is admittedly limited.

If you find something else, maybe consider raising a PR?