keybase / keybase-issues

A single repo for managing publicly recognized issues with the keybase client, installer, and website.
902 stars 37 forks source link

Symbolic links in KBFS should be restricted to a safer set #2086

Open timmc opened 8 years ago

timmc commented 8 years ago

I would encourage the Keybase developers to restrict what symlinks can be written to KBFS as a security precaution: Symlinks should only point outwards from more private areas to less private ones; from places definitely under the user's control to places possibly not under their control.

From the KBFS docs, a rationale for allowing them:

The Keybase client allows symbolic links that lead outside a TLF [Top Level Folder]. This is by design, and we envision a variety of great use cases:

  • a subfolder of your public folder, where you link to friends' public folders you endorse
  • storing private links to all your favorite private folders
  • a link entirely outside KBFS to another global filesystem you endorse. For example, to something in IPFS.

The text then goes on to describe various situations in which a user could be tricked into revealing private files.

Incautiously handled symlinks are the cause of a great number of security issues. I like to watch the security updates as they come in to my OS. The top type of problem is of course unsafe memory operations (buffer overflows and whatnot), but a close follower is symlink mismanagement. People are notoriously bad at handling symlinks safely. "Don't do that then" has a pretty bad track record in this area.

Here's a sample proposal, although I would entertain many variations on it:

This would avoid situations where an attacker is able to place a symlink to OS files, user data files, or private KBFS objects on the victim's computer as part of an attack based on mistaken file identity. Further consideration might reveal other combinations of source and target that are safe, such as the aforementioned links to IPFS.

strib commented 8 years ago

@timmc, thanks very much for the thoughtful writeup! I tend to agree with your points. The technical reason we haven't restricted symlinks more is that they can be moved arbitrarily by users after they are created. For example:

ln -s ../../../etc/password /keybase/public/strib/a/b/c/
mv /keybase/public/strib/a/b/c/passwd /keybase/public/strib/

So we can't just police this at symlink-creation-time, we have to do it on moves, and probably on client reads as well to protect against malicious clients. But I think it gets a little tricky to refuse to serve symlink reads after a move, when the kernel might already have that data cached.

I think we just need to think through a bunch of issues like this, see what the overall community wants, and figure something out. We have other pressing features to implement at the moment, but I'll put this on our internal issue tracker, and keep this open so others can add their thoughts.

malgorithms commented 8 years ago

agreed - this is a very thoughtful writeup. I tend to agree with the suggestions, too. In the short term, if we need to choose between all symlinks or no symlinks, I'd still choose for them to exist. But I like the 4 rules above for possible restrictions. I guess the only one I question is unshared private symlinks.

Given that you're writing and reading private symlinks only for yourself, it seems you should be able to write whatever symlinks you want. Just like you could in your home directory or anywhere else outside of KBFS.

@strib - yeah, we'd definitely have to restrict this on both the write and read side. On the write side, we'd want to block the write of an unacceptable link and alert the user what they did and why it's not acceptable (if electron installed). On the read side we'd need to check again just in case someone had bypassed the write restrictions.

There may be alternative solutions here. For example we allow all symlinks, but a user gets GUI alerted when they try to access one - and they have to approve it on any machine before KBFS will allow read access to it. Just trying to think outside the box...

We'll have plenty of time to talk about this, because I think the short term story is to allow them.

timmc commented 8 years ago

Given that you're writing and reading private symlinks only for yourself, it seems you should be able to write whatever symlinks you want.

Ah, fair -- unshared private should be unrestricted. I'll make that change.

ghost commented 8 years ago

I think it might be more prudent, and simpler, to disallow symlinks for the time being. Add them later as a feature. Focus now on getting the key security and usability issues ironed out. This is, IMHO, an edge case which can be more or less mitigated for the time being by simply not having symlinks.