search

anoadragon453 / qubes-file-trust

Service files and daemon for file-based trust levels on QubesOS
3 stars 3 forks source link

paths over-interpreted #2

Open jpouellet opened 7 years ago

jpouellet commented 7 years ago

https://github.com/anoadragon453/qubes-mime-types/blob/0b8b5a85522499462e48e00dc4915246a73ec790/qubesfiletrust/qvm_file_trust.py#L459-L461

This isn't your job, this is the caller's job. By doing this expansion yourself you allow attackers to potentially affect other files by inserting special (expanding) character sequences in file names.

anoadragon453 commented 7 years ago

Fair enough, removed these lines and the ability to add in anything besides absolute paths in 30da410.

jpouellet commented 7 years ago

Err, those aren't the same thing. ~/... etc. gets expanded by the shell (or other caller) before being passed to you. ~/... in the config file getting expanded to the current user is fine and I think makes sense as a feature to have.

anoadragon453 commented 7 years ago

I do end up getting '~' in the string though, I don't think it's expanded without calling expanduser.

I think os.path.expanduser only expands '~' to /home/.... Not sure if I can see any attack stemming from that. Should we just keep it in?

jpouellet commented 7 years ago

Virtually no program handles ~ in file path arguments specially (config files are a different story). This expansion in arguments is the job of the shell.

Double-interpreting it could indeed be dangerous. For example, an attack might involve extracting a zip file which creates a directory literally named ~, which may then allow escallation of filesystem access through path traversal to the users home directory (for example, leading to placing files in ~/.config, leading to arbitrary code execution on startup of some program).

anoadragon453 commented 7 years ago

Ok, re-read this and get what you mean. Won't double-interpret from the shell. Will expand from the config file.

anoadragon453 commented 7 years ago

Changed in e842039