umpProcessor sysex7/8 potential buffer overruns

[paulhuggett]

I'm sure that you're probably sick of hearing from me by now… just let me know if you'd like me to stop!

umpProcessor.cpp contains the following code:

if(mt == UMP_SYSEX7 && sendOutSysex != nullptr){  umpData mess = umpData();
  mess.umpGroup = group;
  mess.messageType = mt;
  mess.form = (umpMess[0] >> 20) & 0xF;
  mess.dataLength  = (umpMess[0] >> 16) & 0xF;
  uint8_t sysex[6];
…
  mess.data = sysex;
  sendOutSysex(mess);

This sets mess.dataLength from the 4 bits extracted from the 32-bit UMP packet — the ”#of bytes” field — and sets mess.data to point to a 6 byte stack-allocated array. Unfortunately, this 4-bit field could contain any value up to 15. Bad input data where the field contains a value > 6 could cause the sendOutSysEx() callback to read beyond the end of the mess.data buffer.

My suggested fix is simply to clamp the value of dataLength:

mess.dataLength = std::min((umpMess[0] >> 16) & 0xF, 6U);

(Discovered when building some fuzz tests for umpProcessor.)

[paulhuggett]

Sorry, I should have included some sample input that exposes the issue rather than just vague warbling… Try:

0x39671FF1, 0x92021008, 0xFFFFFFFF, 0x66BF99A, 0x00000001

This was generated by the fuzzer so is likely nonsensical!

[paulhuggett]

Later… I think there's a similar bug in the sysex8 handling where we have (line #369):

mess.dataLength  = (umpMess[0] >> 16) & 0xF

dataLength shouldn't be greater than 13 because we then pass a pointer to an array of 13 bytes to the callback function. A simple fix would be std::min().

I also think that the conditions in lines #373 to #385 should be using >= rather than >. i.e.

if(mess.dataLength > 1)sysex[0] =  umpMess[0] & 0x7F;
if(mess.dataLength > 2)sysex[1] =  (umpMess[1] >> 24) & 0x7F;
…

should be:

if(mess.dataLength >= 1)sysex[0] =  umpMess[0] & 0x7F;
if(mess.dataLength >= 2)sysex[1] =  (umpMess[1] >> 24) & 0x7F;
…

This means that a dataLength of 1 extracts from umpMess[0], a dataLength of 2 from umpMess[1], and so on.

Actually, there's another tweak necessary here because the sysex8 bytes are each 8-bits so the mask should be 0xFF (not 0x7F):

if(mess.dataLength >= 1)sysex[0] =  umpMess[0] & 0xFF;
if(mess.dataLength >= 2)sysex[1] =  (umpMess[1] >> 24) & 0xFF;
…

[starfishmod]

@paulhuggett Thanks for these - I have updated the code as you suggested :)