kwaj_read_headers(): fix handling of non-terminated strings

libmspack/ChangeLog

@@ -1,3 +1,11 @@
+2017-11-26  Stuart Caie <kyzer@cabextract.org.uk>
+
+	* kwajd_read_headers(): fix up the logic of reading the filename and
+	extension headers to avoid a one or two byte overwrite. Thanks to
+	Jakub Wilk for finding the issue.
+
+	* test/kwajd_test.c: add tests for KWAJ filename.ext handling
+
 2017-10-16  Stuart Caie <kyzer@cabextract.org.uk>
 
 	* test/cabd_test.c: update the short string tests to expect not only

libmspack/Makefile.am

@@ -24,7 +24,7 @@ lib_LTLIBRARIES =	libmspack.la
 noinst_LTLIBRARIES =	libmscabd.la libmschmd.la
 noinst_PROGRAMS =	examples/cabd_memory examples/multifh test/cabd_md5 \
 			test/cabd_test test/chmd_find test/chmd_md5 \
-			test/chmd_order test/chminfo
+			test/chmd_order test/chminfo test/kwajd_test
 
 libmspack_la_SOURCES =	mspack/mspack.h \
 			mspack/system.h mspack/system.c \
@@ -89,3 +89,5 @@ test_chmd_order_SOURCES =	test/chmd_order.c test/md5.c test/md5.h \
 test_chmd_order_LDADD =		libmschmd.la
 test_chminfo_SOURCES =		test/chminfo.c libmschmd.la
 test_chminfo_LDADD =		libmschmd.la
+test_kwajd_test_SOURCES =	test/kwajd_test.c libmspack.la
+test_kwajd_test_LDADD = 	libmspack.la

libmspack/mspack/kwajd.c

@@ -198,30 +198,36 @@ static int kwajd_read_headers(struct mspack_system *sys,
 
     /* filename and extension */
     if (hdr->headers & (MSKWAJ_HDR_HASFILENAME | MSKWAJ_HDR_HASFILEEXT)) {
-	off_t pos = sys->tell(fh);
-	char *fn = (char *) sys->alloc(sys, (size_t) 13);
-
+	int len;
 	/* allocate memory for maximum length filename */
-	if (! fn) return MSPACK_ERR_NOMEMORY;
-	hdr->filename = fn;
+	char *fn = (char *) sys->alloc(sys, (size_t) 13);
+	if (!(hdr->filename = fn)) return MSPACK_ERR_NOMEMORY;
 
 	/* copy filename if present */
 	if (hdr->headers & MSKWAJ_HDR_HASFILENAME) {
-	    if (sys->read(fh, &buf[0], 9) != 9) return MSPACK_ERR_READ;
-	    for (i = 0; i < 9; i++, fn++) if (!(*fn = buf[i])) break;
-	    pos += (i < 9) ? i+1 : 9;
-	    if (sys->seek(fh, pos, MSPACK_SYS_SEEK_START))
+	    /* read and copy up to 9 bytes of a null terminated string */
+	    if ((len = sys->read(fh, &buf[0], 9)) < 2) return MSPACK_ERR_READ;
+	    for (i = 0; i < len; i++) if (!(*fn++ = buf[i])) break;
+	    /* if string was 9 bytes with no null terminator, reject it */
+	    if (i == 9 && buf[8] != '\0') return MSPACK_ERR_DATAFORMAT;
+	    /* seek to byte after string ended in file */
+	    if (sys->seek(fh, (off_t)(i + 1 - len), MSPACK_SYS_SEEK_CUR))
 		return MSPACK_ERR_SEEK;
+	    fn--; /* remove the null terminator */
 	}
 
 	/* copy extension if present */
 	if (hdr->headers & MSKWAJ_HDR_HASFILEEXT) {
 	    *fn++ = '.';
-	    if (sys->read(fh, &buf[0], 4) != 4) return MSPACK_ERR_READ;
-	    for (i = 0; i < 4; i++, fn++) if (!(*fn = buf[i])) break;
-	    pos += (i < 4) ? i+1 : 4;
-	    if (sys->seek(fh, pos, MSPACK_SYS_SEEK_START))
+	    /* read and copy up to 4 bytes of a null terminated string */
+	    if ((len = sys->read(fh, &buf[0], 4)) < 2) return MSPACK_ERR_READ;
+	    for (i = 0; i < len; i++) if (!(*fn++ = buf[i])) break;
+	    /* if string was 4 bytes with no null terminator, reject it */
+	    if (i == 4 && buf[3] != '\0') return MSPACK_ERR_DATAFORMAT;
+	    /* seek to byte after string ended in file */
+	    if (sys->seek(fh, (off_t)(i + 1 - len), MSPACK_SYS_SEEK_CUR))
 		return MSPACK_ERR_SEEK;
+	    fn--; /* remove the null terminator */
 	}
 	*fn = '\0';
     }

libmspack/test/.gitignore

@@ -8,3 +8,4 @@ chmd_find
 chmd_md5
 chmd_order
 chminfo
+kwajd_test

libmspack/test/kwajd_test.c

@@ -0,0 +1,116 @@
+/* KWAJ regression test suite */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <mspack.h>
+
+unsigned int test_count = 0;
+#define TEST(x) do {\
+    test_count++; \
+    if (!(x)) {printf("%s:%d FAILED %s\n",__FUNCTION__,__LINE__,#x);exit(1);} \
+} while (0)
+
+/* test parsing of KWAJ filename/extension headers */
+void kwajd_open_test_01() {
+    struct mskwaj_decompressor *kwajd;
+    struct mskwajd_header *hdr;
+
+    kwajd = mspack_create_kwaj_decompressor(NULL);
+    TEST(kwajd != NULL);
+
+    hdr = kwajd->open(kwajd, "test_files/kwajd/f00.kwj");
+    TEST(hdr != NULL);
+    TEST(hdr->filename == NULL);
+    kwajd->close(kwajd, hdr);
+
+#define TEST_FNAME(testfile, fname)      \
+    hdr = kwajd->open(kwajd, testfile);  \
+    TEST(hdr != NULL);                   \
+    TEST(hdr->filename != NULL);         \
+    TEST(!strcmp(fname, hdr->filename)); \
+    kwajd->close(kwajd, hdr)
+#define TEST_FNAME_BAD(testfile)         \
+    hdr = kwajd->open(kwajd, testfile);  \
+    TEST(hdr == NULL);                   \
+    TEST(kwajd->last_error(kwajd) == MSPACK_ERR_DATAFORMAT)
+
+    TEST_FNAME("test_files/kwajd/f01.kwj", ".1");
+    TEST_FNAME("test_files/kwajd/f02.kwj", ".12");
+    TEST_FNAME("test_files/kwajd/f03.kwj", ".123");
+
+    TEST_FNAME("test_files/kwajd/f10.kwj", "1");
+    TEST_FNAME("test_files/kwajd/f11.kwj", "1.1");
+    TEST_FNAME("test_files/kwajd/f12.kwj", "1.12");
+    TEST_FNAME("test_files/kwajd/f13.kwj", "1.123");
+
+    TEST_FNAME("test_files/kwajd/f20.kwj", "12");
+    TEST_FNAME("test_files/kwajd/f21.kwj", "12.1");
+    TEST_FNAME("test_files/kwajd/f22.kwj", "12.12");
+    TEST_FNAME("test_files/kwajd/f23.kwj", "12.123");
+
+    TEST_FNAME("test_files/kwajd/f30.kwj", "123");
+    TEST_FNAME("test_files/kwajd/f31.kwj", "123.1");
+    TEST_FNAME("test_files/kwajd/f32.kwj", "123.12");
+    TEST_FNAME("test_files/kwajd/f33.kwj", "123.123");
+
+    TEST_FNAME("test_files/kwajd/f40.kwj", "1234");
+    TEST_FNAME("test_files/kwajd/f41.kwj", "1234.1");
+    TEST_FNAME("test_files/kwajd/f42.kwj", "1234.12");
+    TEST_FNAME("test_files/kwajd/f43.kwj", "1234.123");
+
+    TEST_FNAME("test_files/kwajd/f50.kwj", "12345");
+    TEST_FNAME("test_files/kwajd/f51.kwj", "12345.1");
+    TEST_FNAME("test_files/kwajd/f52.kwj", "12345.12");
+    TEST_FNAME("test_files/kwajd/f53.kwj", "12345.123");
+
+    TEST_FNAME("test_files/kwajd/f60.kwj", "123456");
+    TEST_FNAME("test_files/kwajd/f61.kwj", "123456.1");
+    TEST_FNAME("test_files/kwajd/f62.kwj", "123456.12");
+    TEST_FNAME("test_files/kwajd/f63.kwj", "123456.123");
+
+    TEST_FNAME("test_files/kwajd/f70.kwj", "1234567");
+    TEST_FNAME("test_files/kwajd/f71.kwj", "1234567.1");
+    TEST_FNAME("test_files/kwajd/f72.kwj", "1234567.12");
+    TEST_FNAME("test_files/kwajd/f73.kwj", "1234567.123");
+
+    TEST_FNAME("test_files/kwajd/f80.kwj", "12345678");
+    TEST_FNAME("test_files/kwajd/f81.kwj", "12345678.1");
+    TEST_FNAME("test_files/kwajd/f82.kwj", "12345678.12");
+    TEST_FNAME("test_files/kwajd/f83.kwj", "12345678.123");
+
+    TEST_FNAME_BAD("test_files/kwajd/f04.kwj");
+    TEST_FNAME_BAD("test_files/kwajd/f14.kwj");
+    TEST_FNAME_BAD("test_files/kwajd/f24.kwj");
+    TEST_FNAME_BAD("test_files/kwajd/f34.kwj");
+    TEST_FNAME_BAD("test_files/kwajd/f44.kwj");
+    TEST_FNAME_BAD("test_files/kwajd/f54.kwj");
+    TEST_FNAME_BAD("test_files/kwajd/f64.kwj");
+    TEST_FNAME_BAD("test_files/kwajd/f74.kwj");
+    TEST_FNAME_BAD("test_files/kwajd/f84.kwj");
+
+    TEST_FNAME_BAD("test_files/kwajd/f90.kwj");
+    TEST_FNAME_BAD("test_files/kwajd/f91.kwj");
+    TEST_FNAME_BAD("test_files/kwajd/f92.kwj");
+    TEST_FNAME_BAD("test_files/kwajd/f93.kwj");
+    TEST_FNAME_BAD("test_files/kwajd/f94.kwj");
+
+
+    mspack_destroy_kwaj_decompressor(kwajd);
+}
+
+int main() {
+  int selftest;
+
+  MSPACK_SYS_SELFTEST(selftest);
+  TEST(selftest == MSPACK_ERR_OK);
+
+  kwajd_open_test_01();
+
+  printf("ALL %d TESTS PASSED.\n", test_count);
+  return 0;
+}

libmspack/test/test_files/kwajd/make.pl

@@ -0,0 +1,17 @@
+#!/usr/bin/perl -w
+use strict;
+my $name = '123456789';
+for my $file (0 .. 9) {
+    for my $ext (0 .. 4) {
+	open my $fh, '>', "f$file$ext.kwj";
+        my $offset = 14  + $file + $ext;
+	my $flags  = ($file > 0 ? 8 : 0) | ($ext > 0 ? 16 : 0);
+	print $fh pack 'A4Vvvv', 'KWAJ', 0xD127F088, 0, $offset, $flags;
+	print $fh substr $name, 0, $file if $file > 0;
+	print $fh "\0" if $file > 0 && $file < 9;
+	print $fh substr $name, 0, $ext if $ext > 0;
+	print $fh "\0" if $ext > 0 && $ext < 4;
+        print $fh "\xFF";
+	close $fh;
+    }
+}