Re: [PATCH 02/11] This adds code to read OSTC3 firmware files.

On 15 December, 2014 - Jef Driesen wrote:

On 21-11-14 21:28, Anton Lundin wrote:

...

+// FIXME: This key is specific to the ostc3 +// The ostc sport has another key but the same protocoll +// How should we refactor the code for that? +static unsigned char ostc3_key[16] = {

• 0xF1, 0xE9, 0xB0, 0x30,
• 0x45, 0x6F, 0xBE, 0x55,
• 0xFF, 0xE7, 0xF8, 0x31,
• 0x13, 0x6C, 0xF2, 0xFE

+};

This should be marked as const.

Agreed and fixed.

...

+static unsigned int hw_ostc3_firmware_checksum(hw_ostc3_firmware_t *firmware) +{

• unsigned short low = 0;
• unsigned short high = 0;
• for(unsigned int i = 0;i < SZ_FIRMWARE;i++)
• {

• low  += firmware->data[i];
  

• high += low;
  

• }
• return (((unsigned int)high) << 16) + low;

+}

This looks like a variant of the fletcher16 checksum (with a 16bit sum instead of 8bit sum, and modulo 2^n instead of 2^n-1). So I suggest moving it to checksum.c where it can be reused elsewhere.

Is that such a wise idea? Sure, its a cousin of fletcher16, but do you think anyone else will be using the same cousin of fletcher16?

If it where a straight fletcher16, i would have agreed with you but this one, I'm not so sure. Just think, what should we name the darn thing when we move it to checksum.c? ostc3_firmware_fletcher16_almost()?

...

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

• char ascii[40];
• // 1 byte :, 6 bytes addr, X*2 bytes hex -> X bytes data.
• 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_IO;
  

• if (array_convert_hex2bin(ascii + 1, 6, faddr_byte, sizeof(faddr_byte)) != 0) {

• return DC_STATUS_IO;
  

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

• return DC_STATUS_IO;
  

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

• return DC_STATUS_IO;
  

• return DC_STATUS_SUCCESS;

+}

This will needs some improvements to be able to deal with different line endings (e.g CRLF vs LF). See ihex.c for an example.

None of the Ostc3 or Ostc Sport firmware files have CRLF endings. Probably due to the tool with which they bin generated.

But, when re-reading the HexFile::loadEncrypted of ostc-planner, it would accept a file with CRLF encodings, so we should do the same.

Fixed.

DC_STATUS_DATAFORMAT is also a more appropriate error code than DC_STATUS_IO here. Except for the fread one.

Agreed and fixed.

...

+static dc_status_t +hw_ostc3_firmware_readfile (hw_ostc3_firmware_t *firmware, dc_context_t *context, const char *filename) +{

• dc_status_t rc = DC_STATUS_SUCCESS;
• FILE *fp;
• unsigned char iv[16] = {0};
• unsigned char tmpbuf[16] = {0};
• unsigned char encrypted[16] = {0};
• unsigned int bytes = 0, addr = 0, faddr = 0;
• char checksum[4];
• if (firmware == NULL) {

• ERROR (context, "Invalid arguments.");
  

• return DC_STATUS_INVALIDARGS;
  

• }
• // Initialize the buffers.
• memset (firmware->data, 0xFF, sizeof (firmware->data));
• firmware->checksum = 0;
• fp = fopen (filename, "rb");
• if (fp == NULL) {

• ERROR (context, "Failed to open the file.");
  

• return DC_STATUS_IO;
  

• }
• rc = hw_ostc3_firmware_readline(fp, 0, iv, sizeof(iv));
• if (rc != DC_STATUS_SUCCESS) {

• fclose (fp);
  

• ERROR (context, "Failed to parse header.");
  

• return rc;
  

• }
• bytes += 16;
• // Load the iv for AES-FCB-mode
• AES128_ECB_encrypt(iv, ostc3_key, tmpbuf);
• for(addr = 0; addr < SZ_FIRMWARE; addr += 16, bytes += 16) {

• rc = hw_ostc3_firmware_readline(fp, bytes, encrypted, sizeof(encrypted));
  

• if (rc != DC_STATUS_SUCCESS) {
  

• 	fclose (fp);
  

• 	ERROR (context, "Failed to parse file data.");
  

• 	return rc;
  

• }
  

• // Decrypt AES-FCB data
  

• for(int i=0;i < 16;i++)
  

• 	firmware->data[addr + i] = encrypted[i] ^ tmpbuf[i];
  

• // Run the next round of encryption
  

• AES128_ECB_encrypt(encrypted, ostc3_key, tmpbuf);
  

• }
• // This file format contains a tail with the checksum in
• rc = hw_ostc3_firmware_readline(fp, bytes, checksum, sizeof(checksum));
• if (rc != DC_STATUS_SUCCESS) {

• fclose (fp);
  

• ERROR (context, "Failed to parse file tail.");
  

• return rc;
  

• }
• fclose(fp);
• firmware->checksum = array_uint32_le(checksum);
• if (firmware->checksum != hw_ostc3_firmware_checksum(firmware)) {

• ERROR (context, "Failed to verify file checksum.");
  

• return DC_STATUS_IO;
  

• }
• return DC_STATUS_SUCCESS;

+}

Two questions here:

If I understand correctly, the address in the firmware file is not really an address in the normal sense, but more like a sequence number, right? Can they be in a random order, or are they always nicely incrementing as the code assumes?

All the firmware files I've seen, they are always linear. The ostc-planner code also assumes that they always will be and will fail reading the file if they aren't, so thats the reason this code will reject such a file to.

Is the payload always guaranteed to be 16 bytes (except for the last line with the checksum)? I assume it is because there is no length stored anywhere.

I haven't seen any files in any other format. They are all the exact same size, 307256.

If yo take a look at the decrypted data, its padded to be this exact firmware size.

//Anton