4 ms·

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 analysi

by ievans 5y ago

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/ https://deps.semgrep.dev/

Osiris 5y ago
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).
- ev1 5y ago
  > 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.
- zhenyavinogrdov 5y ago
  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.
- arp242 5y ago
  > 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?
  - Osiris 5y ago
    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.
- mnahkies 5y ago
  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)
arp242 5y ago
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.
- capableweb 5y ago
  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.
  - cobertos 5y ago
    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)

by ievans 5y ago

Osiris 5y ago

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).

ev1 5y ago
> 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.
zhenyavinogrdov 5y ago
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.
arp242 5y ago
> 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?
- Osiris 5y ago
  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.
mnahkies 5y ago
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)

arp242 5y ago

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.

capableweb 5y ago
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.
- cobertos 5y ago
  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)