Update vpa108csulzh.cpp (CVE-2015-2278) - memset v array

[ret5et]

Hi, this is patch what fix reason of CVE-2015-2278. Commit 4182281 did not fix root of problem (CVE-2015-2278) - v array not initialized.

[martingalloar]

Hi ! Thanks for taking the time to submit the PR. I'll try to do some tests and port this also to the Wireshark plugin.

Do you think there's a way to trigger the issue before this patch but after https://github.com/CoreSecurity/pysap/commit/41822818ada94f476602c4a21891c9cdf17b0814? While initialization is the root cause and it would be good to fix it, I think https://github.com/CoreSecurity/pysap/commit/41822818ada94f476602c4a21891c9cdf17b0814 should suffice at preventing the issue to be triggered.

[ret5et]

I think 4182281 should suffice at preventing the issue to be triggered.

Maybe. But can we control the values of *p and s? In data from the test case which crashes the app the value of s is constant and equals to 257.

in CsObjectInt::BuildHufTree (this=0x7ffffffac480, b=0x7ffffffac554, n=280, s=257, d=0x7ffff58a8500 <cplens>, e=0x7ffff58a82c0 <CsExtraLenBits>, t=0x7ffffffbe5a8, m=0x7ffffffac540)
    at pysapcompress/vpa108csulzh.cpp:352

If we remove 4182281 patch and check value of p in the moment of Segmentation fault p contains uninitialized data. I think that patch 4182281 does not fix the root of the problem.

Do you think there's a way to trigger the issue before this patch but after 4182281?

p array depends on v which depends on x, etc. v -> x -> c -> b - > cshu.dd_ll

vpa108csulzh.cpp:809: cshu.dd_ll[border[j]] = (unsigned)cshu.bb & 7;

Even if we can control the value of * p, it wouldn't be that easy.

$ cat vpa108csulzh.cpp | grep BuildHufTree

...
    rc = BuildHufTree (l, 288, 257, cplens, cplext, &(cshu.tlitlen), &(cshu.blitlen));
    if ((rc = BuildHufTree (l, 30, 0, cpdist, cpdext, &(cshu.tdistcode), &(cshu.bdistlen))) < 0)
    if ((rc = BuildHufTree (cshu.dd_ll, 19, 19, NULL, NULL, &(cshu.dd_tl), &(cshu.dd_bl))) != 0)
    if ((rc = BuildHufTree (cshu.dd_ll, cshu.dd_nl, 257, cplens, cplext, &(cshu.dd_tl), &(cshu.dd_bl))) !=0)
    rc = BuildHufTree (cshu.dd_ll + cshu.dd_nl, cshu.dd_nd, 0, cpdist, cpdext, &(cshu.dd_td), &(cshu.dd_bd));

In every case that is showed above the value of s is constant and equals one of the following values: 257, 0, 19, 257, 0

P.S. Even if we do have a memory leak (e.g. we can read data from stack) - how we do get this data back?

[martingalloar]

If we remove 4182281 patch and check value of p in the moment of Segmentation fault p contains uninitialized data. I think that patch 4182281 does not fix the root of the problem.

Agree that 4182281 does not address root cause. However, I not sure there's an input that can make vpa108csulzh.cpp:345 or vpa108csulzh.cpp:346 to read out of the d and e arrays given that the fix checks that *p -s is within the array length.

Either way, it's a good improvement and I'll be merging this.

Even if we do have a memory leak (e.g. we can read data from stack) - how we do get this data back?

Yes, I don't think there's any chance to turn this more that into a out of bounds read and crash the program.

[martingalloar]

Merged this change, thanks !