Use of md5 is not FIPS compliant. Express server requests using etag fail with FIPS compiled NodeJS #17
Comments
|
Hi @agenaille, this is certainly something that sucks :/ So just in general, before the rest of this information (as the TL;DR) I would say: don't use this module then. Sure, you are using Express.js, but that only uses this module if you don't turn ETags off or provide your own custom implementation. This also means that your quickest path to resolution is to provide your own custom ETag generation function to Express.js to skip using this module for your requirements. Now, for the long answer. Yes, it sucks that you have to use something that stops you from using a certain hash algorithm, even if the use of the algorithm is actually usable in the domain it is used for here (and we even harden it by providing the original content length as out-of-band information in the ETag). The main reason we use md5 is because it's very fast and provides the needs we have w.r.t the ETags. I think providing a way to choose the underlying algorithm is out-of-scope for this module, as this module's purpose is to simply provide a very simply way to generate the ETags in a reproducible way and by the time we start adding the ability to choose the algorithm, we are not only exposing implementation details across our interface, but at that point this module is really not doing anything. I'm not going to close this issue, because I think this requires some investigation on our end--to figure out how to deal with FIPS enabled OpenSSL in Node.js, a bigger conversation I need to understand from Node.js core on what modules are expected to do. In addition, if we do change the algorithm, it will have to be a major version bump, including not getting into Express.js until it does a major version bump. This is because people have brought up the concern that they want to understand exactly how we generate our ETags so they could duplicate them on things like caching servers and mirrors, so changing the algorithm in Express 4.x has sailed, unfortunately (but you can easily set your own custom ETag generator in Express.js to use sha256 if you want). We also need to understand the performance implications of changing the algorithm (you can see we provide benchmarks on the README, as we do really check how fast the ETag generation is, because it makes a huge difference in Node.js since we are generating them in a sync fashion, which blocks the event loop from proceeding forward until the generation has completed. This was one of the reasons we choose the MD5 algorithm with the out-of-band original byte size. I hope this makes sense! |
|
Thank you for your reply @dougwilson. We will look into plugging in a custom etag generation module. My hope is that in the future, you would be ok with providing the customization of the algorithm per environment. Teams needing to use FIPS would be able to use your nice etag module, and teams not using it would be 100% unaffected and still having the same md5 performance that exists today. Thanks again, Alex |
|
Seems like your shortest path @agenaille would be to fork this repo and then change this line to The tests included won't pass since it has the hashes hardcoded in them (which is correct, IMO), but it should work similarly with the exception being that it would use |
|
Thanks @DavidTPate. I understand that part, and plan to fork as you said, but the harder and more time consuming part of this will be figuring out how to add a custom etag generation module into my express app so that all of my REST routes are etagged, as well as static file responses. :/ |
|
Removed my comment based on @dougwilson's feedback here please see this for a proper implementation. |
|
$$$$$$$$$$$$ money! Thank you @DavidTPate you're the man! :) |
|
Hi @DavidTPate, your example is not correct, please never, ever set those "* fn" settings, as that can cause you to break in any upgrade, as those are private. Please read the documentation on the |
Hopefully this is not hard to time-consuming in any way. If you are finding gaps in the Express.js documentation, please file them at https://github.com/expressjs/expressjs.com so we can get this documented! Here are the answers for your two cases: // This controls the ETag generation or your dynamic responses through res.send/res.json/res.jsonp, etc.
app.set('etag', function dynamicETag (body) {
return crypto.createHash('sha256').update(body).digest('hex')
})
// This is for your static files served through serve.static ; consult documentation if you are using something else (and I hope you are NOT serving production files through Node.js....)
app.use(express.static('/public', {
setHeaders: function (res, path, stat) {
return crypto.createHash('sha256').update(stat.mtime.getTime().toString(16) + '-' + stat.size.toString(16)).digest('hex')
}
})) |
Absolutely. I have not closed this issue and still investigating it. I have not had anytime since I last posted and now, so cannot provide you any updates, which is why you have not heard further. This module will never provide a configurable algorithm, because that is just a violation of the abstraction here. You should in now way care that we are using a hash under the hood, and if you are, it is only because you are trying to copy the ETag generation in other servers (like nginx or similar), though usually that is done the other way around. I know I have not sat down to write out the goals of this module, which I also need to do, but a core goal of this module is that there is no configuration and you just get an ETag for the given content; as soon as you start adding options, this turns into something you could have easily implemented yourself, since it just doesn't do much. I do provide a lot of modules, but the core tenant is that these are not kitchen sink modules, and they do not provide knobs to turn. The expectation is that users who are using this modules understand JavaScript and can easily write it, so a module like this that does not do much is going to continue to do that, as that is the goal of the module, and if you need a different ETag generation scheme, then just write the 1-2 lines of code to do that. That all being said, the reason this is under investigate is do better understand the FIPS mode and understand what Node.js core TC expects from module developers when this is enabled. From my initial understanding of the FIPS 140-2, is that the usage of MD5 is not banned outright, only that is is not allowed to be used under certain conditions, and as far as I understand, the usage of MD5 in this module does not violate FIPS 140-2, as it is set out to cover the algorithms to use in security purposes, not in the general purpose. Yes, I see that OpenSSL (or Node.js?) is disabling all the non-verificated modules in the code, assuming that all uses of the OpenSSL library are going to be for security purposes, but that is too board, and has of course even lead Microsoft to tell people to stop enabling FIPS-mode in Windows (from my understanding; see https://blogs.technet.microsoft.com/secguide/2014/04/07/why-were-not-recommending-fips-mode-anymore/). I still need to understand from Node.js TC what they expect from module developers in this regard (i.e. are they expecting that modules on npm support a crippled code crypto module). |
|
@dougwilson Thanks for pointing that out, I replace my comment with just a note pointing to yours. Don't want anyone getting brazen based on that and messing themselves up. |
|
@dougwilson Thanks for clarifying David's code suggestion. I really appreciate both of you gentlemen's support with this! :) Doug, I was hoping I could pick your brain on one of your comments.
We do plan to serve our UI through Node.js, as we can use Node to provide filtering of requests for static assets to require an authenticated session, backed by redis, to access them. This is a business application, and we cannot have unauthenticated users downloading all of our static content, as they could learn a lot about our APIs that way. I'm wondering if you can clarify what you mean when you say you hope we aren't going to serve our static content through Node.js. I'm wondering if you thought we just had plain vanilla static assets with no authentication requirements, in which case one might just serve with Nginx, or if you think Node has some inherent problems with being production ready. We are using Node.js v6.2 because the next LTS will release on v6.x and I feel like it is ready for production, but if it is not, i'd love to hear what you have to say on that matter. :) Thanks again, Alex. |
|
The best way to override this etag module with my own ended up being:
Anyone else needing FIPS compliant etag can simply add the above to your package.json in your Express project and npm will automatically flatten the dependency out and overwrite the jshttp/etag module with the sha1 equipped module, in the node_modules folder with no code changes necessary. Thanks everyone for your help and suggestions! |
We use Express v4 and our client requires any crypto providers be FIPS compliant. So, we build NodeJS v6.2 with the openssl fips module.
When we make requests to the server, they fail with the following error:
In our effort to use Node with FIPS compliant crypto, we have noticed several modules using md5, which is officially deemed insecure. Now, I realize etag is only using the hash to get a unique identifier for the response content, but the openssl-fips module has no way to know if a module is encrypting some sensitive content, or if it is just hashing a response. :/
We have proven a fix for this locally by simply changing md5 to sha256 on line 48 of the index.js. We think sha256 would be future proof, as sha1 is going to be insecure pretty soon, most likely. This fix would greatly help us in our efforts to prepare for a security penetration test. :)
We realize that use of sha will degrade performance, so what would you think of adding something like this:
A private var, which allows us to customize the hash algorithm.
Use the customized, or defaulted value
I've opened a PR for this: #18
Thank you very much for any support you can provide on this,
Alex
The text was updated successfully, but these errors were encountered: