[PATCHES] MSZIP support for cabinet files

[Jeremy Allison]

On Mon, Jul 03, 2017 at 07:11:07PM +0200, Aurélien Aptel via samba-technical wrote:
> After more careful reading I think code will fail early already, I've
> switched some variables to size_t for counts and offsets just in case.
> 
> In the pull function I've added more remaining byte checks for when we
> are raw-pulling CFDATAs.
> 
> I've also changed the magic 15 constant to MAX_WBITS, like it was
> changed in other code paths.
> 
> Feel free to squash if you think it's appropriate. I'm attaching the
> whole thing again with my last 3 commits appended.

Getting better. This part concerns me:

+                * similarly, we don't know the compressed
+                * size yet, remember offset, write zeros,
+                * fill out later
+                */
+               compressed_offset = ndr->offset;
+               NDR_CHECK(ndr_push_uint16(ndr, NDR_SCALARS, 0));
+               NDR_CHECK(ndr_push_uint16(ndr, NDR_SCALARS, r->cbUncomp));
+
+
+               switch (cab_ctype) {
+               case CF_COMPRESS_NONE:
+                       /* just copy the data */
+                       NDR_CHECK(ndr_push_bytes(ndr, r->ab.data, r->ab.length));
+                       compressed_length = r->cbUncomp;
+
+                       break;
+

We write a 16-bit value: r->cbUncomp, then write out
r->ab.length bytes and set compressed_length = r->cbUncomp - a totally
different (and client supplied I think) field.

What if they don't match ? The other case statement uses the
offset from struct ndr_push * after the push has been done.
Can we do the same in the uncompressed case too ? Or maybe
abort if r->cbUncomp != r->ab.length in the uncompressed
case (as they should be the same).

Inside:

+static enum ndr_err_code ndr_push_compression_mszip_cab_chunk(struct ndr_push *ndrpush,
+                                                             struct ndr_pull *ndrpull,
+                                                             struct ndr_compression_state *state)
+{
+       DATA_BLOB comp_chunk;
+       uint32_t comp_chunk_size;
+       DATA_BLOB plain_chunk;
+       uint32_t plain_chunk_size;
+       uint32_t plain_chunk_offset;
+       uint32_t max_plain_size = 0x00008000;
+       /*
+        * The maximum compressed size of each MSZIP block is 32k + 12 bytes
+        * header size.
+        */
+       uint32_t max_comp_size = 0x00008000 + 12;
+       int z_ret;
+       z_stream *z;
+
+       plain_chunk_size = MIN(max_plain_size, ndrpull->data_size - ndrpull->offset);

Can you add a check that 'ndrpull->data_size > ndrpull->offset'.

I know this will never happen, but I really hate seeing raw
unchecked arithmetic on values in marshalling/unmarshalling code.
It makes me nervous :-). I need to feel calm when reading this
code :-).

Also here:

+	comp_chunk_size = 2 + z->total_out;

Jeremy.