Insecure download of amalgamation

[dionyziz]

The following is a non-HTTPS download of unsigned code that is downloaded and executed on the user's machine. This is trivial to tamper with and insert a virus into for any attacker with very basic techniques:

https://github.com/leapcode/pysqlcipher/blob/e42f5a80391f7a4cb3b1305990ab3842ac2bea53/setup.py#L125

This puts all users of pysqlcipher at risk. As pysqlcipher is a security-related library, this issue is critical. In particular, because it's used in our project, OpenBazaar, a project related to bitcoin, it is a vector for bitcoin-stealing malware.

This should be replaced with either of these options: (1) An HTTPS-based download (2) An insecure download followed by checksum verification, where the checksum is hard-coded in the installer

[elijh]

I think a checksum verification is greatly preferred, although both would be nice.

[kalikaneko]

I wasn't counting the amalgamation to be used in production environments (linking against system libraries is preferred, see the debian package), but you're right. No excuses on my part, I'll move the amalgamation to https uri asap.

[kalikaneko]

merged fix to develop. will tag release with fix real soon.

[kalikaneko]

Maybe we should make the default to try to compile against system libsqlcipher, and leave --bundled as a secondary option that will download amalgamation. Less convenient for pip users, but more secure. Thoughts?

[dionyziz]

Thanks for taking immediate care of this :+1:

[Renelvon]

@kalikaneko I 'd prefer to keep the --bundled option as default for a little bit longer, since libsqlcipher isn't readily available on Travis CI (running 12.04) and we will have issues with our build. But of course, linking against a system library is a much much saner idea.

[yurivict]

I also have a problem with amalgamation download. I am making FreeBSD port, and any direct downloads aren't allowed in the port.

Besides security issues, there is also no explanation why amalgamation is chosen over the simple SQLite3 dependency, which would have been the standard way of using SQLite.

Even solution suggested by #dionyziz still leaves custom download on which will cause headache for every packaging system which tries to adopt pysqlcipher.

I suggest you make amalgamation download an option in setup.py, making the SQLite dependency the default.

[kalikaneko]

@yurivict: glad to hear you're working on the freeBSD port!

Besides security issues, there is also no explanation why amalgamation is chosen over the simple SQLite3 dependency, which would have been the standard way of using SQLite.

pysqlcipher does not use sqlite, it uses sqlcipher, which is a different codebase. The amalgamation used here is the sqlcipher amalgamation, not the sqlite one.

at the moment of creating this python package, libsqlcipher was not packaged for debian or any other distribution I know, and the only reason for the download to exist is just that it was the quickest way to get the library into a developer environment with near to zero hassle. I just placed a manually built sqlcipher amalgamation replacing the sqlite one in the original pysqlite code to simplify development process.

However, now that more people is using this code, I acknowledge that this situation is far from desirable. I just realized that ghaering also removed the amalgamation download from pyslqite, I'm guessing that for the same reasons discussed here:

https://github.com/ghaering/pysqlite/commit/2d7360cc94c652b8ebfd063940beeaa7451db9dd

For the package building needs, what we do for the debian package is to link against system lib instead, see here: https://github.com/leapcode/pysqlcipher#build-against-libsqlcipher

My proposal is to wait a little time (1 month?) so we won't be breaking people's builds without previous notice, as @Renelvon asks, and transition to making the linking against installed libsqlcipher the default in the next release (2.6.5). We could make an optional --bundled option to download the needed stuff, or completely remove it and leave it under the user responsibility. Thoughts?

[kalikaneko]

@dionyziz I've uploaded version 2.6.4 to pypi, it's hidden by the moment. can you guys try it on osx to see if the wheel is working ok?

pip install pysqlcipher==2.6.4

[kalikaneko]

For the optional --bundled option I was thinking about something similar to what pyzmq does:

pip install pyzmq --install-option="--zmq=bundled"

but I would rather not ship the sqlcipher source in the python package. So I guess that adding a hardcoded hash to the url to have an extra guard in there would be a good solution.

https://github.com/zeromq/pyzmq/wiki/Building-and-Installing-PyZMQ https://github.com/brandon-rhodes/pyzmq-static

[dionyziz]

@kalikaneko We appreciate you taking the time to work on important security issues. Your next beer is on us ;) @changetip

[changetip]

Hi kalikaneko, dionyziz sent you a Bitcoin tip worth 1 beer (8,897 bits/$3.50), and I'm here to deliver it ➔ collect your tip at ChangeTip.com.

Learn more about ChangeTip

[yurivict]

Even if sqlcipher is made a dependency, it installs libsqlite.so with the same name as mainstream SQLite, and most systems already use SQLite. They need to rename their libs to be practical.

[yurivict]

Hi @kalikaneko , I think the original conflict has been resolved in libsqlcipher. Do you know when you will be able to fix and close this bug? I think OpenBazaar folks depend on it for their upcoming release, and still have to use amalgamation.

[kalikaneko]

hi @yurivict. If you're talking about the non-ssl download of amalgamation, this was fixed in 2.6.4 release of pysqlcipher now available by default in pypi. If you're talking about compiling against the the libsqlcipher dependency, that's indeed possible, see the README.

I think it's not a problem that the extension in pysqclipher is called _sqlite.so, since it doesn't collide with pysqite namespace.

Closing this issue.