Use securerandom for control token

ioquatix commented 7 years ago

https://github.com/jc00ke/guard-puma/blob/fecd9d94d386b8831ac47b92b434e4803df85e9c/lib/guard/puma/runner.rb#L11

I think you should use SecureRandom.hex(32) or something like that. It may not be a big issue, but there is potential for that to backfire... it's probably just good practice not to use a fixed control token.

jc00ke commented 7 years ago

Since Guard is generally used for dev and test I'm not too concerned with the control token being static. Thanks for looking out though.

ioquatix commented 7 years ago

Would you accept a PR for this?

jc00ke commented 7 years ago

I'm not convinced using a static control token is insecure, so no, I wouldn't accept it. However, I'm open to being convinced if you can show me how a dynamic token is more secure while still being user friendly.

ioquatix commented 7 years ago

Well, here is what I think.

Secure by default is good.

Using a static token is not secure. By nature it means that someone could control puma.

Any system which allows multiple users to log in, is a risk.

There are plenty of situations, but imagine a university with computers set up for development. At least in my case, it was possible to log into any machine via SSH. In this situation, one user could take control of another user's instance of puma if it was being managed by guard-puma.

The user should never see the token by default so it would not in any way be less user friendly. In fact, it's more user friendly in a technical sense because it's more secure.

jc00ke commented 7 years ago

Hmm, I'm still not convinced it's worth defaulting to a dynamic token. My thoughts:

there are times when one may want to control a puma instance started by guard, and it's handy to not have to always go get the control token
if I did want the dynamically generated token I'd have to print it out to the screen, which isn't secure
we already have the option to pass in control_token: SecureRandom.hex(32) when configuring guard, and I think that gives you the security you're looking for.

I guess I'm not really concerned that an attacker might get a hold of a running puma instance since it's really not meant to be run in a production environment. SSH access to the computer you're running is more of an attack surface than guard-puma.

Thanks for the suggestion and the conversation, but I'm gonna deny this change for now.

ioquatix commented 7 years ago

Optional security is no security at all

I'll agree with you in general - I don't think that this gem is significant enough to be an issue. However, sometimes it only takes on backdoor to escalate privileges or cause havoc.

if I did want the dynamically generated token I'd have to print it out to the screen, which isn't secure

You're right, so you shouldn't be printing it to the screen.

there are times when one may want to control a puma instance started by guard, and it's handy to not have to always go get the control token

What use cases do you see here?

I guess I'm not really concerned that an attacker might get a hold of a running puma instance since it's really not meant to be run in a production environment. SSH access to the computer you're running is more of an attack surface than guard-puma.

This isn't just about production, it's about any environment where multiple users can access the same machine. SSH access to a machine doesn't imply insecure.

Security should be by default, and yes, sometimes it's a little bit inconvenient.

ioquatix commented 7 years ago

@evanphx it would be great to have your input here.

ioquatix commented 7 years ago

Okay, this issue became much more serious.

I noticed something while using guard-puma.

* Listening on tcp://0.0.0.0:9292
* Starting control server on tcp://0.0.0.0:9293

0.0.0.0 is all network interfaces.

I run guard-puma on my laptop, but I can control it from my desktop.

koyoko% pumactl --control-url tcp://fukurou.local:9293 --control-token pumarules status
Puma is started
koyoko% pumactl --control-url tcp://fukurou.local:9293 --control-token pumarules stop  
Command stop sent success

On fukurou.local:

^_^ > rake
Generating transient session key for development...
01:17:33 - INFO - Guard::Puma will now start your app on port 9292 using development environment.
01:17:33 - INFO - Guard::RSpec is running
01:17:33 - INFO - Guard is now watching at '/Users/samuel/Documents/Programming/ioquatix/www.codeotaku.com'
Puma starting in single mode...
* Version 3.8.2 (ruby 2.4.0-p0), codename: Sassy Salamander
* Min threads: 0, max threads: 16
* Environment: development
[1] guard(main)> * Listening on tcp://0.0.0.0:9292
* Starting control server on tcp://0.0.0.0:9293
Use Ctrl-C to stop
- Gracefully stopping, waiting for requests to finish
=== puma shutdown: 2017-03-24 01:20:28 +1300 ===
- Goodbye!

So, anyone using guard-puma without this PR can be controlled from anywhere on the local network. If there is a bug in pumactl it may lead to something more serious.

evanphx commented 7 years ago

If an instance of puma is tied to a specific usage of guard, then using SecureRandom to generate the control token is a good idea. Another idea is to generate one and store it in ~/.guard-puma and reuse it, so at least it's unique per machine.

jc00ke commented 7 years ago

I did a little digging:

Guard Rack listens on 0.0.0.0, as does Guard Passenger. I don't see anything specific to the binding address in Guard Unicorn.

I don't use virtual machines, but I could see binding to 0.0.0.0 to be a handy default for those that do and want to talk across the network.

So, 2 issues:

static control token
default binding of 0.0.0.0

I think the .guard-puma file option is interesting, and I can see having a per-project/per-user/per-machine file. That also seems like a lot for something that people might not really use.

I'd maybe rather have Guard listen for some key and then have the token written to a file in the dir running Guard so that the developer can use it. That way it doesn't get printed to the screen and doesn't automatically get written to disk.

As for the default binding of 0.0.0.0 I'm OK with changing it to 127.0.0.1 as long as that doesn't break cross-VM communications. I'm not going to install Vagrant and configure a box to do that though, so that'll be on someone else.

ioquatix commented 7 years ago

Thanks for looking into this issue further.

Guard Rack listens on 0.0.0.0, as does Guard Passenger. I don't see anything specific to the binding address in Guard Unicorn.

Thanks for finding some other examples, it's good to compare with.

Binding to 0.0.0.0 is okay for web server by default -- it's a reasonable choice, although one might consider localhost to be a more secure option -- but it's not suitable for an unprotected control server.

After reviewing both guard-rack and guard-passenger, I can't see any form of control server bound to 0.0.0.0, only the web server itself. They both use the pidfile for sending signals to manage the child process.

So, 2 issues: static control token and default binding of 0.0.0.0

To clarify, these are not separate issues: the issue here is binding the control socket to all network interfaces WITH a predictable token. This allows anyone to use pumactl to control the server.

The PR I've supplied fixes this issue. Whether the user chooses to write that to disk, print it to screen, is another issue entirely, but I don't think it's the responsibility of guard-puma because users shouldn't be interacting directly with puma in this setup anyway.

ioquatix commented 7 years ago

Thanks @evanphx for your feedback. @jc00ke just in case you didn't realise, that is the developer of Puma.

Using ~/.guard-puma might be beyond the scope of this project. If users want to set that up, they could do it themselves and add control_token: File.read('~/.guard-puma'). That way you don't have to implement policy (where to read/write the file, when to write, etc) in your code.

ioquatix commented 7 years ago

The other thing to consider here is whether Puma's control server should be used at all. A traditional pidfile would also suit and avoid this issue entirely.

jc00ke commented 7 years ago

Thanks @evanphx for your feedback. @jc00ke just in case you didn't realise, that is the developer of Puma.

Never heard of him. I'm not even convinced he's a real person, just the incarnation of talent and geniality.

I vaguely remember trying a pidfile years ago, but I don't remember where I got with it. I'm not stuck on the control server, but I'm also not very motivated to re-write the guts to avoid using the control server either.

I'll continue to think about how I want to handle this issue. I doubt I'll have time to work on it until next weekend.

ioquatix commented 7 years ago

There are really three solutions.

Use the PR as suggested.
Don't use the control server.
Don't listen the control server on 0.0.0.0 but instead 'localhost'.

Which one do you prefer, if you don't have time to write code perhaps I may.

jc00ke commented 7 years ago

I think changing the control server binding is the way to go for now. Thanks for the forthcoming PR.

ioquatix commented 7 years ago

Here is the PR. It also seems like you haven't committed version.rb as there is a new release 0.3.5 but version.rb still refers to 0.3.4

UPDATE: my bad, not sure but git didn't pull the update, odd. So, the version.rb was updated correctly.

ioquatix commented 7 years ago

Even with the PR to only bind to localhost, I'd still wish you seriously consider this PR as I'm of the opinion that security should be by default. There are issues with the current implementation. Additionally, I can't see any valid use case where a user should be running guard-puma and yet want to run pumactl independently, if you could explain this perhaps I'd understand your POV better and we could work towards a better solution.

jc00ke / guard-puma

Use securerandom for control token #23