Fix potential ReDOS-Attack-Vector in Headerparser

[Uzlopak]

tl;dr;
An attacker could send a payload with large headers, which contain no colon, repeatedly, resulting in a REDoS-Attack.

The Regex for handling headerpairs is prone to catastrophic backtracking.

This is even more critical, as busboys default limit for multipart headers is 80kby and that if a header is not valid, the counter for headers is not increasing. Meaning, that the attacker could send a big payload of multipart formdata, were each multipart contains 80kby of invalid headerdata resulting in a DoS-Attack.

To avoid this, we just check if the line contains a colon and if so ensure that it is not at the first position. If not we exit early. If not we can savely use the Regex for the header as before.

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[Uzlopak]

benchmarks before:

npm run benchmark-fastify -- -p low

> [email protected] benchmark-fastify /home/aras/Workspace/fastify/busboy/benchmarks
> node busboy/executioner.js -c 1 "-p" "low"

Mean time for fastify-busboy is 285299.9631169119 nanoseconds
all done

benchmarks after:

npm run benchmark-fastify -- -p low

> [email protected] benchmark-fastify /home/aras/Workspace/fastify/busboy/benchmarks
> node busboy/executioner.js -c 1 "-p" "low"

Mean time for fastify-busboy is 284407.45698760776 nanoseconds
all done

no performance degredation.

[kibertoad]

shouldn't we throw an error instead? is this even valid payload?

[Uzlopak]

Current behavior is skipping the header, if it is invalid. I would not want to change that, as I am not sure what side effects this could have. Maybe there is some reason that we just ignore it.

Or somebody is using for years busboy in his legacy product with a self written client. Then he wants to update his product to make it more resilient and then we throw errors resulting in a bigger man power investment as now the program breaks for something which was always handled gracefully. This could result in keeping the old busboy in the product with the argument, that the security fix is not worth the time to bugfix the legacy product.

[kibertoad]

would be helpful to at least log an error then. silent ignoring may be confusing and hard to debug

[Uzlopak]

We have no logging functionality in busboy :/
Also before there was no logging of the invalid header.

[kibertoad]

we should add some. not in this pr, though

[Uzlopak]

thanks for the mercy :)

[Uzlopak]

@kibertoad
Anything I a can do?

[kibertoad]

@Uzlopak I think this ties in to having strict mode discussed in another PR. If we go in that direction, enabling strict mode should probably emit an error in this case as well.

[Uzlopak]

I can understand that, but I would actually recommend to merge this anyway as this fixes a potential security issue and does not modify in any way the original busboy behaviour. We can still throw/emit an Error when we decide to implement a strict Mode.