Skip to content
This repository has been archived by the owner on Apr 7, 2021. It is now read-only.

Don't write a PID file in pwd/tmp/ #28

Merged
merged 1 commit into from
Sep 6, 2013
Merged

Conversation

lfaraone
Copy link
Contributor

@lfaraone lfaraone commented Sep 6, 2013

If camo is running as a privileged user the current behaviour would allow a race condition leading to the overwriting of arbitrary files.

This is exploitable when running camo from an init script, since then your process as a working directory of /.

Since camo does not fork into the background there isn't really a reason to write its PID out to a file; people writing initscripts can simply use start-stop-daemon's --make-pidfile flag.

@atmos
Copy link
Owner

atmos commented Sep 6, 2013

Camo assumes you're starting the process from the directory the server.js is in. We use the pid file for monitoring in our system.

@atmos atmos closed this Sep 6, 2013
@lfaraone
Copy link
Contributor Author

lfaraone commented Sep 6, 2013

That assumption isn't documented anywhere. As such, there's a security risk for users deploying Camo in an environment where untrusted users have unprivileged access but can write to /tmp.

Since the README does link to this gist, I assume you're using god for monitoring. Your software runs in the foreground, and as such god can handle tracking your PID for you:

[…] god will take care of daemonizing it and keeping track of the PID for us. When possible, it's best to let god daemonize processes for us, that way we don't have to worry about specifying and keeping track of PID files.

I was looking to package this for Debian, but I can't prepare a safe package with the current, potentially buggy behaviour.

See symlink races for a description of the general problem.

@atmos
Copy link
Owner

atmos commented Sep 6, 2013

We actually run everything on heroku now so we can survive fine without the pid file.

Can you update the server.coffee and regenerate the server.js file?

@atmos atmos reopened this Sep 6, 2013
@lfaraone
Copy link
Contributor Author

lfaraone commented Sep 6, 2013

Ah, right, that. I can update the pull request, but you bring up something else: would it be possible to remove the generated server.js file entirely?

Requiring users to make use of coffee -c or run directly with coffee doesn't seem unreasonable, and I feel that's preferable to keeping the autogenerated file in git.

@atmos
Copy link
Owner

atmos commented Sep 6, 2013

Distributing the server.js means you don't need npm, any deps, or any path hacks to make it happy. Just run it with the node command. I'm in favor of developing against the .coffee file and generating the server.js file as needed. This project only gets a commit once every few months anyway.

If `camo` is running as a privileged user the current behaviour would
allow a race condition leading to the overwriting of arbitrary files.

This is exploitable when running `camo` from an init script, since then
your process as a working directory of `/`.

Since `camo` does not fork into the background there isn't really a reason
to write its PID out to a file; people writing initscripts can simply use
`start-stop-daemon`'s `--make-pidfile` flag.
@lfaraone
Copy link
Contributor Author

lfaraone commented Sep 6, 2013

I've made the requisite changes.

To comply with Debian policies around reproducibility of source code and shipping the preferred form of modification, I'll continue stripping out server.js from the source tarballs we ship and regenerating it during build; its trivial to depend on the relevant (already packaged in Debian) node/js packages and build it.

atmos added a commit that referenced this pull request Sep 6, 2013
Don't write a PID file in `pwd`/tmp/
@atmos atmos merged commit 9d46ff1 into atmos:master Sep 6, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants