Uncaught `error` event from busboy can crash http servers

[max-mathieu]

There is a case of malformed requests that can take down a full nodejs app, due to an uncaught error event thrown by busboy

Using the latest 1.4.5-lts.1 from npm, the following code results in a full crash of the app (tested on both node 14 and node 18)

const express = require('express')
const multer  = require('multer')
const http  = require('http')
const upload = multer({ dest: 'uploads/' })
const port = 8888

const app = express()

app.post('/upload', upload.single('file'), function (req, res) {
  res.send({})
})

app.listen(port, () => {
  console.log(`Listening on port ${port}`)

  const boundary = 'AaB03x'
  const body = [
    '--' + boundary,
    'Content-Disposition: form-data; name="file"; filename="test.txt"',
    'Content-Type: text/plain',
    '',
    'test without end boundary'
  ].join('\r\n')
  const options = {
    hostname: 'localhost',
    port,
    path: '/upload',
    method: 'POST',
    headers: {
      'content-type': 'multipart/form-data; boundary=' + boundary,
      'content-length': body.length,
    }
  }
  const req = http.request(options, (res) => {
    console.log(res.statusCode)
  })
  req.on('error', (err) => {
    console.error(err)
  })
  req.write(body)
  req.end()
})

Result:

# node multer-bug.js
Listening on port 8888
events.js:377
      throw er; // Unhandled 'error' event
      ^

Error: Unexpected end of form
    at Multipart._final (/Users/max/src/tests/node_modules/busboy/lib/types/multipart.js:588:17)
    at callFinal (internal/streams/writable.js:610:10)
    at processTicksAndRejections (internal/process/task_queues.js:82:21)
Emitted 'error' event on Multipart instance at:
    at emitErrorNT (internal/streams/destroy.js:106:8)
    at emitErrorCloseNT (internal/streams/destroy.js:74:3)
    at processTicksAndRejections (internal/process/task_queues.js:82:21) {
  storageErrors: []
}
# back to prompt

The crash doesn't happen as soon as process.on('uncaughtException') is set, which probably explains why this test passes https://github.com/expressjs/multer/blob/lts/test/error-handling.js#L226-L250 with the same request.

This is not happening with multer 1.4.3 (and busboy pre-1.0)

I tracked this down to the call to busboy.removeAllListeners in https://github.com/expressjs/multer/blob/lts/lib/make-middleware.js#L40-L46 happening before busboy emits another error event (which I think comes from the _destroy call).

Since I feel like this removeAllListeners is mostly for cleanup/mem-leak prevention, I tried wrapping the removeAllListeners call with process.nextTick, and it does eliminate the issue.

I'm opening a PR with this fix, but I am unable to adjust the tests since they don't fail with the exact same payload

[max-mathieu]

For context, we had a client calling the API with malformed requests (I guess crafting a multipart/form-data request is hard in some languages...) which resulting in our API crashing and restarting, though interrupting other users' ongoing uploads

As such, this is a potential vector for a bad actor to take down apps with relatively minimal effort.

Only protection for now seems to have a process.on('uncaughtException') that filters out this specific exception (since it's best practice to not prevent the crash in the uncaughtException listener).

[eamon0989]

We are facing the same issue. We use NestJs and while file uploads work in general, malformed requests crash the server. We discovered the issue recently during pen-testing. I tried adding:

      process.nextTick(() => {
        busboy.removeAllListeners()
      })

to node_modules/multer/lib/make-middleware.js as you did in your PR and I can confirm that it fixes the issue.