denisidoro / navi

An interactive cheatsheet tool for the command-line
Apache License 2.0
15.01k stars 499 forks source link

Some concerns about security with variables #533

Open matclab opened 3 years ago

matclab commented 3 years ago

Navi is really nice and the ability to use cheatsheet from other people is very nice and allows to learn new tricks. The use of variable with fzf is a real usability gain.

But those two features together raises some security concerns, as it may leads a navi user to run malicious or erroneous shell commands with unwanted side effects.

I'm afraid I've no solution apart disabling variables or setting up a mechanism which would allows only accepted and reviewed code to be executed.

welcome[bot] commented 3 years ago

Thanks for opening your first issue here! In case you're facing a bug, please update navi to the latest version first. Maybe the bug is already solved! :)

denisidoro commented 3 years ago

Would a confirmation prompt before running the variable command help?

Example:

  1. Start navi
  2. Select a snippet (eg git checkout <branch>)
  3. Do you want to run the following command? branch: git branch yes/no
  4. Select yes
  5. Select a branch
  6. Snippet is executed / pasted into the shell prompt
enisozgen commented 3 years ago

That's a reason why I am always looking for the files :)

Yes/No question can be good! but asking many time yes/no questions not good user experience.

I guess, it shouldn't have to ask more than one time for permitted command.

matclab commented 3 years ago

I'm afraid that:

I'd like to see how other software cope with such a difficulty. I'll report here if I see some interesting solutions.

matclab commented 3 years ago

The more I think about it, the more it looks like the best solution would be a database (one for each known shell ?) of safe and accepted command, reviewed by humans.

The database could be:

enisozgen commented 3 years ago

Safe commands database is also not easy job since there are many different commands.

Who will decide for this? You need some kind of community for this decisions. Even if we review all those commands, in the future, behavior of commands can change nobody can give the full guaranty(There are very pessimistic ideas!!).

Every user have to care about his own security and it must be personal decision.

Since cheats can come from other people. User have to know that action can be harmful for him/her.

If there is a malicious command, at the first run of the $ block. Navi have to show outputs like

But trying to save user in that scenario shouldn't be navi's responsibility.

denisidoro commented 3 years ago

Thanks for all the input! This is a very good point.

(Un)fortunately navi doesn't emit any user usage metrics, so I can't support any hypothesis using data.

However, considering the relatively low activity in denisidoro/cheats compared to navi, I believe most users don't use downloaded cheatsheet and write their own instead. If this is true, this concern wouldn't hold for most users. I'll never know for sure, though.

That said, I think that asking for yes/no (+ caching the reply in a database/file) before shelling out is a nice trade-off, provided it's an opt-in feature (let's call it "safe mode") .

There are multiple scenarios where this can happen:

  1. Command for variable suggestion
  2. Command for --map
  3. Command for --preview
  4. Final command, if navi is invoked without the shell widget

The only problem is for --preview. In "safe mode" it should be completely turned off.

Since I only work on navi in my limited free time, I don't know when I'll be able to implement this feature, but if anyone reading this wants to contribute, I'll be more than glad to give some pointers on implementation. 👍