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

/tmp/fishd.socket.user permissions checking (CVE-2014-2905) #1436

Closed
zanchey opened this issue Apr 28, 2014 · 11 comments
Closed

/tmp/fishd.socket.user permissions checking (CVE-2014-2905) #1436

zanchey opened this issue Apr 28, 2014 · 11 comments
Assignees
Milestone

Comments

@zanchey
Copy link
Member

@zanchey zanchey commented Apr 28, 2014

Neither fish nor fishd check the credentials of processes communicating over the fishd universal variable server UNIX domain socket.

@zanchey zanchey closed this in ba1b5e3 Apr 28, 2014
@zanchey
Copy link
Member Author

@zanchey zanchey commented Apr 28, 2014

In fixing this issue, I went with getpeereid(3) on platforms that support it, borrowing the fallback implementation from PostgreSQL.

I decided against manipulating umask, checking permissions or checking socket owners because none of those are guaranteed to be respected by the socket API - in particular, on Solaris, anyone can connect to a socket filename regardless of owner or file mode.

@zanchey zanchey added this to the 2.1.1 milestone Apr 29, 2014
@zanchey zanchey reopened this Apr 29, 2014
@zanchey
Copy link
Member Author

@zanchey zanchey commented Apr 29, 2014

It has been pointed out that this probably isn't a sufficient fix; there is no verification that there is a fishd process is at the other end, and so a symlink can be used to cause fish to talk the fish protocol to other sockets owned by the same user - such as a program which may not be expecting fish input.

getpeername(2) could be used, but it doesn't dereference the symlink on Solaris.

I think, unfortunately, we are going to have to move the socket path, and I don't think there's any way of falling back to the old path securely, so this will require a restart of all running fish instances before universal variables work.

@lledey
Copy link
Contributor

@lledey lledey commented Apr 29, 2014

I saw @ridiculousfish's work on death_of_fishd branch. Wouldn't that solve the issue since fishd would be gone (if I get it right)?

@zanchey
Copy link
Member Author

@zanchey zanchey commented Apr 29, 2014

Yes, but that is not yet ready and is a intrusive change that may not be suitable for a point release.

@lledey
Copy link
Contributor

@lledey lledey commented Apr 29, 2014

That makes sense.

@zanchey
Copy link
Member Author

@zanchey zanchey commented Jun 29, 2014

I dropped the ball on this a bit but I think it's time to get the fix out.

My plan at present is to move the socket path entirely, using a secure path like tmux, and give up on getpeereid. What I haven't been able to work out yet is how to ease the upgrade path.

The problem with moving the path is that it is possible to have two versions of fishd running at once, and they will not cooperate to write the universal variable store file, leading to a race condition which may cause loss of data.

The options are:

  • Have fishd provide old and new paths (with hardlinks) - but it is difficult to tell the old fishd to shut down. Sending SIGTERM to the process at the other end of the old (/tmp/fishd.socket.user) socket is fraught with danger, and I don't have any ideas for how to work out the difference between old and new-style fishd processes.
  • Have fish connect to the new socket, then the old socket if that doesn't work, but this reintroduces the same security issue.
  • Tell users and distributors to restart all running fish and fishd instances - currently the only viable option as I see it.
@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Jun 29, 2014

A killall fishd should do it. Maybe that could even go into the install script?

zanchey added a commit that referenced this issue Aug 3, 2014
This reverts commit 8412c86.

Just checking the credentials of the peer turns out to be insufficient.
See #1436.
@zanchey
Copy link
Member Author

@zanchey zanchey commented Aug 3, 2014

OK, I've added this in 4cb4fc3, review would be much appreciated.

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Aug 3, 2014

Thanks zanchey, overall this looks very solid. Two suggestions:

  1. common_get_runtime_path has leaks memory in its return value. Suggest returning std::string instead.
  2. In the case in common_get_runtime_path where check_runtime_path has failed, let's print a more actionable error message, e.g. "Try deleting the directory %s and restarting fish."
@zanchey
Copy link
Member Author

@zanchey zanchey commented Aug 4, 2014

OK, I've made those and a couple of other changes in zanchey/fish-shell@2b5cd21. Does that look ok?

@zanchey
Copy link
Member Author

@zanchey zanchey commented Aug 6, 2014

If there's no other problems, I'm hoping to roll the 2.1.1 release in about 48 hours time.

zanchey added a commit that referenced this issue Aug 8, 2014
This reverts commit aea9ad4.

Just checking the credentials of the peer turns out to be
insufficient.
See #1436.
@zanchey zanchey assigned zanchey and unassigned zanchey Aug 29, 2014
jdxcode pushed a commit to jdxcode/fish-shell that referenced this issue Aug 28, 2017
This reverts commit aea9ad4.

Just checking the credentials of the peer turns out to be
insufficient.
See fish-shell#1436.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.