SECURITY: api.load() recklessly downloads & runs arbitrary python code

[gojomo]

The api.load() utility will grab fresh Python code, in the form of an __init__.py file inside the Github project gensim-data download area, and then run it at the user's machine.

This form of dynamic code loading & execution:

• violates user expectations: requesting a dataset should not run arbitrary new code that was never explicitly installed on the user's machine. Further, there's no indication in the api.load() docs that this could occur.
• creates a severe security risk: if a bad-faith actor obtains the ability to edit the gensim-data Github "releases" files, they can cause arbitrary new code to run on gensim users' machine, when those users use api.load(). The users wold think, "I'm still using this old, assumed-safe-through-widespread-use gensim-X.Y.Z version" – but they'd be running arbitrary all-new code from over the network. It's hard to tell who has gensim-data project rights. It's also not clear that anyone would quickly notice edits/changes there.

Further, these __init__.py files in the gensim-data releases aren't even in version-control – instead, they're uploaded as 'assets' via the Github releases interface. (There's no file-size need to do this; the __init__.py I reviewed, for wiki-english-20171001 at https://github.com/RaRe-Technologies/gensim-data/releases/download/wiki-english-20171001/__init__.py, is just a tiny shim and I imagine most other such files are, as well. It's code, it should be versioned.)

That they are not in version-control makes them hard to review through normal means (such as browsing the Github website), and raises the possibility they could be changed to something malicious, and then back again, without anyone noticing or it being visible in any persistent logs.

Recommendations:

The api.load() mechanism should be immediately redesigned to not load any new code over the network – and the developer guidelines for gensim & associated projects should make it clear such dynamic loading of code outside normal package-installation processes (like pip) is unacceptable.

If supporting new datasets requires dataset-specific code, that code should go through normal collaboration/version-control/release procedures, waiting for a new pip-installable gensim (or other new supporting project) explicit release before running on users' computers.

ATTN: @menshikh-iv @piskvorky @chaitaliSaini

[piskvorky]

-1 from me on all points:

• If a bad actor gains access to the repo, we're screwed anyway (python packages always execute arbitrary code)
• Why do you think there's an expectation of not running any code with a dataset? AFAIK the opposite is true: our contract is that the dataset comes with sane code to use it (as opposed to only raw data).
• Releases are meant to be immutable (never changed), so version controlling them makes no sense. Dataset files are published as Github releases (as opposed to tracked inside the repo itself) due to severe file size limitation on Github.
• Seeing who has gensim or gensim-data access rights is completely standard.

I agree we could try to devise a better scheme, where editing an existing release file triggers some warning / is impossible. Just to prevent accidents, not to stop malicious actors with repo write access (that's not possible).

[gojomo]

• If a bad actor gains access to the repo, we're screwed anyway (python packages always execute arbitrary code)

This is far more subtle and dangerous than that typical risk. With a normal package - like gensim itself - you only get fresh code when you pip install. As a result, there are more eyes on code as it enters the release branch, then gets approved for release, then winds up at a package repository. Even if all those steps fail, you're getting the same code as all others who install the same-versioned package, so if/when there's malicious code found, it's more likely to be discovered, and a loud alert sounded, and community remedial actions taken.

None of those mitigating factors help with this risky practice. The gensim api.load() code is loading and executing a small bit of code that's not in version control and was reviewed by no one but whomever uploaded it to the Github releases file area. (Further, from a quick glance, it appears release files can be added, changed, and removed without necessarily leaving any persistent public log.)

This isn't: "I'm trusting the maintainers of the project to get their major releases right, or for the larger community of users to eventually notice any problem." This is, "I'm trusting the maintainers (or anyone else who gets even temporary access to their github accounts) to inject new executable code onto my system at undisclosed later points in time, without any formal review or release process, or logging." That's very different from the threat of a bad-actor sending version-controlled code into the normal review/release/publish/install process.

• Why do you think there's an expectation of not running any code with a dataset? AFAIK the opposite is true: our contract is that the dataset comes with sane code to use it (as opposed to only raw data).

That's not at all clear in the api.load() documentation or examples. And perhaps this is a generational/cultural gap, but I personally would not expect any software or library, by security-competent developers, to blind-download code from a network location, without clear disclosure of that behavior, and much stronger processes around what could appear at that location.

I'm relatively sure the NLTK loading mechanism only loads non-executable data.

The expectation is not that a dataset wouldn't need custom code. It's that a mere "load" will only bring inert data, and that executed-code is only installed & run at explicit, chosen software-installation boundaries.

And, there's no need for this recklessness! There could easily be a properly-versioned gensim-datasets package. When a dataset's custom code must be revved, that package would get normal review/release procedures... and when a user wanted to use that new code, they'd just pip install gensim-datasets --upgrade. (And still, the bulk data itself would exist somewhere else to be fetched separately.)

• Releases are meant to be immutable (never changed), so version controlling them makes no sense. Dataset files are published as Github releases (as opposed to tracked inside the repo itself) due to severe file size limitation on Github.

That logic may make sense for large, non-executable data when facing certain implementation limits. It makes no sense for short executable __init__.py files, which could arbitrarily compromise a user's system. Wouldn't you agree that having often-run code, with potentially unbounded effects, completely outside normal source viewing/versioning is a bad practice?

Also, part of the general reason for a release to be "immutable" is to allow reasoning about it as a frozen artifact. If every gensim release can later load new arbitrary executable code from the network, whenever a new api.load() is run, those releases have lost an important aspect of their 'immutability' for analytical/trust purposes.

• Seeing who has gensim or gensim-data access rights is completely standard.

I can't find anything in the github interface which lists who could change the files at https://github.com/RaRe-Technologies/gensim-data/releases/download/*. I believe that info is private to the project owner(s).

I agree we could try to devise a better scheme, where editing an existing release file triggers some warning / is impossible. Just to prevent accidents, not to stop malicious actors with repo write access (that's not possible).

We could adopt the baseline standard that executable code is only delivered through explicit installs, and all executable code is version-controlled, as per my recommendation. That limits the damage a bad-actor with repo access can do. Then, damaging gensim users requires either compromising the full release process to PyPI & other repos, or tricking knowledgeable users (who consciously choose to install pre-release code) to load from a branch that's been visibly compromised with logged/attributed bad commits.

As it stands, a bad actor who even temporarily compromises the Github accounts of @menshikh-iv, @piskvorky, @chaitaliSaini, and perhaps others (whose admin privileges I can't see) can make it so that any gensim user using older gensim release, but executing a fresh api.load(), will run their new malicious code. I'm not sure any of you would even get a notification that one of the files under https://github.com/RaRe-Technologies/gensim-data/releases/download/* has changed. The bad actor wouldn't have made any noticeable commits, nor would they have had to separately compromise any PyPI release credentials, nor create any sort of noticed package release/version-increment.

I'm a bit surprised to have to make this case. If you aren't convinced by my examples, please check with others whose opinions you'd trust on what sort of dynamic code load-and-execution practices are safe or commonly expected for open-source libraries, without prominent disclosure.

[piskvorky]

If I understand you correctly, you'd like the code delivered separately (installed via pip install, followed by api.load()) as opposed to together (current, just api.load()).

As long as the releases are immutable, this difference is cosmetic / documentation. So the real question is, how can we ensure that releases stay immutable?

When a dataset's custom code must be revved

I don't know what you mean by "revved" – there's no release reversing allowed in gensim-data.

I'm not opposed to separating the code from data. But it'd not be for versioning reasons (there's no versioning in immutable releases), nor to limit "repository write access for malicious users" (completely wrong solution resolution for this attack vector). It'd be more to catch accidents and to ensure all changes stay above board.

May be worth asking Github support whether they support "immutable release files" = the simplest solution.

Another option would be to inform users to review the code associated with the dataset they're installing. The same way you'd ask them to review setup.py or the rest of the package. This may actually be both easier to manage as well as easier for the users to accomplish (less code to review, no irrelevant scaffolding).

Pull requests welcome! Unfortunately we won't have time to get to this in the near future ourselves.

[menshikh-iv]

I partially agree with @gojomo, this is really security risk (exactly by reason that "GitHub release" have no guarantees to immutability, this guarantee exist only by our agreement).

I'm +1 for:

• move "load" code to git (for better tracking & real guarantees), stay "data files" in releases
• clarify documentation of api.load (describe process "how that works" in details)

I'm not sure about "gensim-data" as python package (that will store code for loading), from one hand - good idea (consistent, fully tracked), from another hand - make a release for each new dataset is pretty annoying + user always should upgrade gensim-data first before download new data/model.

BTW, "immutable release files" on GitHub doesn't help much: you have no chance to mistake in that case (in the current scheme, this is hard to test it properly before you make a github release, we need that first).

[gojomo]

With regard to the potential addition of HuggingfaceHub-specific downloading functionality in #3206, @piskvorky writes:

Changing how the gensim-data models and corpora are uploaded / maintained is definitely possible – pending a strong contributor to actually implement it.

If I understand correctly, the options now are:

1. Rethink and reimplement the whole gensim-data API first, before considering integration with HFH.
   Pros: We can architect a better system.
   Cons: Non-trivial work; keep in mind no one has stepped up to do it for years.
2. Accept this PR.
   Pros: Useful, to at least N users, with N >= 3 :) (real user base for this HFH-Gensim integration unclear)
   Cons: Adding a bit of complexity we may eventually replace via 1) or 3).
3. Other?

I'm against integrating the PR in #3206, for the reasons outlined in a comment there.

gensim.downloader violates a bunch of good project-hygeine practices that should be rarely if ever violated, such as:

• all code is in version control attributed to a contributor
• all code sent to non-expert users goes through a formal & standard release/packaging process
• novel unversioned/unreviewed/hard-to-review code isn't fetched & executed from over the internet without prominent warning

Given that, it shouldn't have to wait for anyone to "architect a better system". It should just be discontinued ASAP - especially if there are no current maintainers eager to fix its issues.

A wiki page full of download links can replace it: "If you want to work with data X, here's a link to a bundle: x.gz. Download, expand, view & work-with the files inside. Remember that if you unpickle a data-file, or run a .py file, you'll be running new arbitrary code from the internet."

I'd even argue that explicit process is better for users' understanding of what data files are now on their local drive, and what code/classes/calls they might use to work with it.

Compare the current approach, of encouraging extremely-novice users to run code like:

import gensim.downloader as api
model = api.load("fasttext-wiki-news-subwords-300")

What do you think model is, after that code?

There's zero documentation of what kind of object this opaque magic call just delivered.

Here and here are two examples of users in just the last few days who quite reasonably thought it might be a FastText model that could offer OOV word lookup.

But it's not a FastText object. After searching and finding no docs declaring what it is, you might be tempted to try to browse the project source code to figure out what it is. But the __init__.py source code – that's downloaded & executed to create that model – isn't in any git repo that I can find. It's only in the 'release' assets download area here:

https://github.com/RaRe-Technologies/gensim-data/releases/tag/fasttext-wiki-news-subwords-300

You have to download it & view it locally to know what it's doing. (It turns out it's only using a static set of full-word output vectors from FastText, and loading them as a plain KeyedVectors - so it's somewhat of a misnomer to even call this a 'FastText' model at all, especially one which claims to be leveraging 'subwords'.)

And while the release is attributed to @menshikh-iv, I believe any one of the (nowhere-publicly-displayed) maintainers of this project can replace any of those 'asset' files via a Github upload interface at any time. That is, the normal processes which create confidence in the code that's shipped as part of the core Gensim repo, or officially packaged releases – attributed commit logs, test cycles, formal releases – are not active for this source code file.

(Even the gensim.downloader code's token effort to checksum-verify the bulk data is kind of a joke. It's using MD5 - a broken algorithm that cryptographers have been warning about, due to suspected flaws, since all the way back in 1994. Researchers published engineered hash-collisions for it in 2010. It was already obsolete for this purpose when this code was written in 2017.)

And what if someone wanted to improve this confusing user experience with the fasttext-wiki-news-subwords-300 bundle? There's no code repo against which to propose changes to that __init__.py, to use a more appropriate dataset/model-class, or to add extra docs, or to point people to a better example of a true FastText model with OOV functionality.

So this idiosyncratic delivery-method is also resistant to the normal mechanisms by which an open-source project can improve over time.

[piskvorky]

It should just be discontinued ASAP - especially if there are no current maintainers eager to fix its issues.

That's the option 3) I guess.

That option (with a replacement) I also mentioned in the part of the comment you didn't quote here:

Or possibly outsource the whole "models/corpora management" problem altogether, if it turns out something sufficient already exists.

To be clear, I'm also not a fan of the current design of gensim-data. It should go the way of the infamous #1777 (same ppl).
But I do think removing the functionality without a replacement is inferior to keeping it: axe < keep < replace < re-architect.

[gojomo]

Indeed, we should 'replace' if possible - but an easy way to do that is to set up a replacement web page, with all the same info as in the gensim-data list.json, including direct-download links for the datasets.

The key value of the collection – reliable download URLs for known useful datasets – will remain, needing just a few extra clicks from users to use.

And those extra explicit steps would be a positive improvement over the opaque magic of api.load(), that hides which files arrive, & what Gensim code/classes are being used to work on what data formats. A user of a dataset from the cloud should know which filepath it's at, and which line-of-code is needed to load it, and what Gensim class it gets loaded as. Hiding those details is a disservice to users, especially the beginners enticed by those singular api.load() lines.