Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a whitelist of commands for the upload #170

Open
wants to merge 3 commits into
base: devel
from
Open

Conversation

@matteosuppo
Copy link
Member

@matteosuppo matteosuppo commented Aug 24, 2017

Fix #165: Rather than signing commands on the server, the commands are hardcoded in commands.go, and the client sends the id of the command instead of the command itself.

Fix #165: Rather than signing commands on the server, the commands are hardcoded in commands.go, and the client sends the id of the command instead of the command itself.
matteosuppo added 2 commits Aug 30, 2017
This doubles as a security measure against malicious attacks
This ensure that a malicious user cannot inject arbitrary code in the commandline
Copy link
Member

@facchinm facchinm left a comment

Intel boards are missing; I can see the benefit of this approach but the maintenance burden is HUGE

@gitname
Copy link

@gitname gitname commented Jan 23, 2018

Please see the disclaimer at the bottom of this comment prior to reading the remainder of this comment.

I read the conversations in Issue #165 and in this Pull Request.

I think one compromise could be to add a switch (e.g. a checkbox or pair of radio buttons) to the agent, that the user can toggle between two states: (A) Normal mode and (B) Dangerous mode (required for special configurations) (someone else can choose the actual wording).

When mode (A) is selected, the agent would only issue commands that exist in some white list (e.g. the one @matteosuppo included in this PR). That white list may cover the most common use cases; but not all use cases, due to the maintenance burden (which @facchinm pointed out) of keeping the white list up-to-date with new use cases. This would be the default mode.

On the other hand, when mode (B) is selected, the agent would issue any arbitrary command (that's my understanding of its current behavior, after having read the conversation in Issue #165). That would cover the remaining use cases.

The act of switching the mode from (A) to (B) could involve reading a warning and agreeing to it. For example:

WARNING: By enabling Dangerous Mode, you are allowing Arduino Create to issue
arbitrary commands on your computer.

Click CONTINUE to enable Dangerous Mode or click CANCEL to remain in Normal Mode.

I think this approach would make updating a white list (over time) less urgent than if the agent would only run commands that are on the white list. At the same time, I think it would keep more users safer by default.

* Disclaimer: I haven't used the agent yet (I was evaluating it when I came across Issue #165 and subsequently decided not to install it).

@hannobraun
Copy link
Contributor

@hannobraun hannobraun commented Jan 26, 2018

@matteosuppo Can you please tell me what the status of this pull request is? @mastrolinux pointed me to this pull request as something I can help with. Do you need something specific, or do you want me to try and take this over from you?

@matteosuppo
Copy link
Member Author

@matteosuppo matteosuppo commented Jan 26, 2018

@hannobraun we're still evaluating our options.

The problem is that there are a lot of different commandlines. Another option would be to whitelist the programs that can be ran (which are basically avrdude, bossac, and few others) and keep the rest of the commandline free. Keep in mind that bashisms such as && or ; or $() wouldn't work, so it should be pretty safe.

@gitname in extremes cases we could do that, but I'm not really a fan of adding complexity to the ui

@mastrolinux
Copy link
Member

@mastrolinux mastrolinux commented Jan 26, 2018

@matteosuppo we do have a working experiment that solves the issue to override config. I will show you more later today, we can probably make a PR about that and add what @gitname is proposing, which I think is a really good approach.

@matteosuppo matteosuppo force-pushed the devel branch 4 times, most recently from fee3d17 to b0b3b31 Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.