CVE-2018-14681 (Other, published 2018-07-28T23:29Z)
Description: value: An issue was discovered in kwajd_read_headers in mspack/kwajd.c in libmspack before 0.7alpha. Bad KWAJ file header extensions could cause a one or two byte overwrite.
Severity: MEDIUM (CVSS v3 base score: 8.8)
Weakness: CWE-787 \u2014 Out-of-bounds Write
The product writes data past the end, or before the beginning, of the intended buffer.
Repository: https://github.com/kyz/libmspack
Files changed:
libmspack/ChangeLog
libmspack/Makefile.am
libmspack/mspack/kwajd.c
libmspack/test/.gitignore
libmspack/test/kwajd_test.c
libmspack/test/test_files/kwajd/make.pl
Commit message: kwaj_read_headers(): fix handling of non-terminated strings


Vulnerable code:
			test/chmd_order test/chminfo
	off_t pos = sys->tell(fh);
	char *fn = (char *) sys->alloc(sys, (size_t) 13);

	if (! fn) return MSPACK_ERR_NOMEMORY;
	hdr->filename = fn;
	    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))
	    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))

Fixed code:
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

			test/chmd_order test/chminfo test/kwajd_test
test_kwajd_test_SOURCES =	test/kwajd_test.c libmspack.la
test_kwajd_test_LDADD = 	libmspack.la
	int len;
	char *fn = (char *) sys->alloc(sys, (size_t) 13);
	if (!(hdr->filename = fn)) return MSPACK_ERR_NOMEMORY;
	    /* 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))
	    fn--; /* remove the null terminator */
	    /* 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))
	    fn--; /* remove the null terminator */
kwajd_test
/* 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;
}
#!/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;
    }
}