Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed a bug which could cause a crash in HeaderParser, and as consequence could potentially crash a web server based on it #22

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

RolandHeinze
Copy link

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 _realFinish and all of it's occurances in Dicer.js as it is not necessary, increases the code size, and adds unwanted complexity;
  • removes an unnecessary else ifclause in the function Dicer.prototype._oninfo();

@RolandHeinze RolandHeinze changed the title Removed a bug which could cause a crash in HeaderParser, and as consequence could potentially crash a web server based on Removed a bug which could cause a crash in HeaderParser, and as consequence could potentially crash a web server based on it Aug 5, 2021
HeaderParser.prototype._parseHeader
constructor function HeaderParser.
In the case of start > 0, end would be wrong.
constructor function Dicer and all its occurances,
as it is not necessary, increases the code size,
and adds unwanted complexity.
The else if clause was exactly the opposite of the if clause.
@kibertoad
Copy link

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

maxpoulin64 added a commit to maxpoulin64/thelounge that referenced this pull request Dec 5, 2021
I've been notified the current implementation is abandonned and has been forked by fastify to fix bugs, including some crashes and hangs:
See:
* mscdex/busboy#250
* mscdex/dicer#22
* mscdex/dicer#25
@nathan-gilbert
Copy link

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?

@mscdex
Copy link
Owner

mscdex commented May 20, 2022

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

@nathan-gilbert
Copy link

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

@mscdex
Copy link
Owner

mscdex commented May 20, 2022

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

@nathan-gilbert
Copy link

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

@nsandeepn
Copy link

There is a Denial of Service (DoS) security flaw has been introduced in multer@1.4.4. 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.

@mscdex
Copy link
Owner

mscdex commented May 23, 2022

@nsandeepn multer just needs to upgrade to the latest version of busboy. For existing multer installations, there's nothing that can be done (see #22 (comment)).

@Karlinator
Copy link

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.

@AnkurBansalSF
Copy link

AnkurBansalSF commented May 26, 2022

@Karlinator
npm audit report

dicer *
Severity: high
Crash in HeaderParser in dicer - GHSA-wm7h-9275-46v2
fix available via npm audit fix --force
Will install firebase-admin@7.0.0, 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

@yurisim
Copy link

yurisim commented May 26, 2022

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
expressjs/multer#1095

@rudxde
Copy link

rudxde commented May 27, 2022

multer just needs to upgrade to the latest version of busboy. For existing multer installations, there's nothing that can be done (see #22 (comment)).

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: expressjs/multer#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.

@colinhowe
Copy link

colinhowe commented May 27, 2022

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.

@lahirumaramba
Copy link

lahirumaramba commented Jun 1, 2022

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 firebase/firebase-admin-node#1718 (comment)). 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.

@Kondamon
Copy link

Kondamon commented Jun 3, 2022

dicer was originally created for use by busboy, 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?

@mscdex
Copy link
Owner

mscdex commented Jun 3, 2022

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

@hardysabs2
Copy link

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?

@rafaelmaeuer
Copy link

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?

@colinhowe
Copy link

@rafaelmaeuer 0.2.5 - I'll update my comment to say that

@vinutha93bnvs
Copy link

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?

sgammon added a commit to sgammon/axios-fetch that referenced this pull request Dec 19, 2022
Severity:
High

References:
CVE-2022-24434
SNYK-JS-DICER-2311764
mscdex/busboy#250
mscdex/dicer#22

Notes:
Only used during test anyway.
sgammon added a commit to sgammon/axios-fetch that referenced this pull request Dec 19, 2022
Severity:
High

References:
CVE-2022-24434
SNYK-JS-DICER-2311764
mscdex/busboy#250
mscdex/dicer#22

Notes:
Only used during test anyway.
sgammon added a commit to sgammon/axios-fetch that referenced this pull request Dec 19, 2022
Severity:
High

References:
CVE-2022-24434
SNYK-JS-DICER-2311764
mscdex/busboy#250
mscdex/dicer#22

Notes:
Only used during test anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.