Bookmark injection

[cym13]

The issue

It is possible to inject bookmarks without the user explicitely asking to add the bookmark.

How to reproduce

Create a filename such as test\na:/tmp/other. Since the new line character is valid in filenames on most filesystems (such as ext4) this results in two bookmarks being added at once.

The risk

Not much really, adding bookmarks doesn't seem very critical. Still, it can be annoying. One point though is that it can be stealthy by adding ANSI codes to the filename to hide any suspicious part of the filename.

The fix

Escape new line characters, by encoding them for example.

[mananapr]

Hi, I wasn't able to reproduce this issue.

Firstly, cfiles bookmarks directories and not files and forward slash is not a valid character for directory names (atleast in ext4).

Secondly, I tried making a directory named test\na:Public and bookmarked it to z. Then I pressed ' + z to go to test\na:Public and it worked.

You can see this terminal cast for example. Thanks!

[cym13]

I see the issue, it's not a forward slash, it's a newline "\n".

You may create it using the following shell command:

mkdir 'test'$'\n''a:Public'

Then go in this directory and bookmark it. I definitely could reproduce the issue in the latest version.

[mananapr]

You are right, the issue exists. How can I encode \n? To be clear I mean what would I encode it too?

[cym13]

I'd replace it with something that isn't allowed in a filename... Since you only ever take paths from the system maybe a double slash?

z:/tmp/test//a:Public

The worst that can happen is if you take a path from the user and she provides it with a double slash (which is valid but is reduced by the system to only one slash) but I can't see that happening in the case of bookmarks.

Either that or use an encoding convention, either limited such as saying "The % sign is followed by two characters giving the ascii code of the character, \n becomes %0A, % becomes %25" (url encoding), or go full encoding by storing base64 for example.

EDIT: I'm really not fan of the double-slash solution. I had a look at my bookmarks and saw a path starting by //, so I must have entered it somehow outside of related tests. My assumption that it can't happen seems flawed, I'd go with something like % encoding instead.

[mananapr]

There was a bug due to which paths used to start with // instead of / after visiting the root directory once. I fixed this in 666a935. So is the double slash solution good enough now?

[cym13]

I think so, and if it ever comes back to be an issue it'll still be time to change.

[mananapr]

I went with the double slash solution. rev 72771db has the relevant changes. Let me know if it has any errors.
Also sorry for the late reply, I have been getting a lot work from my uni.