Buffer overflow in "safe" strncpy usage about nnn HOT 14 CLOSED

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! 👍

from nnn.

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).

from nnn.

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.

from nnn.

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

from nnn.

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

from nnn.

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

from nnn.

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.

from nnn.

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

from nnn.

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

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

from nnn.

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

from nnn.

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).

from nnn.

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.

from nnn.

Another link for later reference.

from nnn.

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?

from nnn.