Merge pull request #568 from DrDoof/fix/upload-boundary-cr

httpd: re-check the byte that breaks a partial boundary match

httpd/httpd.c

@@ -504,9 +504,10 @@ uint8_t stream_upload(void)
 				memcpy(flash_buf + write_len, boundary, bindex);
 				write_len += bindex;
 				bindex = 0;
-			}
+			} else {
 				crc16_bank1(upload_settings.p + upload_settings.bptr);
 				flash_buf[write_len++] = upload_settings.p[upload_settings.bptr++];
+			}
 			if (write_len >= FLASH_PAGE_SIZE) {
 				/* The staged image spans FIRMWARE_UPLOAD_START to twice that,
 				 * the span check_and_flash_update_image() reads back. Nothing
@@ -538,7 +539,6 @@ uint8_t stream_upload(void)
 					memcpy(flash_buf, flash_buf + FLASH_PAGE_SIZE, write_len);
 				}
 			}
-			bindex = 0;
 		}
 	} while(1);
 }

test/test_httpd_tx.c

@@ -81,7 +81,7 @@ static uint8_t written_config[CONFIG_LEN];
 static uint16_t written_config_len;
 static uint32_t config_erase_addr, config_write_addr;
 static int config_erases, config_writes;
-static uint8_t flash_written[64];
+static uint8_t flash_written[1024];
 static size_t flash_written_len;
 static int flash_write_bad;
 
@@ -552,7 +552,7 @@ static void scenario_bad_l4_checksum(void)
 
 
 /* Private HTTP lifecycle values; drive the real callback, not a model. */
-enum { TSTATE_NONE = 0, TSTATE_TX = 1 };
+enum { TSTATE_NONE = 0, TSTATE_TX = 1, TSTATE_POST = 4 };
 extern uint16_t slen, cont_len, sent_len, bindex, crc_final, write_len;
 extern uint8_t boundary[], verify_crc, outbuf[];
 extern struct { uint8_t *p; uint16_t bptr, plen; } upload_settings;
@@ -1156,6 +1156,115 @@ static void scenario_upload(const char *name, const char *part_type, int split)
 	flash_size = saved_flash_size;
 }
 
+extern uint32_t uptr;
+
+#define UPLOAD_LEN	600
+#define UPLOAD_BOUNDARY	"\r\n--XyZ"
+#define UPLOAD_TRAILER	UPLOAD_BOUNDARY "--\r\n"
+
+static int upload_in_two(uint8_t *body, uint16_t body_len, uint16_t split)
+{
+	uptr = FIRMWARE_UPLOAD_START;
+	write_len = 0;
+	bindex = 0;
+	verify_crc = 0;
+	flash_written_len = 0;
+	flash_write_bad = 0;
+	strcpy((char *)boundary, UPLOAD_BOUNDARY);
+	uip_conn = &uip_conns[0];
+	uip_conn->appstate.tstate = TSTATE_POST;
+
+	upload_settings.p = body;
+	upload_settings.bptr = 0;
+	upload_settings.plen = split;
+	stream_upload();
+	if (uip_conn->appstate.tstate == TSTATE_POST) {
+		upload_settings.p = body + split;
+		upload_settings.bptr = 0;
+		upload_settings.plen = body_len - split;
+		stream_upload();
+	}
+	return uip_conn->appstate.tstate == TSTATE_NONE
+		&& uptr == FIRMWARE_UPLOAD_START + UPLOAD_LEN && !flash_write_bad
+		&& flash_written_len == UPLOAD_LEN + 1 && !flash_written[UPLOAD_LEN];
+}
+
+static void scenario_upload_cr_before_boundary(void)
+{
+	uint8_t body[UPLOAD_LEN + sizeof(UPLOAD_TRAILER)];
+	uint16_t body_len, split;
+	int completed = 0, intact = 0, splits = 0;
+
+	for (uint16_t i = 0; i < UPLOAD_LEN; i++)
+		body[i] = pattern(i);
+	body[UPLOAD_LEN - 2] = 'x';
+	body[UPLOAD_LEN - 1] = '\r';
+	body_len = UPLOAD_LEN + sizeof(UPLOAD_TRAILER) - 1;
+	memcpy(body + UPLOAD_LEN, UPLOAD_TRAILER, sizeof(UPLOAD_TRAILER) - 1);
+
+	for (split = 0; split <= body_len; split++) {
+		splits++;
+		if (upload_in_two(body, body_len, split)) {
+			completed++;
+			if (!memcmp(flash_written, body, UPLOAD_LEN))
+				intact++;
+		}
+	}
+	if (verbose)
+		printf("      %d of %d splits complete, %d intact\n", completed, splits, intact);
+
+	CHECK(completed == splits,
+	      "upload: an image ending in CR still finds the closing boundary");
+	CHECK(intact == splits,
+	      "upload: the CR and the bytes before it reach the flash unchanged");
+}
+
+static int upload_whole_and_split(uint8_t *body, uint16_t body_len)
+{
+	return upload_in_two(body, body_len, body_len) && !memcmp(flash_written, body, UPLOAD_LEN)
+		&& upload_in_two(body, body_len, UPLOAD_LEN) && !memcmp(flash_written, body, UPLOAD_LEN);
+}
+
+static void scenario_upload_last_byte(void)
+{
+	uint8_t body[UPLOAD_LEN + sizeof(UPLOAD_TRAILER)];
+	uint16_t body_len = UPLOAD_LEN + sizeof(UPLOAD_TRAILER) - 1;
+	int bad = -1;
+
+	for (uint16_t i = 0; i < UPLOAD_LEN; i++)
+		body[i] = pattern(i);
+	memcpy(body + UPLOAD_LEN, UPLOAD_TRAILER, sizeof(UPLOAD_TRAILER) - 1);
+	for (int last = 0; last < 256 && bad < 0; last++) {
+		body[UPLOAD_LEN - 1] = last;
+		if (!upload_whole_and_split(body, body_len))
+			bad = last;
+	}
+	if (bad >= 0)
+		printf("      last byte 0x%02x loses the closing boundary\n", bad);
+	CHECK(bad < 0, "upload: the part ends after the payload whatever its last byte");
+
+	body[UPLOAD_LEN - 1] = '\r';
+	body[UPLOAD_LEN - 2] = '\r';
+	CHECK(upload_whole_and_split(body, body_len), "upload: an even run of CR before the boundary");
+	body[UPLOAD_LEN - 3] = '\r';
+	CHECK(upload_whole_and_split(body, body_len), "upload: an odd run of CR before the boundary");
+}
+
+static void scenario_upload_broken_matches(void)
+{
+	uint8_t body[UPLOAD_LEN + sizeof(UPLOAD_TRAILER)];
+	uint16_t body_len = UPLOAD_LEN + sizeof(UPLOAD_TRAILER) - 1;
+
+	memcpy(body + UPLOAD_LEN, UPLOAD_TRAILER, sizeof(UPLOAD_TRAILER) - 1);
+	memset(body, '\r', UPLOAD_LEN);
+	CHECK(upload_whole_and_split(body, body_len),
+	      "upload: a run of CR longer than a flash page is staged intact");
+	for (uint16_t i = 0; i < UPLOAD_LEN; i++)
+		body[i] = "\r\n-"[i % 3];
+	CHECK(upload_whole_and_split(body, body_len),
+	      "upload: repeated partial delimiters are staged as payload");
+}
+
 int main(int argc, char **argv)
 {
 	if (argc > 1 && !strcmp(argv[1], "-v"))
@@ -1187,6 +1296,9 @@ int main(int argc, char **argv)
 	scenario_upload("octet-stream control", "Content-Type: application/octet-stream\r\n", 0);
 	scenario_upload("MacBinary, split part terminator", "Content-Type: application/macbinary\r\n", 1);
 	scenario_upload("no part Content-Type", "", 0);
+	scenario_upload_cr_before_boundary();
+	scenario_upload_last_byte();
+	scenario_upload_broken_matches();
 
 	scenario_login_with_valid_cookie_reuses_its_slot();
 	scenario_login_without_cookie_replaces_least_recently_used();