Hacker News Reader

Comment by ievans | Hacker News Reader

Our approach to solving this on our open source project:

1. Enforce use of lock files 2. On any PR that changes any hash in the lock file, run a static analysis that looks for “permission” changes in any source file. E.g., used network, used env vars, used filesystem 3. Comment on the PR with any observed permission changes

It is very alpha-quality and not yet generally available but here are some details: https://deps.semgrep.dev/

View on HN

Does that tool resolve things like:

   setTimeout("dan" + "gerous" + "func" + "tion" + "()", 1);

I'm only somewhat superficially familiar with JavaScript (and only in the browser, not NodeJS), but you can probably get far more obfuscated than that; or at least, you can in some other similar-ish languages. For example in Ruby on Rails they use (or used? It's been years so may be different now) all sorts of meta-programming stuff, which at times actually made it somewhat difficult to find a method definition if you didn't know where to look; it was all dynamically defined at runtime.

My worry with such a tool would be that malicious actors would go out of their way to hide their code from such tools, and that the tool would give me a false sense of security.

Yeah, static analysis will never be enough, you'll need to parse the code and evaluate it in a sandbox to find evil functions.

Another more obfuscated example:

    function hello() {
      console.log("I'm stealing all your passwords now");
    };hello();

Can be turned into:

    (function(_0x3c7f26,_0x1837dd){var _0x8bd36e=_0x5929,_0x90b69b=_0x3c7f26();while(!![]){try{var _0x249481=parseInt(_0x8bd36e(0x1cc))/0x1*(parseInt(_0x8bd36e(0x1d0))/0x2)+-parseInt(_0x8bd36e(0x1ce))/0x3+parseInt(_0x8bd36e(0x1d5))/0x4+-parseInt(_0x8bd36e(0x1d4))/0x5*(parseInt(_0x8bd36e(0x1d2))/0x6)+-parseInt(_0x8bd36e(0x1d1))/0x7+-parseInt(_0x8bd36e(0x1cd))/0x8+-parseInt(_0x8bd36e(0x1cf))/0x9*(-parseInt(_0x8bd36e(0x1cb))/0xa);if(_0x249481===_0x1837dd)break;else _0x90b69b['push'](_0x90b69b['shift']());}catch(_0x19e610){_0x90b69b['push'](_0x90b69b['shift']());}}}(_0x2849,0xf22bd));function hello(){var _0x2f0a61=_0x5929;console[_0x2f0a61(0x1d3)]('I\x27m\x20stealing\x20all\x20your\x20passwords\x20now');}function _0x5929(_0x2fab05,_0x1e12e){var _0x284982=_0x2849();return _0x5929=function(_0x5929e4,_0x24e341){_0x5929e4=_0x5929e4-0x1cb;var _0x1372b0=_0x284982[_0x5929e4];return _0x1372b0;},_0x5929(_0x2fab05,_0x1e12e);};hello();function _0x2849(){var _0x114094=['8091OneRlW','38iQspVa','2224236lvStpa','54nJkbsM','log','908785hkDdiw','7617548ADhgFx','32610QhPrTC','44539hBNEpr','7460904ZMwHlf','5412480lnulMs'];_0x2849=function(){return _0x114094;};return _0x2849();}

No way for a static analysis tool to catch that.

Tumblr themes used to have a similar static analysis and this was the exact way to get around it (well, `atob()` in that specific case)

Our analysis actually covers this case (and the obfuscation case below) just fine: we don’t care about being precise, just very conservative. So any use of setTimeout where the first argument is not a function is flagged as a potential dynamic execution. There are a couple other security teams at other companies who are collaborators on the project with us who have contributed Semgrep rules that cover what should be all the dynamic injection points in JS (eval, vm.*, child_process). In fact many obfuscation techniques will show up as a permission change as they involve the addition of dynamic behavior. Which is a great signal to have—why is my dependency publishing obfuscated code?

And making it does highlight “where should we look” in lockfile diffs, which makes us more comfortable updating frequently.

Also, the strongest signal comes from 0 -> 1+ permission transitions. Like if leftpad suddenly adds an exec or setTimeout call.

Would love to hear more critique of the approach though!

> Would love to hear more critique of the approach though!

It wasn't really a "critique" as such, and more of a question. It's not hard to imagine being able to fool a naïve tool – flagging any dynamic code execution seems like the right approach.

An unfortunate side-effect of this type of system, in my experience, is that you end up being stuck on very old dependencies for a very long time.

When a security issue is discovered, perhaps months or years later, it is extremely difficult to upgrade because

1) the patch will always be applied to the most current release, which could be many major versions beyond what you run, and

2) manually backporting the security fix is impossible without creating your own fork and the code will likely be so different as to not be able to cleanly apply the patch.

You are creating so much friction to update dependencies that you will find yourself unable to update them in the future.

Small example: I recently upgraded a dependency that required us to move from node 12 to node 14. That required us to upgrade more packages, including our database ORM. The ORM was 2 years out of date. Now we have changes that literally affect the entire application and require significant regression testing (automated and manual) because the risk surface is so much larger. This wouldn't have happened if we didn't use a lock file with pinned versions.

------

In the past (node 4.x), lock files weren't written by default, and I (on a small project) had sufficient test coverage that always allowed NPM to install the latest minor version bump of every package, every time. If there was a problem caught by the tests, then I immediately fixed the issue with the upgrade.

The nice thing about this is that I was always on the latest version of every dependency, so I always had the latest security fixes, and the online docs for the dependencies were always accurate to what I was using.

If this particular exploit happened, it would have been more likely to get deployed but it would have also automatically gotten fixed as soon as the patch update rolled around. Whereas with a lock file, someone may have updated it and then not been aware when the "fixed" version came out because npm/yarn don't tell you that.

As a JavaScript dev (full stack) I think lock files are a bad idea by making us LESS secure, not more (keeping around insecure older dependencies that can't be patched).

> If this particular exploit happened, it would have been more likely to get deployed but it would have also automatically gotten fixed as soon as the patch update rolled around.

Note that updating your deps to a non-hacked version would not "fix" it, though. The machine is persistently compromised until reimaged if it was given the chance to install the new package.

Lock files making you less secure is untrue. Using lock files does not mean your dependencies are not being updated, it means you have control over _when_ they are updated. Which means 1. you won't be stuck on a dependency update breaking your code while you are working on an unrelated feature, because you can work on adapting your code to the update independently, 2. you can check whether lock file update breaks the tests separately from testing your own changes, 3. with vcs you can retroactively investigate which dependency update caused a particular breakage or behavior change.

> Now we have changes that literally affect the entire application and require significant regression testing (automated and manual) because the risk surface is so much larger. This wouldn't have happened if we didn't use a lock file with pinned versions.

But without a lock file you would always be using the latest version, right?

Wouldn't those two years of constant possible breaking updates (would you know if something updates to the latest version? How do you manage that all versions are the same across all machines?) require a lot more regression testing over those entire two years and introduce a lot more risk of breakage?

No, because the changes would have been many small incremental changes, each of which is easier to validate on it's own. It's the sun of those dozens of update all at once that creates the problem.

Pre-lockfiles I had multiple instances of a new version of a dependency being released that didn't follow semver correctly (it can be hard to identify what constitutes a breaking change at times).

Without a lockfile this was a pain to fix, now you can just not merge the upgrade until the problem is resolved (rather than every branch suddenly failing)

Reply on news.ycombinator.com