Potential bug: Busboy crashes with unescaped quotes in field names/filenames (or: Unexpected strictness in quote handling)

[furnivall]

Hi folks,

It appears that busboy is able to trigger application crashes when encountering unescaped quotes within field names or filenames. This behavior has been observed in multiple applications and endpoints, leading to full application crashes which could cause denial of service and unexpected behavior.

It's important to note that the crash only occurs if the consuming application does not have proper error handling to catch unhandled exceptions or errors emitted by Busboy. If the application has robust error handling, it can potentially catch these errors and prevent the process from crashing. However, the underlying issue of Busboy encountering parsing errors due to unescaped quotes remains, even if the application doesn't crash.

I was also able to get this to crash applications which consume busboy via other libraries (specifically multer)

Minimal Reproduction:

• Create a file (x.js in our example) with the following content:

const http = require('http');
const Busboy = require('busboy');

http.createServer((req, res) => {
  if (req.method === 'PUT' && req.headers['content-type'].startsWith('multipart/form-data')) {
    const busboy = Busboy({ headers: req.headers });

    busboy.on('error', (err) => {
      console.error('Busboy error:', err);
      res.writeHead(500, { 'Connection': 'close' });
      res.end('Error');
    });

    req.pipe(busboy);
  } else {
    res.writeHead(404);
    res.end();
  }
}).listen(3000, () => {
  console.log('Server listening on port 3000');
});

• run the application with node x.js
• send the following curl request:

curl 'http://localhost:3000'   -X PUT   -H 'Content-Type: multipart/form-data; boundary=bound'   --data-binary $'--bound\r\nContent-Disposition: form-data; name="name"\r\n\r\n"javascript:alert(1)"\r\n--bound\r\nContent-Disposition: form-data; name="photo"; filename="x.jpg"\r\nContent-Type: image/jpeg\r\n\r\n--bound--\r\n'

Results

This should, in theory, crash the app with the following error:

Busboy error: Error: Malformed part header
    at SBMH.ssCb [as _cb] (/Users/me/Developer/node_modules/busboy/lib/types/multipart.js:398:32)
    at SBMH.destroy (/Users/me/Developer/node_modules/streamsearch/lib/sbmh.js:111:12)
    at Multipart._final (/Users/me/Developer/node_modules/busboy/lib/types/multipart.js:586:19)
    at prefinish (node:internal/streams/writable:906:14)
    at finishMaybe (node:internal/streams/writable:920:5)
    at Writable.end (node:internal/streams/writable:835:5)
    at IncomingMessage.onend (node:internal/streams/readable:946:10)
    at Object.onceWrapper (node:events:632:28)
    at IncomingMessage.emit (node:events:518:28)
    at endReadableNT (node:internal/streams/readable:1696:12)
Busboy error: Error: Unexpected end of form
    at Multipart._final (/Users/me/Developer/node_modules/busboy/lib/types/multipart.js:588:17)
    at prefinish (node:internal/streams/writable:906:14)
    at finishMaybe (node:internal/streams/writable:920:5)
    at Writable.end (node:internal/streams/writable:835:5)
    at IncomingMessage.onend (node:internal/streams/readable:946:10)
    at Object.onceWrapper (node:events:632:28)
    at IncomingMessage.emit (node:events:518:28)
    at endReadableNT (node:internal/streams/readable:1696:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
node:_http_server:345
    throw new ERR_HTTP_HEADERS_SENT('write');
    ^

Error [ERR_HTTP_HEADERS_SENT]: Cannot write headers after they are sent to the client
    at ServerResponse.writeHead (node:_http_server:345:11)
    at Multipart.<anonymous> (/Users/me/Developer/x.js:10:11)
    at Multipart.emit (node:events:518:28)
    at emitErrorNT (node:internal/streams/destroy:169:8)
    at emitErrorCloseNT (node:internal/streams/destroy:128:3)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  code: 'ERR_HTTP_HEADERS_SENT'
}

Caveat

I'm not sure if this is intended behaviour but I thought it was worth reporting on the unlikely off chance this hasn't been investigated previously. If it IS intended behaviour, the error messages could perhaps be a little more descriptive than they are.

[mscdex]

It appears that busboy is able to trigger application crashes when encountering unescaped quotes within field names or filenames.

1. busboy is not triggering crashes, the lack of error handling would be.
2. Raising an error on unescaped quotes seems sensible to me. Are you saying that's not happening or what? The reason I ask is that you later phrase things like (emphasis mine): "This should, in theory, crash the app with the following error"

node:_http_server:345
throw new ERR_HTTP_HEADERS_SENT('write');
^

Error [ERR_HTTP_HEADERS_SENT]: Cannot write headers after they are sent to the client
at ServerResponse.writeHead (node:_http_server:345:11)
at Multipart. (/Users/me/Developer/x.js:10:11)
at Multipart.emit (node:events:518:28)
at emitErrorNT (node:internal/streams/destroy:169:8)
at emitErrorCloseNT (node:internal/streams/destroy:128:3)
at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
code: 'ERR_HTTP_HEADERS_SENT'
}

Is this what you're referring to when you're saying crashes are being triggered? If so, it's because you have buggy logic in your error handler where you're trying to write response headers multiple times. That error is coming from node core and has nothing to do with busboy.

[furnivall]

It appears that busboy is able to trigger application crashes when encountering unescaped quotes within field names or filenames.

1. `busboy` is not triggering crashes, the lack of error handling would be.

2. Raising an error on unescaped quotes seems sensible to me. Are you saying that's _not_ happening or what? The reason I ask is that you later phrase things like (emphasis mine): "This should, _in theory_, crash the app with the following error"

node:_http_server:345
throw new ERR_HTTP_HEADERS_SENT('write');
^
Error [ERR_HTTP_HEADERS_SENT]: Cannot write headers after they are sent to the client
at ServerResponse.writeHead (node:_http_server:345:11)
at Multipart. (/Users/me/Developer/x.js:10:11)
at Multipart.emit (node:events:518:28)
at emitErrorNT (node:internal/streams/destroy:169:8)
at emitErrorCloseNT (node:internal/streams/destroy:128:3)
at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
code: 'ERR_HTTP_HEADERS_SENT'
}

Is this what you're referring to when you're saying crashes are being triggered? If so, it's because you have buggy logic in your error handler where you're trying to write response headers multiple times. That error is coming from node core and has nothing to do with busboy.

My apologies. I completely agree with everything you've said here and I'm not trying to suggest that there's anything fundamentally wrong with busboy here, and I fully appreciate the issue may be badly phrased.

Essentially, you can boil down why I reported this like so:

1. This is able to crash a lot of applications who are consuming busboy or multer improperly (i.e. without appropriate error handling) using malformed curl requests.

2. The error message is not particularly clear in terms of trying to diagnose what went wrong - although I again appreciate there may be reasons for this.

If you don't deem it to be an issue that's completely fine and I apologise again for the hassle if not!