repetier / Repetier-Firmware

Firmware for Arduino based RepRap 3D printer.
815 stars 734 forks source link

[BUGFIX] text message length is not validated for binary protocol v2 #1089

Closed toomium closed 2 years ago

toomium commented 2 years ago

While performing a fuzzing campaign on different G-Code parsers, I stumbled across this bug. It allows the null-terminator of the text message to be written up to 255 Bytes away from the beginning of the text message inside the "commandReceiving" buffer, leading to a global buffer overflow. It is caused by a missing validation of the text length, that is included in the binary protocol V2. Fixing this bug is just as easy as letting the "textlen" variable be no higher than 80, just as it is done in the "computeBinarySize" function.

repetier commented 2 years ago

I see your point. If the size is wrong - being an attack or communication error it will lead to a byte being changed outside it's buffer. But the solution is wrong I feat. textlen = p++; sets it to p value and increases p afterwards. textlen = RMath::min(80, p++ + 1); should be textlen = RMath::min(80, *p);p++;

But that also might create issues if you had many other values set (>4) so we exceed 96 byte. Plus this normally only happens on com errors so it causes some errors later when checking checksums. So that should be recovered by resend requests afterwards. So it must be just prevented to write outside. Will think about a correct solution and add it to dev branch.

Thanks.