search

mas-bandwidth / netcode

Secure client/server connections over UDP
BSD 3-Clause "New" or "Revised" License
2.43k stars 190 forks source link

Reporting security vulnerabilities #95

Closed maxweisel closed 4 years ago

maxweisel commented 4 years ago

BANNED

maxweisel commented 4 years ago

Hey @pushrax & @gafferongames,

My apologies, I was planning to write up a detailed explanation and proof of concept before submitting. The last time I reported an overflow on yojimbo, I was only met with comments about the software being improperly configured. It wasn't until I had spent months debugging it myself that I found the actual cause of the issue and it turns out, it was not my config as @gafferongames claimed multiple times, it was an overflow. Call it a difference of opinion, but I was using the default config, and I don't believe it should be possible for default configuration to cause an overflow.

This time around, I figured I would put a repro project together beforehand to avoid any confusion. However, in working on this repro project, I came to the conclusion that I do not believe it is possible to properly secure yojimbo / netcode, and I removed it as a dependency from my project entirely.

As such, I have not had the time to finish this repro project (again the last one took a long time to reproduce reliably), but I am able to reliably crash netcode with a fuzzer. All length / size checks are written using netcode_assert, which gets compiled to a no-op in production builds: https://github.com/networkprotocol/netcode.io/blob/master/netcode.h#L253.

Personally, I think this is unacceptable. Production builds should not be allowed to overflow. That said, even if you change release builds to call exit(), I do not believe our software should quit when given malformed data, it should be able to handle those cases correctly and continue running.

I hope this is helpful, sorry for the salty message...

Max