Re: [PATCH v2 04/15] Add code to read and decrypt OSTC3 firmware files

On 19 December, 2014 - Jef Driesen wrote:

On 2014-12-17 23:11, Anton Lundin wrote:

...

+// This is a variant of fletcher16 with 16 bit sum instead of 8bit +// and modulo 256 instead of 255 +static unsigned int hw_ostc3_firmware_checksum(hw_ostc3_firmware_t *firmware)

The comment is wrong. It's modulo 65536 (2^16) vs 65535 (2^16-1) due to the 16bit sum.

Yea, you're correct. I just wanted to document that its not fletcher16 or fletcher32, but some sort of in-between.

...

+static dc_status_t +hw_ostc3_firmware_readline(FILE *fp, unsigned int addr, unsigned char data[], unsigned int size) +{

• unsigned char ascii[40];
• // 1 byte :, 6 bytes addr, X*2 bytes hex -> X bytes data.
• const unsigned line_size = size * 2 + 1 + 6 + 1;
• unsigned char faddr_byte[3];
• unsigned int faddr = 0;
• if (line_size > sizeof(ascii))

• return DC_STATUS_INVALIDARGS;
  

• if (fread (ascii, sizeof(char), line_size, fp) != line_size)

• return DC_STATUS_IO;
  

• if (ascii[0] != ':')

• return DC_STATUS_DATAFORMAT;
  

• // Is it a CRLF file?
• // Read away that trailing LF
• if (ascii[line_size] == '\r')

• if (fread (ascii + line_size, sizeof(char), 1, fp) != 1)
  

• 	return DC_STATUS_DATAFORMAT;
  

• if (array_convert_hex2bin(ascii + 1, 6, faddr_byte,

sizeof(faddr_byte)) != 0) {

• return DC_STATUS_DATAFORMAT;
  

• }
• faddr = array_uint24_be (faddr_byte);
• if (faddr != addr)

• return DC_STATUS_DATAFORMAT;
  

• if (array_convert_hex2bin (ascii + 1 + 6, size*2, data, size) != 0)

• return DC_STATUS_DATAFORMAT;
  

• return DC_STATUS_SUCCESS;

+}

There are two bugs in this code. You read line_size bytes, and then try to access ascii[line_size], which is left uninitialized. The ascii array is also not large enough to store CRLF, resulting in a buffer overflow.

There are two -1 that was missing there to make the code do what i ment for it to do.

I never ment to store CRLF, only LF, so if there is a CRLF ending, throw away the CR.

I have attached a different implementation, which is based on the existing ihex code. It can handle arbitrary line endings, and not just CRLF or LF. So if you don't mind, I prefer to use this version.

I actually think its uglier, and thats why i didn't copy that pattern. But if thats your preferred way to read data, you're the maintainer.

And... a bit more thinking. The firmware files are distributed inside zip-files. If you do anything to them, i would say its a feature to reject that file. The odds of flipping some bits that decrypt into something that matches the checksum are pretty slim, but i wouldn't trust a firmware file that had a blank line in it...

//Anton