Security: Don't take commandline as input in upload POST

[rakeshpai]

I'm an amateur, so please feel free to correct me if I'm wrong below.

The commandline parameter in the upload POST is very bad security practice. On no account should arbitrary strings from an external source be executed directly in the shell.

You'd argue that the command is signed with public-key cryptography, and is (mostly) unbreakable, and that's correct. However, it's only secure if your private key is not compromised. You'd then argue that if the private key is compromised it's all over anyway, and you'd be right again. But it's not just your machines that would be vulnerable when the key is compromised, but the attacker will also be able to execute commands on all of your users' computers as well, since arduino-create-agent will trust all such commands.

Instead I'd suggest that you build the command at the place where you'll be executing it (in arduino-create-agent when uploading over serial, and on your server when using network uploads). When building the command, avoid using user input directly in the command, and use them only as flags that enable/disable parts of the command. If you can't avoid interpolating user input, consider that a security vulnerability, and proceed with extreme caution to sanitise the input, and never trust that your sanitisation code works perfectly. If this is done right, you won't need signatures for the command anymore, and will have simplified the whole setup.

Also, I'm not sure why the upload via the network is done over ssh/scp. It unnecessarily introduces more attack surfaces that you need to safeguard. Use plain HTTP requests instead - it's well understood, has lots of tooling around it, scaling it is well understood, and most importantly doesn't give execute permissions on the shell. You already use HTTP as a fallback from my understanding.

[facchinm]

Hi @rakeshpai , everything you say is correct. However, the composition of the command needs (at least) the parts to be composed, and it's not feasible without an additional "commands" file which should be signed too (and so can be compromised). The attack surface would be diminished but not removed. About the SSH upload, it's compulsory for older Yuns which will never be updated to newer firmwares; newer boards use plain HTTP endpoints with passwords and the target is to deploy an even easier upload method on Linux based boards.

I'll think about the maintenance effort if we adopt a different approach and report you back. Thanks!

[matteosuppo]

The commands are usually built by the builder-api. The agent could use that (but it would diminish the reusability of the project)

[rakeshpai]

I'd suggest baking the commands into the distribution binary itself. So, the commands sit as a .go file in this repo, and not as a separate file. If the code that generates the commands needs to be reused elsewhere, it could be packaged as a go library/module/package (not sure what the terminology is). That way, the only way to attack the setup would be to patch the binary itself.

I'd argue that executing user strings into a shell, while very flexible, is too powerful to be comfortable with from a security standpoint. A hand-curated subset of commands is far more secure. That way, even if the private key is compromised, the most an attacker can do is flash an Arduino, not take over the machine.

[matteosuppo]

Problem is, you would require a new arduino-create-agent version each time the commands change.

[rakeshpai]

That is correct. However, there should only be a few commands anyway. I'm not sure how boards-manager works in the desktop IDE, but I'm guessing the command to flash the device is coming from the tool itself. So, theoretically, if a tool is installed, it should have enough information to construct the command. Actually, commands don't need to be specified in the agent at all, it could simply pick it up from the tools files. (I'm making wild guesses about how things work here. I'm sorry if I'm wrong.)

It would be very hard to build trust in the setup if Arduino's servers can send down strings to a client at runtime, based on server logic which can't be audited, which the client then executes in a shell.

[fallberg]

Look, guys, you can be serious with this type of implementation. You basically hand over every client on a silver platter (and that includes yourselves). What prevent me from extracting the security credentials from your binary to access your cluster through ssh? Take it apart and please rethink this one. The road you are going with this will lead to your sw being branded malware by every single security software out there (including those built into the OSes).

[facchinm]

Mmmmh, I'm not getting this one. What we "bundle" in the binary is only the "decryption" key (https://github.com/arduino/arduino-create-agent/blob/devel/tools/download.go#L65) like in any asymmetric encryption protocol. There is no (known) way to use it to extract the encryption key (and if it happens, this project will be the very last problem :smile: )

[rakeshpai]

My bad. The SSH/SCP totally scared me, and I assumed it was connecting to your server using auth creds baked into the binary. I didn't realise that that's how Yun worked.

The ability of the server executing shell commands at the user's end still stands though.

[fallberg]

Yes, keys aside, executing arbitrary shell strings is always a bad idea and bad practice. You really need to adress that.

[mastrolinux]

@rakeshpai how boards-manager works in the desktop IDE, but I'm guessing the command to flash the device is coming from the tool itself. So, theoretically, if a tool is installed, it should have enough information to construct the command this is note entirely true, the command line is constructed by the arduino builder at runtime based on the target board + used libs + etc., that's the magic of Arduino and there is no simple way to know all the different commands in advance, in addition with multiple libs + cores there can be more than few hundreds of different command lines, maybe even more. But is also true we have this long list of different commands for each tool and we are already working on having them pre-composed and in local cache file. The problem is it will take some months before releasing this improvement, it is already in our roadmap. In the meantime feel free to use the Arduino IDE instead of Create.

@fallberg What prevent me from extracting the security credentials from your binary to access your cluster through ssh? the fact that the credentials are not in the binary is what is preventing your to access them. Please read the code here: https://github.com/arduino/arduino-create-agent/blob/7b4122f9e813ee46fc876e04417a8338f551af92/conn.go#L190

Trust me we are taking this seriously and this is also why now we do not accept cores from third parties we cannot control completely. We will keep you updated.

[rakeshpai]

Thanks, @mastrolinux.

Just to drive the point home, if you allow me to make a ridiculous claim, I could claim that Create runs (or is geared up to run) a botnet, where it can enlist all computers that have arduino-create-agent installed, and remotely control them to do it's bidding, by executing arbitrary code on the user's machine. It might have done this already, and maybe installed a dormant daemon on users' computers waiting to wake up on a command from the mother-ship. There's no way a user could have known. A look at the code for the agent would show that this is entirely possible.

In this scenario, we don't even have an attacker. Instead, what's going on is Arduino is saying, 'Give us shell access to execute arbitrary code as we see fit from our server. We'll never do anything wrong. Promise.' and the user has to place immense trust in you. I'm sure you see why that's not good enough.

I do trust that you are taking this seriously. It is a pretty big issue, I'm sure you understand. :+1: I'm excited for this project, and am looking forward to recommend it to people to use.

[fallberg]

@mastrolinux ok, it's just that this line looks like a nice please to use a debugger to access credentials: https://github.com/arduino/arduino-create-agent/blob/4fb14ba971116a11ba6cb7737e09a48b0e952cbc/upload/upload.go#L379

But if you say you got it covered, I suppose I have to trust you. Considering that you want to run arbitrary commands in my shell I have to put considerable amount of trust in you anyway...

[mastrolinux]

@rakeshpai thanks for your advice, I do agree with you, indeed it opens to us a shell to the client machine. As said we are working on this and Pull Requests to solve the issue are very welcome. This is also a huge workload and will take a while to be solved.

@fallberg I do not get your point at all, the key is a public key and there are no private secrets shared anywhere in the code.

[rakeshpai]

:+1: I can see why this is a big undertaking. Thanks for considering it. I wish I could help with code, but the last two days is literally the first time I've read Go code. (Side note: I'm impressed by how readable it is! I should look at Go more closely.)

In @fallberg's defense, I take the blame for the misdirection. (Edit: we've been having a private conversation stemming from evaluating arduino-create-agent for another project that I'm working on.) I saw the SSH/SCP in the code, doing a network upload and remote execution, with creds picked up from some place locally, I didn't consider Yun, and assumed you were executing commands on some remote server, which would have been another can of worms. Entirely my fault for not understanding what was going on. It's probably a good thing that this technique is deprecated now anyway. Cheers!

[rakeshpai]

Good morning! Now that I've slept over it, it seems like the solution is not as complex as initially thought. This project isn't doing any compilation anyway, so presumably you are compiling in the cloud. This means that all the calls to avr-gcc, which is the arduino magic, including the linking of libs and dependencies, are all on your servers. The only call in the agent should be to avrdude to flash the chip. In fact, from the example in the readme, the command-line looks as follows:

"{runtime.tools.avrdude.path}/bin/avrdude" "-C{runtime.tools.avrdude.path}/etc/avrdude.conf" {upload.verbose} -patmega32u4 -cavr109 -P{serial.port} -b57600 -D "-Uflash:w:{build.path}/{build.project_name}.hex:i"

...which should be all that's needed in the agent. Either the agent or client-side JS knows all these parameters locally, so it doesn't need a server to tell it what command to run. And the number of variations in this command are very small - board, port and hex location are the ones most likely to change - and these params don't require 'eval'ing arbitrary strings from remote sources.

Point being, chances are, the most complex commands are running on your server, so the agent command line has very minimal possible variations. So, it shouldn't be too hard to remove the need for the server to tell the agent what to do, and hard-code the command construction logic in the agent.

Just my 2c. Not sure how this fits in with downloaded tools.

[mastrolinux]

@rakeshpai a lot of partners have a lot of different installers and platform, you mentioned just AVR but we support more than 50 different cores (sam, samd, esp, etc. here an incomplete list of cores: https://github.com/arduino/Arduino/wiki/Unofficial-list-of-3rd-party-boards-support-urls + https://github.com/arduino?utf8=%E2%9C%93&q=arduino-core&type=&language=) and each of them can have easily (and they do) 4-5 different tools with different command lines. You are hiding some complexity, if it was easy we already did it, if we did not do that it is because this problem is hard.

If you look closely to https://github.com/arduino/arduino-create-agent/blob/607334d2ae9af54e4e2c63dc03281531d4852b44/upload/upload.go#L53 you can find user cannot just inject everything it wants in the command line and as of now is not that easy to exploit (I am not saying it is impossible, but just that we already mitigated the issue a bit) So this issue will be kept open while until we will have a proper solution. But is not as simple as you describe. If you really think is easy we are waiting for your Pull Request to fix the code.

[rakeshpai]

You've used an interesting conversational style with the 'if you think it's easy, why don't you fix it yourself' style comment. I've at no point claimed that it's easy. The arduino magic of managing and linking-in libs and dependencies that you brought up, having thought about it, isn't relevant to the issue at hand, and we can limit the scope of the issue to just flashing. That's what I said. I'm trying to help find a solution for a security issue in your software on my own time. I can understand if you would rather not have help. That's fine. But it's interesting that you choose to use a confrontational tone.

I couldn't find a list of boards that Create supports. Does it support all 50+ of the list you linked to, along with the third-party ones, with the third-party tools being executed both on the server, and on the user's machine?

I've said it before: this isn't just about executing injected user strings. It's about handing over users' computers to Arduino's servers to do with it as they please. I'm pretty sure none of your users would be comfortable with you having the power to remote-execute code on their machines, no matter how altruistic Arduino's goals might be. It's hard to overstate this point. Making light of it is not a solution.

Also, the PartiallyResolve function you pointed out does nothing for security. You can test it here. (Yes, I learnt just enough Go to put this test together.)

I've said more than once that I like what this project is aiming to do, and I want to recommend it to people, which is why I'm discussing here. Not all contributions are code, and I'm contributing by giving the app a once-over, and giving you feedback, from an outsider's perspective, on my own time. Interesting choice of language you have then. I'm sure we can make an effort to stick to technical arguments, and not let the conversation devolve.

[fallberg]

I apologize for the misunderstanding. Please disregard my concerns for those attack vectors. But reading the following discussion I must express my concern for the way this legitimate issue and concern has been received. If you think solving this problem in a less authorititive manner is such a complex task (and I am not saying it isn't) then perhaps you should consider it being still under development and launch it once you have resolved it. Or at the very least be very clear to the users of the tool of what they agree to let you do.

But at least I am glad to see that you agree to keep the issue open until you have a proper solution. That confirmes that we are in agreement that the current solution is not a long term viable solution. Whether or not the current one is good enough for launch can always be discussed but it is up to you to decide that of course.