bug: unhandled error from a malformed request can crash the server - Unexpected end of form

eamon0989 commented 1 year ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current behavior

During pen-testing we came across what appears to be a major vulnerability that allows a malicious actor to crash a nestjs/express server using a malformed request. This is not a new issue, there are several issue referencing it, but most have been closed as it was not easy to reproduce the issue. I have made a minimal reproduction below that with two scripts that show how easy it is to reliably crash the server, one using netcat and the other sending a request using node http.

I believe that the following issues are related: https://github.com/nestjs/nest/issues/9489 https://github.com/nestjs/nest/issues/10264

https://github.com/expressjs/multer/issues/1176

Minimum reproduction code

https://github.com/eamon0989/minimal-crash-reproduction-nestjs

Steps to reproduce

Run yarn to install dependencies.
Run yarn run start to start the server.
Run cat crash-file-request.raw | nc localhost 3000 or node malformed-request.mjs from the terminal which will cause the app to crash.

Expected behavior

I would expect the app to handle the bad request and return a 400 response.

Package

[ ] I don't know. Or some 3rd-party package
[ ] code>@nestjs/common</code
[ ] code>@nestjs/core</code
[ ] code>@nestjs/microservices</code
[X] code>@nestjs/platform-express</code
[ ] code>@nestjs/platform-fastify</code
[ ] code>@nestjs/platform-socket.io</code
[ ] code>@nestjs/platform-ws</code
[ ] code>@nestjs/testing</code
[ ] code>@nestjs/websockets</code
[ ] Other (see below)

Other package

busbuy/multer

NestJS version

10.2.5

Packages versions

  "dependencies": {
    "@nestjs/common": "^10.2.5",
    "@nestjs/core": "^10.2.5",
    "@nestjs/platform-express": "^10.2.5",
    "reflect-metadata": "^0.1.13",
    "rxjs": "^7.8.1"
  },
  "devDependencies": {
    "@nestjs/cli": "^ 10.1.17",
    "@nestjs/schematics": "^10.0.2",
    "@nestjs/testing": "^10.2.5",
    "@types/express": "^4.17.17",
    "@types/jest": "29.2.4",
    "@types/multer": "^1.4.7",
    "@types/node": "18.11.18",
    "@types/supertest": "^2.0.11",
    "@typescript-eslint/eslint-plugin": "^5.0.0",
    "@typescript-eslint/parser": "^5.0.0",
    "eslint": "^8.0.1",
    "eslint-config-prettier": "^8.3.0",
    "eslint-plugin-prettier": "^4.0.0",
    "jest": "29.3.1",
    "prettier": "^2.3.2",
    "source-map-support": "^0.5.20",
    "supertest": "^6.1.3",
    "ts-jest": "29.0.3",
    "ts-loader": "^9.2.3",
    "ts-node": "^10.0.0",
    "tsconfig-paths": "4.1.1",
    "typescript": "^4.7.4"
  },

[System Information] OS Version : macOS Unknown NodeJS Version : v18.17.1 YARN Version : 1.22.19

[Nest CLI] Nest CLI Version : 10.1.17

[Nest Platform Information] platform-express version : 10.2.5 schematics version : 10.0.2 testing version : 10.2.5 common version : 10.2.5 core version : 10.2.5 cli version : 10.1.17

Node.js version

18.17.1

In which operating systems have you tested?

[X] macOS
[ ] Windows
[ ] Linux

Other

Here is the exact error:

/Users/eamon/work/minimum-reproduction-crash/node_modules/busboy/lib/types/multipart.js:588
      return cb(new Error('Unexpected end of form'));
                ^
Error: Unexpected end of form
    at Multipart._final (/Users/eamon/work/minimum-reproduction-crash/node_modules/busboy/lib/types/multipart.js:588:17)
    at callFinal (node:internal/streams/writable:698:12)
    at prefinish (node:internal/streams/writable:710:7)
    at finishMaybe (node:internal/streams/writable:720:5)
    at Multipart.Writable.end (node:internal/streams/writable:634:5)
    at IncomingMessage.onend (node:internal/streams/readable:705:10)
    at Object.onceWrapper (node:events:628:28)
    at IncomingMessage.emit (node:events:514:28)
    at endReadableNT (node:internal/streams/readable:1359:12)
    at processTicksAndRejections (node:internal/process/task_queues:82:21)
error Command failed with exit code 1.

https://github.com/nestjs/nest/assets/21305201/5055cd4e-62dd-4613-be4c-0f350379f353

In case this issue gets closed, we are using a temporary workaround from https://github.com/expressjs/multer/pull/1177 where we modify line 44 in node_modules/multer/lib/make-middleware.js from busboy.removeAllListeners() to:

process.nextTick(function () {
  busboy.removeAllListeners()
})

using https://github.com/ds300/patch-package

jmcdo29 commented 1 year ago

Nest is already listening for the error callback from multer, so the fact that multer doesn't return this, means that, I think, it's out of our hands and needs to be fixed in the upstream package.

Thank you so much, however, for the reproduction! I'll tinker with it later to see if there really is anything we can do to keep this crash from happening

eamon0989 commented 1 year ago

No problem, happy to help! It was a tricky one to reproduce! Hopefully something can be done, either upstream or here.

Usama-Tahir commented 1 year ago

Hey, I am experiencing a kinda similar issue with using graphql-upload-ts with nestjs. Unhandled promise from graphql-upload-ts crashes the server. Is there a way in Nest.js to handle it gracefully without crashing the server?

I am using it as a middleware in main.ts file.

  app.use(
    graphqlUploadExpress({
      maxFileSize: 5000000,
      maxFiles: 5,
    }),
  );

kamilmysliwiec commented 1 year ago

https://github.com/nestjs/nest/issues/12415#issuecomment-1731543984

nestjs / nest