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

Security flaw allowing arbitrary remote code execution #25

Closed
Rogdham opened this Issue May 16, 2013 · 2 comments

Comments

Projects
None yet
2 participants
@Rogdham
Contributor

Rogdham commented May 16, 2013

Hello there,

You use pickle to transfer the informations between the server and the client.

This has two problems:

  • waste of resources (mostly bandwidth)
  • major security flaw: only trusted strings should be unpickled, and obviously what is on the wire is not trusted, be it just because other users are not trusted, or the server is not trusted.

As of current master version 2e8006f, this allows for:

  • a malicious server to execute arbitrary code on all clients connected to him
  • a malicious client to execute arbitrary code on the server it is connected to, and possibly on all clients connected to that server as well (edit: I tried that one too (locally of course), it works as well, but required more work)

Easy and quick fix: replace all pickle by json. The api is the same (i.e. loads and dumps), but for some reasons when I tried to change it, it failed on the client side. Since the python client code is wrapped in some vim plugin code I don't understand and this changes all lines numbers, it's a pain for me to debug. So if you could help, it would be great. Here is my branch for that: https://github.com/Rogdham/CoVim/tree/pickle-security-flaw (error around line 77 in client.vim)

This quick fix does not solve the waste of resources though, only the security flaw.

Also this breaks compatibility with prior versions. But you will have to break compatibility anyway in order to fix this security flaw.

Note: I'm not disclosing the exploit for that bug until it is fixed for obvious reasons, but this is not difficult to create it given what I said before.

@FredKSchott

This comment has been minimized.

Owner

FredKSchott commented May 17, 2013

I'm currently working on this. We actually were using json instead of pickle initially, but switched because json encoded strings to unicode, while pickle didn't. I didn't know there were security issues involved, i'll look into changing it back.

Rogdham added a commit to Rogdham/CoVim that referenced this issue May 17, 2013

Replace pickle by json. Fixes security flaw FredKSchott#25.
Breaks compatibility with prior versions.
@Rogdham

This comment has been minimized.

Contributor

Rogdham commented May 17, 2013

Yes, it is a shame that they are such security flaws in pickle, but as written in a big red box in the documentation:

Warning: The pickle module is not intended to be secure against erroneous or maliciously constructed data. Never unpickle data received from an untrusted or unauthenticated source.

Could you take a look at #36?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment