
Npm security post-mortem - IsaacSchlueter
http://blog.npmjs.org/post/80277229932/newly-paranoid-maintainers
======
cyphunk
> * Before they could start, we had a very serious security vulnerability
> responsibly disclosed by Will Farrington and Charlie Somerville

> * We fixed it on February 17th

the fix scares the shit out of me:

[https://github.com/isaacs/st/commit/5a0c1886737a20d78ae00b61...](https://github.com/isaacs/st/commit/5a0c1886737a20d78ae00b61e4724ae3095f4ddd)

    
    
         Properly escape all relevant html entities
         Avoid problems with files named things like '<img>' and so on.
         
         -      var name = f.replace(/"/g, '&quot;')
         +      var name = f
         +          .replace(/"/g, '&quot;')
         +          .replace(/>/g, '&lt;')
         +          .replace(/</g, '&gt;')
         +          .replace(/'/g, '&#39;')

~~~
matt_kantor
It looks like this change has more to do with XSS than the "big" exploit.

The more serious fix occurred here:
[https://github.com/isaacs/st/commit/6b54ce2d2fb912eadd31e2c2...](https://github.com/isaacs/st/commit/6b54ce2d2fb912eadd31e2c25c65456d2c8666e1)

And here:
[https://github.com/isaacs/st/commit/6d6100eec8b19e2774a6f2bb...](https://github.com/isaacs/st/commit/6d6100eec8b19e2774a6f2bb5c9b54fa9e1f9e72#diff-02be450bd9337a4b4e26c2139550120bR160)

With some icing on the cake here:
[https://github.com/isaacs/st/commit/8b2f212f64b762e351f311f4...](https://github.com/isaacs/st/commit/8b2f212f64b762e351f311f4bbfcb291aa997838)

~~~
cyphunk
not much more encouraging. it looks to me like patch work. ive had this in the
past. would give a PoC to a client along with a recommended design change to
the questionable methods of the code. they would send back a new version with
a patch much like all of those linked here. in the end those patches address
the PoC but not the problem. then i just rework the PoC to go around the
patch. This cat-mouse game goes on until they go back, do the f'ing work, and
implement the original design change suggested. I say all that just to point
out that this looks like patch work and is a scary behaviour. Then again,
maybe this is the nature of nodejs (omg).

Also, as a general rule:

    
    
        ANY SECURITY PATCH THAT IS A REGEX IS NOT A SECURITY PATCH

~~~
hueving
>ANY SECURITY PATCH THAT IS A REGEX IS NOT A SECURITY PATCH

Not true. Regex works well for forcing integers.

------
wycats
It's worth noting that the XSS vulnerability ("A user could inject scripts
into the npm website via the README and license fields") assuredly exposed a
whole slew of easy-to-exploit vulnerabilities, and the community should feel
very lucky that such an obvious vulnerability was in the wild for so long
without being exploited.

TL;DR always use a templating engine that makes you think about XSS and don't
allow unsanitized user-provided HTML through raw.

~~~
Pxtl
> don't allow unsanitized user-provided HTML through raw.

It's sad that you even need to say this, both for the fact that Javascript
sandboxing is so terrible and the fact that developers aren't aware of the
hazards of just blindly taking user-provided HTML.

~~~
seldo
As a point of honor, I feel obligated to mention that @rockbot and I are
_very_ aware of these hazards, but our website was put together in somebody's
spare time when Node had like 100 users in total :-)

~~~
zaroth
I read that 'as a point of horror'.

------
ilaksh
Why do they have to apologize for that? Almost every piece of software has
security vulnerabilities.

Do people now really believe that there are definitely no security
vulnerabilities related to the npm registry? Or any other type of registry, or
website or application for that matter?

People have completely unrealistic expectations about security. Every time you
had a significant amount of new functionality, or even a very insignificant
amount, you could have introduced a new security vulnerability.

Basically this other company Lift or whatever could have two full time
engineers doing security audits for npm from here until npm is done, and you
could still have some other hacker who was thinking differently come up with a
vulnerability in some new feature that they missed.

Its great to have the attitude that you are going to make a serious effort,
but totally unrealistic to think that you are going to do one security audit
and then there will be no more vulnerabilities. And also really makes no sense
to make a big deal about it or give them grief or for them to even feel bad.
If you think that way then you don't understand security.

You see all of these large projects having security issues not because they
are full of negligent or sloppy engineers, but because security takes a lot of
resources and is very difficult. The security firms will certainly suggest
that engineers are negligent, of course. That ensures that they will continue
to get new clients. But the reality is even with a lot of resources dedicated
to just security, projects can easily have new vulnerabilities.

So that's great that they are getting regular security audits now. They are
ahead of the curve.

EDIT: I notice I have been buried without anyone bothering to even respond. If
you disagree, say why you disagree.

~~~
roel_v
"Why do they have to apologize for that?"

It's just the sad state of the 'industry'. As soon as some armchair warrior
finds something remotely wrong with whatever, they'll go _nuts_ on you and you
need this sort of touchy-feely PR nonsense to placate the comic book shop
types. Hats off to these guys for being level-headed enough to be able to play
the game this way - I couldn't do it any more, I'd go bonkers over overhead
like this.

~~~
Pacabel
If anyone needs placating, it's those developers, managers and executives who
pushed hard for the use of Node.js in business settings, not expecting a
serious security incident like this one to happen.

For those who are especially serious about their careers, reputations, budgets
and power, incidents like this involving the technologies they hyped and
pushed through can be disastrous. Now they're seen as being very wrong about
something very important, and this in turn makes them extremely angry.

They know that their competitors within the business will use this incident
against them. Their next initiative will surely face comment like, "Why should
we listen to you after the npm disaster?", and they will face a much harder
battle if their decision involves any controversy or doubt at all.

~~~
ilaksh
Every technology is going to have an "incident" like this. Experienced
developers expect security "incidents". There are quite a few managers and
executives who do not understand security and don't. And many people who will
use this sort of "incident" politically. Politics is the problem there, not
the technology.

Just because they found one significant security issue with npm does not mean
they were "very wrong" about using Node. Its just a reality of security with
any technology.

This isn't a disaster, this is a demonstration of maturity, responsibility and
transparency.

------
jeremymcanally
Nice write-up and good on them for fixing that quickly, but it's a serious
bummer they unnecessarily bring in the RubyGems incident as some sort of
awkward "Well at least we didn't screw up that badly!" swipe. It's not
relevant to anything else they said.

~~~
wycats
I think what they're trying to say is that the bug is exactly as bad as the
Rubygems incident, but they got lucky and it wasn't exploited.

~~~
Jgrubb
How I took it as well.

~~~
seldo
That was certainly the intention.

------
akadien
The world would be a little better if every software company could inform its
users of security vulnerabilities and bugs as these guys did.

~~~
Pacabel
They can, and many do or have done so in the past.

Let's not pretend that security incident post-mortems are anything new; they
aren't. There's not really even anything special about this one.

I don't really see much point in applauding these guys for doing what's
perhaps the minimum we should expect from them in this situation.

------
abecedarius
The 'we fixed it' link points to four new lines including these:

    
    
          .replace(/>/g, '&lt;')
          .replace(/</g, '&gt;')
    

I can't really tell without more background and context, but I'm surprised
this doesn't turn > into &gt; and < into &lt;. Is this a mistake? The same
code's still in the HEAD.

~~~
dav-
Yeah, that is very weird. Shouldn't it this?

    
    
        .replace(/>/g, '&gt;')
        .replace(/</g, '&lt;')

~~~
dav-
Actually, someone submitted a pull request to fix this 4 hours before my
original comment:
[https://github.com/isaacs/st/pull/37/files](https://github.com/isaacs/st/pull/37/files)

------
outside1234
Its great to finally have full time folks managing npm and to finally have the
resources to do things right (eg. hire ^lift).

Thanks for doing!

------
cjbprime
Nice disclosure, and can't fault the npmjs team given that they commissioned a
security audit as soon as they possibly could.

I wonder if the npm, Inc. team told ^Lift about the disclosed vulns before
^Lift's own audit completed. I can imagine being tempted to see whether they'd
discover it themselves, to gain more evidence on how comprehensive the audit
was.

~~~
seldo
We told them as soon as we found out, because we needed them to go looking for
the same hole in all of our code bases :-)

As part of the audit, ^Lift audited a lot of the third-party modules we use,
and notified the authors of those packages separately (I'm not aware of the
details, but I don't think there was anything major).

------
maxjus
Y'all might want to limit the number of characters in a user's name...

[https://www.npmjs.org/~maxj](https://www.npmjs.org/~maxj)

------
giovannibajo1
can anybody disclose some figures on how much ^lift (or competitors) costs,
e.g.: for a 100K-line Python codebase? A rough ballpark would help a lot.

~~~
patio11
Thomas could give you better numbers here, but in general, appsec reviews cost
as much as getting software development done. (i.e. For a security review
worth the paper you print the report on, you're looking at $5k at the lower
end if your application is simple or if someone wants to really do you a
favor, and they get _substantially_ more expensive than that. You can get
someone to run an automated scan for $500 and give you a CSV file, but that is
not maximally in your interests.)

I'd turn away any client who asked me to do appsec work, because I don't think
I'd produce work of a sufficient quality to justify the sort of rates I
charge, but I do think I'm probably good enough to roughly scope appsec
projects on technologies I understand well. Example: Appointment Reminder is
an architecturally simple Rails application. I think an audit of the marketing
site, application, and architecture would reasonably require probably 1 to 2
billable weeks depending on how much I asked you to plumb e.g. line-by-line
HIPAA requirements, and that would probably run in the $4k to $12k region
based on my understanding of prevailing rates for appsec work. (I'm sure that
if I had $25k budgeted for that audit many firms could find a way to get me my
money's worth for every penny of that budget, by the way.)

~~~
tptacek
A $4k billable week is incredibly cheap, so much so that I'd worry about the
team delivering it. Our rates are high because big firms bid them up. Why are
the cheap teams turning down free money?

If someone offers you a $4k week, make sure they know they're cutting you a
deal.

~~~
homakov
> Why are the cheap teams turning down free money?

Hi, i double that. Also I understood your advice better now. Undercharging =
making it worse for everyone (consultant profit, work quality, dumping).

~~~
tptacek
Hey, just to be clear: I don't think it's _wrong_ to charge less than the
market rate, and while I'm more likely to do a project _gratis_ than at 1/2 or
1/3 my rate, we've given people breaks before.

------
jmspring
So the audit was mostly about nom the website and the service for maintaining
npm packages? That's a good first step.

Has there been any talk within the node community about auditing node modules
themselves? Maybe start with the most popular? I could see this being popular
with enterprise development, etc.

I want to say that Strong Loop made noise about doing something like this, but
I haven't seen much on it of late.

~~~
daviddias
One of the Node Security Project
([https://nodesecurity.io/](https://nodesecurity.io/)) main efforts is to
audit all the npm modules in a community driven way.

We are accepting contributions from the community to build the tools that get
the job done efficiently and to audit modules, disclosing vulnerabilities in a
responsible manner.

~~~
jmspring
I'll take a look, thanks. Some reviews will, of course, be manual in nature --
implementation correctness of digest auth, for instance, is one that comes to
mind (I need to contribute that back to a particular module).

------
dantiberian
Is this why npm made the changes with self signed certificates
[http://blog.npmjs.org/post/78085451721/npms-self-signed-
cert...](http://blog.npmjs.org/post/78085451721/npms-self-signed-certificate-
is-no-more) or is that unrelated?

~~~
seldo
The original abandoning of the self-signed cert was because self-signed certs
were a bad idea.

The issue that post refers to (it is a little unclear, because we ourselves
were a little unclear what had gone wrong at that point) is when we broke
older clients by moving to a non-GlobalSign cert. We cleared that up here:
[http://blog.npmjs.org/post/78165272245/more-help-with-
self-s...](http://blog.npmjs.org/post/78165272245/more-help-with-self-signed-
cert-in-chain-and-npm)

We had already planned to move to a new cert before the security disclosure,
and hadn't anticipated the size of the problem with a non-GlobalSign cert, so
although the two happened pretty much simultaneously, they weren't really
related.

------
filipedeschamps
One of the things I love about Isaac is his empathy. Reading his blog posts,
listening to his podcasts on Nodeup, looks like he is writing/talking to me,
and since I'm a developer, this makes a huge difference in my motivation.

Great job guys and very responsible choices.

------
qubyte
Security postmortems are a great idea. Until time machines happen.

------
seaghost
Great read!

------
Fasebook
Nearly Paid Muppets?

