Potential Bug in httpsrv_readreq() in RTCS for MQX for KSDK 1.1

[bowe]

We are using MQX for KDSK 1.1.  In looking at httpsrv_readreq() in httpsrv_task.c, it seems that there is a potential bug that might occur if the the buffer is completely filled at line 577 because line 602 might not find a '\n' in the buffer and continue reading past it.  This bug would be exploited in the case that an HTTP request header has a line that is larger than the buffer, which will probably never happen unless intentionally exploited.  I have not experimentally proven this bug, so maybe there is some way I am not seeing that prevents it.

Here is line 577:

read = httpsrv_recv(session, session->buffer.data+session->buffer.offset, HTTPSRV_SES_BUF_SIZE_PRV-session->buffer.offset, 0);

Here is line 602:

while (session->request.process_header && ((line_end = strchr(line_start, '\n')) != NULL))

The fix we implemented until we move to MQX for KSDK 1.2 is to ensure the buffer is never completely filled (and then null-terminating).  This is done by changing line 577, as shown below, and leaving line 602 alone.  The second line in this fix is probably not necessary since the free buffer space is zero-ed in line 653.

read = httpsrv_recv(session, session->buffer.data+session->buffer.offset, HTTPSRV_SES_BUF_SIZE_PRV-session->buffer.offset-1, 0);
if(read > 0) session->buffer.data[session->buffer.offset+read] = '\0';

Again, recommendations/comments are welcome.

-Bowe

NOTE:  In MQX for KSDK 1.2, it looks like this function has been renamed to httpsrv_req_read(), and the potential bug is fixed by using memchr(), which takes a length.  So this potential problem is probably already known.

[danielchen]

Hi Bowe:

Every http header should be ended with "\r\n", please check the httpsrv_sendhdr function in httpsrv_supp.c

....

	/* End of header */
	httpsrv_print(session, "\r\n");

.....,

otherwise, the receive socket maybe blocked.

Regards

Daniel

[bowe]

This has nothing to do with how an HTTP header ends.  This has to do with potentially reading past the end of the buffer in the case of a really long line in a header (probably in the case of an intentional attack).

-Bowe