goodixmoc: Further template parsing fixes

In commit 5c28654d9 ("goodixmoc: Fix print template parsing") the length
check for the verify and duplicate check responses by requiring two
extra bytes at the end of the message.

There were also issues in other places where the length was not checked
correctly, including a scenario that could cause a read beyond the end
of the buffer.

Related: #444
This commit is contained in:
Benjamin Berg
2021-12-27 13:44:16 +01:00
parent eb1013cdb6
commit 038c7108a6

View File

@@ -259,12 +259,9 @@ gx_proto_parse_fingerid (
if (buffer[Offset++] != 67) if (buffer[Offset++] != 67)
return -1; return -1;
fid_buffer_size--;
template->type = buffer[Offset++]; template->type = buffer[Offset++];
fid_buffer_size--;
template->finger_index = buffer[Offset++]; template->finger_index = buffer[Offset++];
fid_buffer_size--;
Offset++; Offset++;
memcpy (template->accountid, &buffer[Offset], sizeof (template->accountid)); memcpy (template->accountid, &buffer[Offset], sizeof (template->accountid));
Offset += sizeof (template->accountid); Offset += sizeof (template->accountid);
@@ -273,6 +270,8 @@ gx_proto_parse_fingerid (
template->payload.size = buffer[Offset++]; template->payload.size = buffer[Offset++];
if (template->payload.size > sizeof (template->payload.data)) if (template->payload.size > sizeof (template->payload.data))
return -1; return -1;
if (template->payload.size + Offset > fid_buffer_size)
return -1;
memset (template->payload.data, 0, template->payload.size); memset (template->payload.data, 0, template->payload.size);
memcpy (template->payload.data, &buffer[Offset], template->payload.size); memcpy (template->payload.data, &buffer[Offset], template->payload.size);
@@ -365,9 +364,12 @@ gx_proto_parse_body (uint16_t cmd, uint8_t *buffer, uint16_t buffer_len, pgxfp_c
if (buffer_len < 3) if (buffer_len < 3)
return -1; return -1;
uint16_t tid_size = GUINT16_FROM_LE (*(uint16_t *) (buffer + 1)); uint16_t tid_size = GUINT16_FROM_LE (*(uint16_t *) (buffer + 1));
if ((buffer_len < tid_size + 3) || (buffer_len > sizeof (template_format_t)) + 3) offset += 3;
if (buffer_len < tid_size + offset)
return -1;
if (gx_proto_parse_fingerid (buffer + offset, tid_size, &presp->check_duplicate_resp.template) != 0)
return -1; return -1;
memcpy (&presp->check_duplicate_resp.template, buffer + 3, tid_size);
} }
break; break;
@@ -380,9 +382,12 @@ gx_proto_parse_body (uint16_t cmd, uint8_t *buffer, uint16_t buffer_len, pgxfp_c
fingerlist = buffer + 2; fingerlist = buffer + 2;
for(uint8_t num = 0; num < presp->finger_list_resp.finger_num; num++) for(uint8_t num = 0; num < presp->finger_list_resp.finger_num; num++)
{ {
uint16_t fingerid_length = GUINT16_FROM_LE (*(uint16_t *) (fingerlist + offset)); uint16_t fingerid_length;
if (buffer_len < offset + 2)
return -1;
fingerid_length = GUINT16_FROM_LE (*(uint16_t *) (fingerlist + offset));
offset += 2; offset += 2;
if (buffer_len < fingerid_length + offset + 2) if (buffer_len < fingerid_length + offset)
return -1; return -1;
if (gx_proto_parse_fingerid (fingerlist + offset, if (gx_proto_parse_fingerid (fingerlist + offset,
fingerid_length, fingerid_length,
@@ -405,7 +410,7 @@ gx_proto_parse_body (uint16_t cmd, uint8_t *buffer, uint16_t buffer_len, pgxfp_c
presp->verify.match = (buffer[0] == 0) ? true : false; presp->verify.match = (buffer[0] == 0) ? true : false;
if (presp->verify.match) if (presp->verify.match)
{ {
if (buffer_len < sizeof (template_format_t) + 10) if (buffer_len < 10)
return -1; return -1;
offset += 1; offset += 1;
presp->verify.rejectdetail = GUINT16_FROM_LE (*(uint16_t *) (buffer + offset)); presp->verify.rejectdetail = GUINT16_FROM_LE (*(uint16_t *) (buffer + offset));
@@ -416,6 +421,8 @@ gx_proto_parse_body (uint16_t cmd, uint8_t *buffer, uint16_t buffer_len, pgxfp_c
offset += 1; offset += 1;
fingerid_size = GUINT16_FROM_LE (*(uint16_t *) (buffer + offset)); fingerid_size = GUINT16_FROM_LE (*(uint16_t *) (buffer + offset));
offset += 2; offset += 2;
if (buffer_len < fingerid_size + offset)
return -1;
if (gx_proto_parse_fingerid (buffer + offset, fingerid_size, &presp->verify.template) != 0) if (gx_proto_parse_fingerid (buffer + offset, fingerid_size, &presp->verify.template) != 0)
{ {
presp->result = GX_FAILED; presp->result = GX_FAILED;