File write vulnerability if username contains `../`

[5225225]

I set my username on client A to ../../home/jess/.ssh and used ?send authorized_keys.

This then wrote the contents of authorized_keys to the actual authorized_keys on the target machine.

I haven't tested it with a malicious filename, but I assume that has the same problem.

A mitigation would be to check that the path you're writing to is under the temp directory, and replace / with some other character when writing.

Also, when creating the file, it should be checked that it doesn't exist (See https://doc.rust-lang.org/std/fs/struct.OpenOptions.html#method.create_new )

[sigmaSd]

Hi,

My opinion is termchat is currently made with friendly network in mind, so no encryption and all features can be abused.

I think remaining like this is a good option in-itself.

If termchat wants to take the direction of secure software, I'm sure there are a lot of things that needs to be changed.

With that in mind regarding this issue, I think sanitizing is too tricky to get right, maybe a more correct approach is to prompt the receiver if they want to receive the file (and I imagine also the same for all the other commands).

Also we can maybe enforce normal usernames (by dropping requests from sender that don't match [0-9A-za-z_]* for example).

For the last issue (creating the file, it should be checked that it doesn't exist ) , I think that should be a separate issue(To fix it, the Stream message needs to be able to express the start of the sending, so the receiver can react at that point if he has the file already).

[Ben-Lichtman]

Even if you don't want to fully sanitise the path, simply checking for "../" will get you most of the way there (on Unix). A more complete option might be to disallow directories entirely and strip out only the final element of the inputted path ie folder/name/test.txt becomes text.txt

[lemunozm]

To summarize, the complete workaround could consist in:

• Avoid user names out of the [0-9A-za-z_]* language.
• Use only the final element of the file path.
• Check if the file exists (modifying the underlying messages). if it is there, then add a simple count in the final of the name (for example).
• bonus: Show the path where the user can find the file once it has been completely received.

Regarding the possibility of allowing to receive a file before sending, it could be tricky to implement since a file can be sent to different users. What happens from the sender side if some users accept the file at different times? Would I need to reopen the file for each user?

[5225225]

Personally, I would prefer usernames stay more open than that. It seems reasonable to expect users to want to put / in their name. So sanitizing on use seems a better idea.

That does come at the cost of being harder to safely work with, but at the moment there is only one place that needs to use the name. (Maybe a newtype wrapper Username(String) and a function that goes from username + filename to open file, and have that function handle all the safety critical stuff?)

As for multiple people accepting at different times, I think opening the file once per receiver is the best bet (but you'd want to be careful about someone spamming receivers and trying to get you to open the file too many times and exausting your open files, so a limit of some value is a good idea there)