[PATCHES] MSZIP support for cabinet files

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
Hi,

attached some patches (mostly from Aurelien) that add MSZIP compression
support to our cabinet file marshalling code. This work is one of the
prerequisites for the future MS-PAR protocol support.

Please review & push.

Thanks,
Guenther
--
Günther Deschner                    GPG-ID: 8EE11688
Red Hat                         [hidden email]
Samba Team                              [hidden email]

cabinet_mszip_support.patch (45K) Download Attachment
signature.asc (208 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
On Tue, Jun 27, 2017 at 05:59:49PM +0200, Günther Deschner via samba-technical wrote:
> Hi,
>
> attached some patches (mostly from Aurelien) that add MSZIP compression
> support to our cabinet file marshalling code. This work is one of the
> prerequisites for the future MS-PAR protocol support.
>
> Please review & push.

I'm a bit scared of integer wrap (see some below). Can you
convince me it's safe ?

:-) :-).

Cheers,

        Jeremy.

> +static enum ndr_err_code ndr_push_folder_cfdata(struct ndr_push *ndr,
> + const struct CFDATA *r,
> + enum cf_compress_type cab_ctype,
> + size_t num_cfdata)
> +{
> + size_t i;
> + enum ndr_compression_alg ndr_ctype = 0;
> +
> + ndr_set_flags(&ndr->flags, LIBNDR_PRINT_ARRAY_HEX|LIBNDR_FLAG_LITTLE_ENDIAN|LIBNDR_FLAG_NOALIGN);
> +
> + if (cab_ctype == CF_COMPRESS_MSZIP) {
> + ndr_ctype = NDR_COMPRESSION_MSZIP_CAB;
> + NDR_CHECK(ndr_push_compression_state_init(ndr, ndr_ctype, &ndr->cstate));
> + }
> +
> + for (i = 0; i < num_cfdata; i++, r++) {
> + uint32_t compressed_offset, compressed_length = 0;
> + uint32_t csum, csum_offset;
> + uint32_t csumPartial;
> +
> + /*
> + * checksum is a function of the size fields
> + * and the potentially compressed data bytes,
> + * which haven't been compressed yet so
> + * remember offset, write zeroes, fill out
> + * later
> + */
> + csum_offset = ndr->offset;
> + NDR_CHECK(ndr_push_uint32(ndr, NDR_SCALARS, 0));
> +
> + /*
> + * 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;
> +
> + case CF_COMPRESS_LZX:
> + /*
> + * we have not yet worked out the details of LZX
> + * compression
> + */
> + return NDR_ERR_COMPRESSION;
> +
> + case CF_COMPRESS_MSZIP: {
> + struct ndr_push *push_sub, *push_compress;
> +
> + /* compress via subcontext */
> + NDR_CHECK(ndr_push_subcontext_start(ndr, &push_sub, 0, -1));
> + push_sub->cstate = ndr->cstate;
> + NDR_CHECK(ndr_push_compression_start(push_sub, &push_compress, ndr_ctype, -1));
> + ndr_set_flags(&push_compress->flags, LIBNDR_FLAG_REMAINING);
> + NDR_CHECK(ndr_push_DATA_BLOB(push_compress, NDR_SCALARS, r->ab));
> + NDR_CHECK(ndr_push_compression_end(push_sub, push_compress, ndr_ctype, -1));
> + NDR_CHECK(ndr_push_subcontext_end(ndr, push_sub, 0, -1));
> + compressed_length = push_sub->offset;
> +
> + break;
> + }
> + default:
> + return NDR_ERR_BAD_SWITCH;
> + }
> +
> + /* we can now write the compressed size and the checksum */
> + SSVAL(ndr->data, compressed_offset, compressed_length);
> +
> + /*
> + * Checksum over compressed data.
> + *
> + * The 8 bytes are the header size.
> + */
> +
> + csumPartial = ndr_cab_compute_checksum(&ndr->data[csum_offset + 8],

                                                                 ^^^^^^^^^^^^^^^^
                                                                 Integer wrap checks ?
Didn't cnum_offset come from the client ?

> +       compressed_length, 0);
> +
> + /*
> + * Checksum over header (compressed and uncompressed length).
> + *
> + * The first 4 bytes are the checksum size.
> + * The second 4 bytes are the size of the compressed and
> + * uncompressed length fields.
> + */
> + csum = ndr_cab_compute_checksum(&ndr->data[csum_offset + 4],
                                                                 ^^^^^^^^^^^^^^^^
                                                                 Integer wrap checks ?
And here also ?

> + 4,
> + csumPartial);
> +
> + SIVAL(ndr->data, csum_offset, csum);
> + }
> +
> + ndr_push_compression_state_free(ndr->cstate);
> + ndr->cstate = NULL;
> +
> + return NDR_ERR_SUCCESS;
> +}
> +
>  _PUBLIC_ enum ndr_err_code ndr_push_cab_file(struct ndr_push *ndr, int ndr_flags, const struct cab_file *r)
>  {
>   uint32_t cntr_cffolders_0;
>   uint32_t cntr_cffiles_0;
> - uint32_t cntr_cfdata_0;
> + uint32_t processed_cfdata = 0;
>   {
>   uint32_t _flags_save_STRUCT = ndr->flags;
>   ndr_set_flags(&ndr->flags, LIBNDR_PRINT_ARRAY_HEX|LIBNDR_FLAG_LITTLE_ENDIAN|LIBNDR_FLAG_NOALIGN);
>   NDR_PUSH_CHECK_FLAGS(ndr, ndr_flags);
> +
>   if (ndr_flags & NDR_SCALARS) {
> - uint32_t next_offset = 0;
> + uint32_t i;
>   NDR_CHECK(ndr_push_align(ndr, 4));
>   NDR_CHECK(ndr_push_CFHEADER(ndr, NDR_SCALARS, &r->cfheader));
>   for (cntr_cffolders_0 = 0; cntr_cffolders_0 < (r->cfheader.cFolders); cntr_cffolders_0++) {
>   NDR_CHECK(ndr_push_CFFOLDER(ndr, NDR_SCALARS, &r->cffolders[cntr_cffolders_0]));
>   }
>   for (cntr_cffiles_0 = 0; cntr_cffiles_0 < (r->cfheader.cFiles); cntr_cffiles_0++) {
> - uint32_t offset = ndr->offset + 4;
>   NDR_CHECK(ndr_push_CFFILE(ndr, NDR_SCALARS, &r->cffiles[cntr_cffiles_0]));
> - if (cntr_cffiles_0 > 0) {
> - next_offset += r->cffiles[cntr_cffiles_0 - 1].cbFile;
> - }
> - SIVAL(ndr->data, offset, next_offset);
>   }
>  #if 0
>   NDR_CHECK(ndr_push_uint32(ndr, NDR_SCALARS, ndr_count_cfdata(r)));
>  #endif
> - for (cntr_cfdata_0 = 0; cntr_cfdata_0 < (ndr_count_cfdata(r)); cntr_cfdata_0++) {
> - NDR_CHECK(ndr_push_CFDATA(ndr, NDR_SCALARS, &r->cfdata[cntr_cfdata_0]));
> +
> + /* write in the folder header the offset of its first data block */
> + for (i = 0; i < r->cfheader.cFolders; i++) {
> + SIVAL(ndr->data, OFFSET_OF_FOLDER_COFFCABSTART(i), ndr->offset);
> + NDR_CHECK(ndr_push_folder_cfdata(ndr, r->cfdata + processed_cfdata, r->cffolders[i].typeCompress, r->cffolders[i].cCFData));
> + processed_cfdata += r->cffolders[i].cCFData;
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                Here also ?

>   }
>   NDR_CHECK(ndr_push_trailer_align(ndr, 4));
>   }
> @@ -160,6 +278,79 @@ _PUBLIC_ enum ndr_err_code ndr_push_cab_file(struct ndr_push *ndr, int ndr_flags
>   return NDR_ERR_SUCCESS;
>  }
>  
> +
> +/* Pull all CFDATA of a folder.
> + *
> + * This works on a folder level because compression type is set per
> + * folder, and a compression state can be shared between CFDATA of the
> + * same folder.
> + *
> + * This is not a regular NDR func as we pass the compression type and
> + * the number of CFDATA as extra arguments
> + */
> +static enum ndr_err_code ndr_pull_folder_cfdata(struct ndr_pull *ndr,
> + struct CFDATA *r,
> + enum cf_compress_type cab_ctype,
> + size_t num_cfdata)
> +{
> + size_t i;
> + enum ndr_compression_alg ndr_ctype = 0;
> +
> + if (cab_ctype == CF_COMPRESS_MSZIP) {
> + ndr_ctype = NDR_COMPRESSION_MSZIP_CAB;
> + NDR_CHECK(ndr_pull_compression_state_init(ndr, NDR_COMPRESSION_MSZIP_CAB, &ndr->cstate));
> + }
> +
> + for (i = 0; i < num_cfdata; i++, r++) {
> + NDR_CHECK(ndr_pull_uint32(ndr, NDR_SCALARS, &r->csum));
> + NDR_CHECK(ndr_pull_uint16(ndr, NDR_SCALARS, &r->cbData));
> + NDR_CHECK(ndr_pull_uint16(ndr, NDR_SCALARS, &r->cbUncomp));
> +
> + switch (cab_ctype) {
> + case CF_COMPRESS_NONE:
> + /* just copy the data */
> + r->ab = data_blob_talloc(ndr->current_mem_ctx,
> + ndr->data+ndr->offset,
> + r->cbUncomp);
> + ndr->offset += r->cbUncomp;
> + break;
> +
> + case CF_COMPRESS_LZX:
> + /* just copy the data */
> + r->ab = data_blob_talloc(ndr->current_mem_ctx,
> + ndr->data+ndr->offset,
> + r->cbData);
> + ndr->offset += r->cbData;
> +
> + break;
> +
> + case CF_COMPRESS_MSZIP: {
> + struct ndr_pull *pull_sub, *pull_compress;
> +
> + /* decompress via subcontext */
> + NDR_CHECK(ndr_pull_subcontext_start(ndr, &pull_sub, 0, r->cbData));
> + pull_sub->cstate = ndr->cstate;
> + NDR_CHECK(ndr_pull_compression_start(pull_sub, &pull_compress,
> +     ndr_ctype, r->cbUncomp, r->cbData));
> + ndr_set_flags(&pull_compress->flags, LIBNDR_FLAG_REMAINING);
> + NDR_CHECK(ndr_pull_DATA_BLOB(pull_compress, NDR_SCALARS, &r->ab));
> + NDR_CHECK(ndr_pull_compression_end(pull_sub, pull_compress, ndr_ctype, r->cbUncomp));
> + NDR_CHECK(ndr_pull_subcontext_end(ndr, pull_sub, 0, r->cbData));
> +
> + break;
> + }
> + default:
> + return NDR_ERR_BAD_SWITCH;
> + }
> + }
> +
> + ndr_pull_compression_state_free(ndr->cstate);
> + ndr->cstate = NULL;
> +
> + return NDR_ERR_SUCCESS;
> +}
> +
>  _PUBLIC_ enum ndr_err_code ndr_pull_cab_file(struct ndr_pull *ndr, int ndr_flags, struct cab_file *r)
>  {
>   uint32_t size_cffolders_0 = 0;
> @@ -169,7 +360,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_cab_file(struct ndr_pull *ndr, int ndr_flags
>   uint32_t cntr_cffiles_0;
>   TALLOC_CTX *_mem_save_cffiles_0 = NULL;
>   uint32_t size_cfdata_0 = 0;
> - uint32_t cntr_cfdata_0;
> + uint32_t processed_cfdata = 0;
>   TALLOC_CTX *_mem_save_cfdata_0 = NULL;
>   {
>   uint32_t _flags_save_STRUCT = ndr->flags;
> @@ -203,8 +394,12 @@ _PUBLIC_ enum ndr_err_code ndr_pull_cab_file(struct ndr_pull *ndr, int ndr_flags
>   NDR_PULL_ALLOC_N(ndr, r->cfdata, size_cfdata_0);
>   _mem_save_cfdata_0 = NDR_PULL_GET_MEM_CTX(ndr);
>   NDR_PULL_SET_MEM_CTX(ndr, r->cfdata, 0);
> - for (cntr_cfdata_0 = 0; cntr_cfdata_0 < (size_cfdata_0); cntr_cfdata_0++) {
> - NDR_CHECK(ndr_pull_CFDATA(ndr, NDR_SCALARS, &r->cfdata[cntr_cfdata_0]));
> + for (cntr_cffolders_0 = 0; cntr_cffolders_0 < (size_cffolders_0); cntr_cffolders_0++) {
> + NDR_CHECK(ndr_pull_folder_cfdata(ndr,
> + r->cfdata + processed_cfdata,
> + r->cffolders[cntr_cffolders_0].typeCompress,
> + r->cffolders[cntr_cffolders_0].cCFData));
> + processed_cfdata += r->cffolders[cntr_cffolders_0].cCFData;
>   }
>   NDR_PULL_SET_MEM_CTX(ndr, _mem_save_cfdata_0, 0);
>   NDR_CHECK(ndr_pull_trailer_align(ndr, 4));
> diff --git a/librpc/ndr/ndr_compression.c b/librpc/ndr/ndr_compression.c
> index 5a821d5..54a6262 100644
> --- a/librpc/ndr/ndr_compression.c
> +++ b/librpc/ndr/ndr_compression.c
> @@ -47,6 +47,251 @@ static void  ndr_zlib_free(voidpf opaque, voidpf address)
>   talloc_free(address);
>  }
>  
> +static enum ndr_err_code ndr_pull_compression_mszip_cab_chunk(struct ndr_pull *ndrpull,
> +      struct ndr_push *ndrpush,
> +      struct ndr_compression_state *state,
> +      ssize_t decompressed_len,
> +      ssize_t compressed_len)
> +{
> + DATA_BLOB comp_chunk;
> + uint32_t comp_chunk_offset;
> + uint32_t comp_chunk_size;
> + DATA_BLOB plain_chunk;
> + uint32_t plain_chunk_offset;
> + uint32_t plain_chunk_size;
> + z_stream *z = state->mszip.z;
> + int z_ret;
> +
> + plain_chunk_size = decompressed_len;
> +
> + if (plain_chunk_size > 0x00008000) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "Bad MSZIP CAB plain chunk size %08X > 0x00008000 (PULL)",
> +      plain_chunk_size);
> + }
> +
> +
> + comp_chunk_size = compressed_len;
> +
> + DEBUG(9,("MSZIP CAB plain_chunk_size: %08X (%u) comp_chunk_size: %08X (%u)\n",
> + plain_chunk_size, plain_chunk_size, comp_chunk_size, comp_chunk_size));
> +
> + comp_chunk_offset = ndrpull->offset;
> + NDR_CHECK(ndr_pull_advance(ndrpull, comp_chunk_size));
> + comp_chunk.length = comp_chunk_size;
> + comp_chunk.data = ndrpull->data + comp_chunk_offset;
> +
> + plain_chunk_offset = ndrpush->offset;
> + NDR_CHECK(ndr_push_zero(ndrpush, plain_chunk_size));
> + plain_chunk.length = plain_chunk_size;
> + plain_chunk.data = ndrpush->data + plain_chunk_offset;
> +
> + if (comp_chunk.length < 2) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "Bad MSZIP CAB comp chunk size %u < 2 (PULL)",
> +      (unsigned int)comp_chunk.length);
> + }
> + /* CK = Chris Kirmse, official Microsoft purloiner */
> + if (comp_chunk.data[0] != 'C' ||
> +    comp_chunk.data[1] != 'K') {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "Bad MSZIP CAB invalid prefix [%c%c] != [CK]",
> +      comp_chunk.data[0], comp_chunk.data[1]);
> + }
> +
> + /*
> + * This is a MSZIP block. It is actually using the deflate
> + * algorithm which can be decompressed by zlib. zlib will try
> + * to decompress as much as it can in each run. If we provide
> + * all the input and enough room for the uncompressed output,
> + * one call is enough. It will loop over all the sub-blocks
> + * that make up a deflate block.
> + *
> + * See corresponding push function for more info.
> + */
> +
> + z->next_in = comp_chunk.data + 2;
> + z->avail_in = comp_chunk.length - 2;
> + z->next_out = plain_chunk.data;
> + z->avail_out = plain_chunk.length;
> +
> + /*
> + * Each MSZIP CDATA contains a complete deflate stream
> + * i.e. the stream starts and ends in the CFDATA but the
> + * _dictionnary_ is shared between all CFDATA of a CFFOLDER.
> + *
> + * When decompressing, the initial dictionnary of the first
> + * CDATA is empty. All other CFDATA use the previous CFDATA
> + * uncompressed output as dictionnary.
> + */
> +
> + if (state->mszip.dict_size) {
> + z_ret = inflateSetDictionary(z, state->mszip.dict, state->mszip.dict_size);
> + if (z_ret != Z_OK) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib inflateSetDictionary error %s (%d) %s (PULL)",
> +      zError(z_ret), z_ret, z->msg);
> + }
> + }
> +
> + z_ret = inflate(z, Z_FINISH);
> + if (z_ret == Z_OK) {
> + /*
> + * Z_OK here means there was no error but the stream
> + * hasn't been fully decompressed because there was
> + * not enough room for the output, which should not
> + * happen
> + */
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib inflate error not enough space for ouput (PULL)");
> + }
> + if (z_ret != Z_STREAM_END) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib inflate error %s (%d) %s (PULL)", zError(z_ret), z_ret, z->msg);
> + }
> +
> + if (z->total_out < plain_chunk.length) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib uncompressed output is smaller than expected (%lu < %zu) (PULL)",
> +      z->total_out, plain_chunk.length);
> + }
> +
> + /*
> + * Keep a copy of the output to set as dictionnary for the
> + * next decompression call.
> + *
> + * The input pointer seems to be still valid between calls, so
> + * we can just store that instead of copying the memory over
> + * the dict temp buffer.
> + */
> + state->mszip.dict = plain_chunk.data;
> + state->mszip.dict_size = plain_chunk.length;
> +
> + z_ret = inflateReset(z);
> + if (z_ret != Z_OK) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib inflateReset error %s (%d) %s (PULL)",
> +      zError(z_ret), z_ret, z->msg);
> + }
> +
> + return NDR_ERR_SUCCESS;
> +}
> +
> +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);
> + plain_chunk_offset = ndrpull->offset;
> + NDR_CHECK(ndr_pull_advance(ndrpull, plain_chunk_size));
> +
> + plain_chunk.data = ndrpull->data + plain_chunk_offset;
> + plain_chunk.length = plain_chunk_size;
> +
> + NDR_CHECK(ndr_push_expand(ndrpush, max_comp_size));
> +
> + comp_chunk.data = ndrpush->data + ndrpush->offset;
> + comp_chunk.length = max_comp_size;
> +
> + /* CK = Chris Kirmse, official Microsoft purloiner */
> + comp_chunk.data[0] = 'C';
> + comp_chunk.data[1] = 'K';
> +
> + z = state->mszip.z;
> + z->next_in = plain_chunk.data;
> + z->avail_in = plain_chunk.length;
> + z->total_in = 0;
> +
> + z->next_out = comp_chunk.data + 2;
> + z->avail_out = comp_chunk.length;
> + z->total_out = 0;
> +
> + /*
> + * See pull function for explanations of the MSZIP format.
> + *
> + * The CFDATA block contains a full deflate stream. Each stream
> + * uses the uncompressed input of the previous CFDATA in the
> + * same CFFOLDER as a dictionnary for the compression.
> + */
> +
> + if (state->mszip.dict_size) {
> + z_ret = deflateSetDictionary(z, state->mszip.dict, state->mszip.dict_size);
> + if (z_ret != Z_OK) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib deflateSetDictionary error %s (%d) %s (PUSH)",
> +      zError(z_ret), z_ret, z->msg);
> + }
> + }
> +
> + /*
> + * Z_FINISH should make deflate process all of the input in
> + * one call. If the stream is not finished there was an error
> + * e.g. not enough room to store the compressed output.
> + */
> + z_ret = deflate(z, Z_FINISH);
> + if (z_ret != Z_STREAM_END) {
> + return ndr_push_error(ndrpush, NDR_ERR_COMPRESSION,
> +      "zlib deflate error %s (%d) %s (PUSH)",
> +      zError(z_ret), z_ret, z->msg);
> + }
> +
> + if (z->avail_in) {
> + return ndr_push_error(ndrpush, NDR_ERR_COMPRESSION,
> +      "MSZIP not all avail_in[%u] bytes consumed (PUSH)",
> +      z->avail_in);
> + }
> +
> + comp_chunk_size = 2 + z->total_out;
> +
> + z_ret = deflateReset(z);
> + if (z_ret != Z_OK) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib deflateReset error %s (%d) %s (PUSH)",
> +      zError(z_ret), z_ret, z->msg);
> + }
> +
> + if (plain_chunk.length > talloc_array_length(state->mszip.dict)) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib dict buffer is too big (PUSH)");
> + }
> +
> + /*
> + * Keep a copy of the input to set as dictionnary for the next
> + * compression call.
> + *
> + * Ideally we would just store the input pointer and length
> + * without copying but the memory gets invalidated between the
> + * calls, so we just copy to a dedicated buffer we now is
> + * still going to been valid for the lifetime of the
> + * compressions state object.
> + */
> + memcpy(state->mszip.dict, plain_chunk.data, plain_chunk.length);
> + state->mszip.dict_size = plain_chunk.length;
> +
> + DEBUG(9,("MSZIP comp plain_chunk_size: %08X (%u) comp_chunk_size: %08X (%u)\n",
> + (unsigned int)plain_chunk.length,
> + (unsigned int)plain_chunk.length,
> + comp_chunk_size, comp_chunk_size));
> +
> + ndrpush->offset += comp_chunk_size;
> + return NDR_ERR_SUCCESS;
> +}
> +
> +
>  static enum ndr_err_code ndr_pull_compression_mszip_chunk(struct ndr_pull *ndrpull,
>   struct ndr_push *ndrpush,
>   z_stream *z,
> @@ -411,6 +656,12 @@ enum ndr_err_code ndr_pull_compression_start(struct ndr_pull *subndr,
>   NDR_ERR_HAVE_NO_MEMORY(ndrpush);
>  
>   switch (compression_alg) {
> + case NDR_COMPRESSION_MSZIP_CAB:
> + NDR_CHECK(ndr_pull_compression_mszip_cab_chunk(subndr, ndrpush,
> +       subndr->cstate,
> +       decompressed_len,
> +       compressed_len));
> + break;
>   case NDR_COMPRESSION_MSZIP:
>   ZERO_STRUCT(z);
>   while (!last) {
> @@ -470,6 +721,7 @@ enum ndr_err_code ndr_push_compression_start(struct ndr_push *subndr,
>   struct ndr_push *uncomndr;
>  
>   switch (compression_alg) {
> + case NDR_COMPRESSION_MSZIP_CAB:
>   case NDR_COMPRESSION_MSZIP:
>   case NDR_COMPRESSION_XPRESS:
>   break;
> @@ -507,6 +759,10 @@ enum ndr_err_code ndr_push_compression_end(struct ndr_push *subndr,
>   ndrpull->offset = 0;
>  
>   switch (compression_alg) {
> + case NDR_COMPRESSION_MSZIP_CAB:
> + NDR_CHECK(ndr_push_compression_mszip_cab_chunk(subndr, ndrpull, subndr->cstate));
> + break;
> +
>   case NDR_COMPRESSION_MSZIP:
>   ZERO_STRUCT(z);
>   while (!last) {
> diff --git a/source4/torture/ndr/cabinet.c b/source4/torture/ndr/cabinet.c
> index 790f7c3..f197534 100644
> --- a/source4/torture/ndr/cabinet.c
> +++ b/source4/torture/ndr/cabinet.c
> @@ -4233,10 +4233,9 @@ static bool cab_file_MSZIP_check(struct torture_context *tctx,
>  
>   blob = data_blob(NULL, r->cfdata[0].cbUncomp);
>   memset(blob.data, 'A', blob.length);
> -#if 0
> - /* once we have MSZIP compression working we can enable this test */
> +
>   torture_assert_data_blob_equal(tctx, r->cfdata[0].ab, blob, "ab");
> -#endif
> +
>   return true;
>  }
>  
> --
> 2.9.4
>
>
> From 9784f4e411eed188d0c3daf91bde90843f2bc6b3 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?G=C3=BCnther=20Deschner?= <[hidden email]>
> Date: Tue, 23 May 2017 15:50:55 +0200
> Subject: [PATCH 13/13] s4-torture: point out why we cannot validate MSZIP
>  compressed files
>
> Guenther
>
> Signed-off-by: Guenther Deschner <[hidden email]>
> ---
>  source4/torture/ndr/cabinet.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/source4/torture/ndr/cabinet.c b/source4/torture/ndr/cabinet.c
> index f197534..8cdf4cb 100644
> --- a/source4/torture/ndr/cabinet.c
> +++ b/source4/torture/ndr/cabinet.c
> @@ -4318,5 +4318,12 @@ struct torture_suite *ndr_cabinet_suite(TALLOC_CTX *ctx)
>  
>   torture_suite_add_ndr_pull_validate_test(suite, cab_file, cab_file_plain_data, cab_file_plain_check);
>  
> + /*
> + * we cannot validate, as libz' compression routines currently create a
> + * slightly different result
> + */
> +
> + /* torture_suite_add_ndr_pull_validate_test(suite, cab_file, cab_file_MSZIP_data, cab_file_MSZIP_check); */
> +
>   return suite;
>  }
> --
> 2.9.4
>





Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
Hi Jeremy,

This is in the push function, the checksum offset is obtained directly
from the ndr writer offset here:

Jeremy Allison via samba-technical <[hidden email]>
writes:

>> +
>> + for (i = 0; i < num_cfdata; i++, r++) {
>> + uint32_t compressed_offset, compressed_length = 0;
>> + uint32_t csum, csum_offset;
>> + uint32_t csumPartial;
>> +
>> + /*
>> + * checksum is a function of the size fields
>> + * and the potentially compressed data bytes,
>> + * which haven't been compressed yet so
>> + * remember offset, write zeroes, fill out
>> + * later
>> + */
>> + csum_offset = ndr->offset;
>> + NDR_CHECK(ndr_push_uint32(ndr, NDR_SCALARS, 0));
>> +


>> + csumPartial = ndr_cab_compute_checksum(&ndr->data[csum_offset + 8],
>
>                                                                  ^^^^^^^^^^^^^^^^
>                                                                  Integer wrap checks ?
> Didn't cnum_offset come from the client ?


The expression will wrap around 4GB indeed but since many fields of CAB
files are 32 bits I'm guessing it's also the file size hard limit of a
CAB file.

Since it's the push function we should probably return an error early if
the data to serialize is too big.

>> + /* write in the folder header the offset of its first data block */
>> + for (i = 0; i < r->cfheader.cFolders; i++) {
>> + SIVAL(ndr->data, OFFSET_OF_FOLDER_COFFCABSTART(i), ndr->offset);
>> + NDR_CHECK(ndr_push_folder_cfdata(ndr, r->cfdata + processed_cfdata, r->cffolders[i].typeCompress, r->cffolders[i].cCFData));
>> + processed_cfdata += r->cffolders[i].cCFData;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Here also ?

Same comment here I think.

--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
On Wed, Jun 28, 2017 at 12:18:00PM +0200, Aurélien Aptel wrote:

> Hi Jeremy,
>
> This is in the push function, the checksum offset is obtained directly
> from the ndr writer offset here:
>
> Jeremy Allison via samba-technical <[hidden email]>
> writes:
> >> +
> >> + for (i = 0; i < num_cfdata; i++, r++) {
> >> + uint32_t compressed_offset, compressed_length = 0;
> >> + uint32_t csum, csum_offset;
> >> + uint32_t csumPartial;
> >> +
> >> + /*
> >> + * checksum is a function of the size fields
> >> + * and the potentially compressed data bytes,
> >> + * which haven't been compressed yet so
> >> + * remember offset, write zeroes, fill out
> >> + * later
> >> + */
> >> + csum_offset = ndr->offset;
> >> + NDR_CHECK(ndr_push_uint32(ndr, NDR_SCALARS, 0));
> >> +
>
>
> >> + csumPartial = ndr_cab_compute_checksum(&ndr->data[csum_offset + 8],
> >
> >                                                                  ^^^^^^^^^^^^^^^^
> >                                                                  Integer wrap checks ?
> > Didn't cnum_offset come from the client ?
>
>
> The expression will wrap around 4GB indeed but since many fields of CAB
> files are 32 bits I'm guessing it's also the file size hard limit of a
> CAB file.
>
> Since it's the push function we should probably return an error early if
> the data to serialize is too big.
>
> >> + /* write in the folder header the offset of its first data block */
> >> + for (i = 0; i < r->cfheader.cFolders; i++) {
> >> + SIVAL(ndr->data, OFFSET_OF_FOLDER_COFFCABSTART(i), ndr->offset);
> >> + NDR_CHECK(ndr_push_folder_cfdata(ndr, r->cfdata + processed_cfdata, r->cffolders[i].typeCompress, r->cffolders[i].cCFData));
> >> + processed_cfdata += r->cffolders[i].cCFData;
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Here also ?
>
> Same comment here I think.

OK, can you send an update with those things in. Just trying to
feel comfortable with this :-).

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
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.

--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

mszip-v2 (49K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
On Thursday, 13 July 2017 01:22:02 CEST Jeremy Allison via samba-technical
wrote:
> 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.

Aurelien,

I've marked your commits where they should be squashed and I have an addional
commit. See

https://git.samba.org/?p=asn/samba.git;a=shortlog;h=refs/heads/master-cab

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
Here's a new version of the patchset. I've addressed Jeremy's last
comments and squashed the commits Andreas marked.

Cheers,

--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

mszip-v3.patchset (1M) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
Hi Aurelien,

I think you sent the wrong patchset.

Cheers,
Guenther

On 17/07/17 15:39, Aurélien Aptel wrote:
> Here's a new version of the patchset. I've addressed Jeremy's last
> comments and squashed the commits Andreas marked.
>
> Cheers,
>


--
Günther Deschner                    GPG-ID: 8EE11688
Red Hat                         [hidden email]
Samba Team                              [hidden email]


signature.asc (208 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
Günther Deschner <[hidden email]> writes:
> I think you sent the wrong patchset.

Grr. Right, sorry about that. Here's the right one.

--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

mszip-v3.patchset (48K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
I'm also running AFL [1] (smart automated fuzzer) on this sample program
[2] that just pulls a CAB file from its stdin. I'll give some updates on
if it finds something in a couple of days.

Cheers,

1: http://lcamtuf.coredump.cx/afl/
2: https://bitbucket.org/knarf/samba/commits/17b1d3d74203dd6e18729ad108068e44db4fa925?at=cab-afl

--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, Jul 17, 2017 at 05:55:43PM +0200, Aurélien Aptel wrote:
> Günther Deschner <[hidden email]> writes:
> > I think you sent the wrong patchset.
>
> Grr. Right, sorry about that. Here's the right one.

Still some issues, sorry :-(.

In:

From: Aurelien Aptel <[hidden email]>
Date: Tue, 23 May 2017 12:09:28 +0200
Subject: [PATCH 13/15] librpc/ndr: add MSZIP compression for cabinet files

In ndr_push_folder_cfdata()

+                       size_t old_off = ndr->offset;
+                       /* just copy the data */

Here you push the data.

+                       NDR_CHECK(ndr_push_bytes(ndr, r->ab.data, r->ab.length));

Then you do the overflow check. That's backwards.

+                       if (ndr->offset <= old_off || r->ab.length != ndr->offset - old_off) {
+                               return ndr_push_error(ndr, NDR_ERR_LENGTH, "strange NDR offsets (integer overflow?)");
+                       }
+                       compressed_length = ndr->offset - old_off;
+                       break;

When you do a ndr_push_bytes(ndr, data, length) you *KNOW*
it's going to advance ndr->offset by length bytes.

So in that case do the wrap checks *before* the push.

Also, look at all the other wrap checks in Samba.
They should always look like:

if (a + b < a) {
        error
}

In:

+               switch (cab_ctype) {
+               case CF_COMPRESS_NONE:
+                       /* just copy the data */
+                       r->ab = data_blob_talloc(ndr->current_mem_ctx,
+                                                ndr->data+ndr->offset,
+                                                r->cbUncomp);

                        Missing talloc return check.

+                       ndr->offset += r->cbUncomp;
+
+                       break;
+
+               case CF_COMPRESS_LZX:
+                       /* just copy the data */
+                       NDR_PULL_NEED_BYTES(ndr, r->cbData);
+                       r->ab = data_blob_talloc(ndr->current_mem_ctx,
+                                                ndr->data+ndr->offset,
+                                                r->cbData);

                        Missing talloc return check.

+                       ndr->offset += r->cbData;
+
+                       break;


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
Jeremy Allison <[hidden email]> writes:

> In ndr_push_folder_cfdata()
>
> +                       size_t old_off = ndr->offset;
> +                       /* just copy the data */
>
> Here you push the data.
>
> +                       NDR_CHECK(ndr_push_bytes(ndr, r->ab.data, r->ab.length));
>
> Then you do the overflow check. That's backwards.

> ...

> When you do a ndr_push_bytes(ndr, data, length) you *KNOW*
> it's going to advance ndr->offset by length bytes.
>
> So in that case do the wrap checks *before* the push.

I guess there's a misunderstanding, that's literally what you asked me
to do previously:

> 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).

I also *know* NDR already checks for overflows with the *_NEED_BYTES().

> Also, look at all the other wrap checks in Samba.
> They should always look like:
>
> if (a + b < a) {
> error
> }

I think I'll use an extra NEED_BYTES() call here which I think is even
less error prone and was made just for this purpose. Sounds good?

> ...
>
> Missing talloc return check.

Good catch.

v4 attached, with corrections and couple of added checks for paranoia,
let me know what you think.

--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

mszip-v4.patchset (49K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
On Wed, Jul 19, 2017 at 04:08:19PM +0200, Aurélien Aptel wrote:

> Jeremy Allison <[hidden email]> writes:
> > In ndr_push_folder_cfdata()
> >
> > +                       size_t old_off = ndr->offset;
> > +                       /* just copy the data */
> >
> > Here you push the data.
> >
> > +                       NDR_CHECK(ndr_push_bytes(ndr, r->ab.data, r->ab.length));
> >
> > Then you do the overflow check. That's backwards.
>
> > ...
>
> > When you do a ndr_push_bytes(ndr, data, length) you *KNOW*
> > it's going to advance ndr->offset by length bytes.
> >
> > So in that case do the wrap checks *before* the push.
>
> I guess there's a misunderstanding, that's literally what you asked me
> to do previously:

Oh, that's possibly true, sorry :-). I don't get much
time to review these so I tend to lose contexts between
reviews, sorry for the mistake.

> > 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).
>
> I also *know* NDR already checks for overflows with the *_NEED_BYTES().
>
> > Also, look at all the other wrap checks in Samba.
> > They should always look like:
> >
> > if (a + b < a) {
> > error
> > }
>
> I think I'll use an extra NEED_BYTES() call here which I think is even
> less error prone and was made just for this purpose. Sounds good?
>
> > ...
> >
> > Missing talloc return check.
>
> Good catch.
>
> v4 attached, with corrections and couple of added checks for paranoia,
> let me know what you think.

I'll try and finish this (finally:-) soon.

> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

> From b5c0cf7359dc512f2f46efce490147baa1af234d Mon Sep 17 00:00:00 2001
> From: Aurelien Aptel <[hidden email]>
> Date: Fri, 30 Jun 2017 15:07:31 +0200
> Subject: [PATCH 01/15] ndr_compression: use MAX_WBITS constant
>
> Signed-off-by: Aurelien Aptel <[hidden email]>
> Reviewed-by: Andreas Schneider <[hidden email]>
> ---
>  librpc/ndr/ndr_compression.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/librpc/ndr/ndr_compression.c b/librpc/ndr/ndr_compression.c
> index d291df5ad8c..e00bcb27364 100644
> --- a/librpc/ndr/ndr_compression.c
> +++ b/librpc/ndr/ndr_compression.c
> @@ -97,7 +97,7 @@ static enum ndr_err_code ndr_pull_compression_mszip_chunk(struct ndr_pull *ndrpu
>   z->zfree = ndr_zlib_free;
>   z->opaque = ndrpull;
>  
> - z_ret = inflateInit2(z, -15);
> + z_ret = inflateInit2(z, -MAX_WBITS);
>   if (z_ret != Z_OK) {
>   return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
>        "Bad inflateInit2 error %s(%d) (PULL)",
> --
> 2.12.3
>
>
> From 480877dfb34804f754d866374636ca1e99c9d38d Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <[hidden email]>
> Date: Tue, 9 May 2017 16:51:43 +0200
> Subject: [PATCH 02/15] librpc:ndr_cab: Cast data pointer correctly
>
> Signed-off-by: Andreas Schneider <[hidden email]>
> Reviewed-by: Guenther Deschner <[hidden email]>
> ---
>  librpc/ndr/ndr_cab.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/librpc/ndr/ndr_cab.c b/librpc/ndr/ndr_cab.c
> index ae95bf45b3b..110206855be 100644
> --- a/librpc/ndr/ndr_cab.c
> +++ b/librpc/ndr/ndr_cab.c
> @@ -79,7 +79,7 @@ static uint32_t ndr_cab_compute_checksum(uint8_t *data, uint32_t length, uint32_
>   pb = data;
>  
>   while (num_ulong-- > 0) {
> - ul = *pb++;
> + ul = (uint32_t)(*pb++);
>   ul |= (((uint32_t)(*pb++)) <<  8);
>   ul |= (((uint32_t)(*pb++)) << 16);
>   ul |= (((uint32_t)(*pb++)) << 24);
> @@ -95,7 +95,7 @@ static uint32_t ndr_cab_compute_checksum(uint8_t *data, uint32_t length, uint32_
>   case 2:
>   ul |= (((uint32_t)(*pb++)) <<  8);
>   case 1:
> - ul |= *pb++;
> + ul |= (uint32_t)(*pb++);
>   default:
>   break;
>   }
> --
> 2.12.3
>
>
> From f2a589e02b3b58f19f26102ff92e08321189f782 Mon Sep 17 00:00:00 2001
> From: Aurelien Aptel <[hidden email]>
> Date: Tue, 24 Jan 2017 19:00:53 +0100
> Subject: [PATCH 03/15] librpc/ndr: remove trailing whitespace from compression
>  file.
>
> Signed-off-by: Aurelien Aptel <[hidden email]>
> Reviewed-by: Guenther Deschner <[hidden email]>
> ---
>  librpc/ndr/ndr_compression.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/librpc/ndr/ndr_compression.c b/librpc/ndr/ndr_compression.c
> index e00bcb27364..240fad8ffd5 100644
> --- a/librpc/ndr/ndr_compression.c
> +++ b/librpc/ndr/ndr_compression.c
> @@ -1,21 +1,21 @@
> -/*
> +/*
>     Unix SMB/CIFS implementation.
>  
>     libndr compression support
>  
>     Copyright (C) Stefan Metzmacher 2005
>     Copyright (C) Matthieu Suiche 2008
> -  
> +
>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
>     the Free Software Foundation; either version 3 of the License, or
>     (at your option) any later version.
> -  
> +
>     This program is distributed in the hope that it will be useful,
>     but WITHOUT ANY WARRANTY; without even the implied warranty of
>     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>     GNU General Public License for more details.
> -  
> +
>     You should have received a copy of the GNU General Public License
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  */
> @@ -51,7 +51,7 @@ static enum ndr_err_code ndr_pull_compression_mszip_chunk(struct ndr_pull *ndrpu
>  
>   NDR_CHECK(ndr_pull_uint32(ndrpull, NDR_SCALARS, &plain_chunk_size));
>   if (plain_chunk_size > 0x00008000) {
> - return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION, "Bad MSZIP plain chunk size %08X > 0x00008000 (PULL)",
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION, "Bad MSZIP plain chunk size %08X > 0x00008000 (PULL)",
>        plain_chunk_size);
>   }
>  
> @@ -282,7 +282,7 @@ static enum ndr_err_code ndr_pull_compression_xpress_chunk(struct ndr_pull *ndrp
>  
>   NDR_CHECK(ndr_pull_uint32(ndrpull, NDR_SCALARS, &plain_chunk_size));
>   if (plain_chunk_size > 0x00010000) {
> - return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION, "Bad XPRESS plain chunk size %08X > 0x00010000 (PULL)",
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION, "Bad XPRESS plain chunk size %08X > 0x00010000 (PULL)",
>        plain_chunk_size);
>   }
>  
> @@ -505,7 +505,7 @@ enum ndr_err_code ndr_push_compression_end(struct ndr_push *subndr,
>   break;
>  
>   default:
> - return ndr_push_error(subndr, NDR_ERR_COMPRESSION, "Bad compression algorithm %d (PUSH)",
> + return ndr_push_error(subndr, NDR_ERR_COMPRESSION, "Bad compression algorithm %d (PUSH)",
>        compression_alg);
>   }
>  
> --
> 2.12.3
>
>
> From 05d5490045c0125ffaa07e61a3b7db6cbfb6a41f Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?G=C3=BCnther=20Deschner?= <[hidden email]>
> Date: Tue, 20 Sep 2016 00:18:43 +0200
> Subject: [PATCH 04/15] libndr/compression: pass down compressed length in
>  ndr_pull_compression_start
>
> Guenther
>
> Signed-off-by: Guenther Deschner <[hidden email]>
> ---
>  librpc/ndr/ndr_compression.c             | 3 ++-
>  librpc/ndr/ndr_compression.h             | 3 ++-
>  pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm | 3 ++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/librpc/ndr/ndr_compression.c b/librpc/ndr/ndr_compression.c
> index 240fad8ffd5..fd915ccaaa6 100644
> --- a/librpc/ndr/ndr_compression.c
> +++ b/librpc/ndr/ndr_compression.c
> @@ -383,7 +383,8 @@ static enum ndr_err_code ndr_push_compression_xpress_chunk(struct ndr_push *ndrp
>  enum ndr_err_code ndr_pull_compression_start(struct ndr_pull *subndr,
>      struct ndr_pull **_comndr,
>      enum ndr_compression_alg compression_alg,
> -    ssize_t decompressed_len)
> +    ssize_t decompressed_len,
> +    ssize_t compressed_len)
>  {
>   struct ndr_push *ndrpush;
>   struct ndr_pull *comndr;
> diff --git a/librpc/ndr/ndr_compression.h b/librpc/ndr/ndr_compression.h
> index 1c22fbf7595..d682d4d3693 100644
> --- a/librpc/ndr/ndr_compression.h
> +++ b/librpc/ndr/ndr_compression.h
> @@ -37,7 +37,8 @@
>  enum ndr_err_code ndr_pull_compression_start(struct ndr_pull *subndr,
>      struct ndr_pull **_comndr,
>      enum ndr_compression_alg compression_alg,
> -    ssize_t decompressed_len);
> +    ssize_t decompressed_len,
> +    ssize_t compressed_len);
>  enum ndr_err_code ndr_pull_compression_end(struct ndr_pull *subndr,
>    struct ndr_pull *comndr,
>    enum ndr_compression_alg compression_alg,
> diff --git a/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm b/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm
> index 6739b5f4391..cfcd29e25a7 100644
> --- a/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm
> +++ b/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm
> @@ -525,11 +525,12 @@ sub ParseCompressionPullStart($$$$$)
>   my $comndr = "$ndr\_compressed";
>   my $alg = compression_alg($e, $l);
>   my $dlen = compression_dlen($e, $l, $env);
> + my $clen = compression_clen($e, $l, $env);
>  
>   $self->pidl("{");
>   $self->indent;
>   $self->pidl("struct ndr_pull *$comndr;");
> - $self->pidl("NDR_CHECK(ndr_pull_compression_start($ndr, &$comndr, $alg, $dlen));");
> + $self->pidl("NDR_CHECK(ndr_pull_compression_start($ndr, &$comndr, $alg, $dlen, $clen));");
>  
>   return $comndr;
>  }
> --
> 2.12.3
>
>
> From 74b735a432cf4f30dad6fe09c79638e903dd10d8 Mon Sep 17 00:00:00 2001
> From: Aurelien Aptel <[hidden email]>
> Date: Tue, 23 May 2017 11:59:59 +0200
> Subject: [PATCH 05/15] librpc/ndr: add new ndr_compression_state
>
> Signed-off-by: Aurelien Aptel <[hidden email]>
> Reviewed-by: Guenther Deschner <[hidden email]>
> ---
>  librpc/ndr/libndr.h          |  6 ++++++
>  librpc/ndr/ndr_compression.c | 11 +++++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/librpc/ndr/libndr.h b/librpc/ndr/libndr.h
> index 072fd662e64..41e68ee7b98 100644
> --- a/librpc/ndr/libndr.h
> +++ b/librpc/ndr/libndr.h
> @@ -47,6 +47,8 @@ struct ndr_token_list {
>   uint32_t count;
>  };
>  
> +struct ndr_compression_state;
> +
>  /* this is the base structure passed to routines that
>     parse MSRPC formatted data
>  
> @@ -71,6 +73,8 @@ struct ndr_pull {
>   struct ndr_token_list array_length_list;
>   struct ndr_token_list switch_list;
>  
> + struct ndr_compression_state *cstate;
> +
>   TALLOC_CTX *current_mem_ctx;
>  
>   /* this is used to ensure we generate unique reference IDs
> @@ -97,6 +101,8 @@ struct ndr_push {
>   struct ndr_token_list dns_string_list;
>   struct ndr_token_list full_ptr_list;
>  
> + struct ndr_compression_state *cstate;
> +
>   /* this is used to ensure we generate unique reference IDs */
>   uint32_t ptr_count;
>  };
> diff --git a/librpc/ndr/ndr_compression.c b/librpc/ndr/ndr_compression.c
> index fd915ccaaa6..b2cb4341058 100644
> --- a/librpc/ndr/ndr_compression.c
> +++ b/librpc/ndr/ndr_compression.c
> @@ -26,6 +26,17 @@
>  #include "../librpc/ndr/ndr_compression.h"
>  #include <zlib.h>
>  
> +struct ndr_compression_state {
> + enum ndr_compression_alg type;
> + union {
> + struct {
> + struct z_stream_s *z;
> + uint8_t *dict;
> + size_t dict_size;
> + } mszip;
> + };
> +};
> +
>  static voidpf ndr_zlib_alloc(voidpf opaque, uInt items, uInt size)
>  {
>   return talloc_zero_size(opaque, items * size);
> --
> 2.12.3
>
>
> From d05af7116964a605e8192b50b86048f272455b7e Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?G=C3=BCnther=20Deschner?= <[hidden email]>
> Date: Tue, 23 May 2017 12:02:10 +0200
> Subject: [PATCH 06/15] librpc/ndr: add new MSZIP compression type for cabinet
>  files
>
> Guenther
>
> Signed-off-by: Guenther Deschner <[hidden email]>
> ---
>  librpc/ndr/libndr.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/librpc/ndr/libndr.h b/librpc/ndr/libndr.h
> index 41e68ee7b98..de93893be19 100644
> --- a/librpc/ndr/libndr.h
> +++ b/librpc/ndr/libndr.h
> @@ -262,6 +262,7 @@ enum ndr_err_code {
>  } while (0)
>  
>  enum ndr_compression_alg {
> + NDR_COMPRESSION_MSZIP_CAB = 1,
>   NDR_COMPRESSION_MSZIP = 2,
>   NDR_COMPRESSION_XPRESS = 3
>  };
> --
> 2.12.3
>
>
> From 846eecfc205bdd80ae35f8090e79dc5c373ac3cb Mon Sep 17 00:00:00 2001
> From: Aurelien Aptel <[hidden email]>
> Date: Tue, 23 May 2017 12:02:33 +0200
> Subject: [PATCH 07/15] librpc/ndr: add helper functions to setup and free
>  compression states.
>
> Signed-off-by: Aurelien Aptel <[hidden email]>
> Reviewed-by: Guenther Deschner <[hidden email]>
> ---
>  librpc/ndr/ndr_compression.c | 145 +++++++++++++++++++++++++++++++++++++++++++
>  librpc/ndr/ndr_compression.h |  10 +++
>  2 files changed, 155 insertions(+)
>
> diff --git a/librpc/ndr/ndr_compression.c b/librpc/ndr/ndr_compression.c
> index b2cb4341058..b0408d58a15 100644
> --- a/librpc/ndr/ndr_compression.c
> +++ b/librpc/ndr/ndr_compression.c
> @@ -524,3 +524,148 @@ enum ndr_err_code ndr_push_compression_end(struct ndr_push *subndr,
>   talloc_free(uncomndr);
>   return NDR_ERR_SUCCESS;
>  }
> +
> +static enum ndr_err_code generic_mszip_init(TALLOC_CTX *mem_ctx,
> +    struct ndr_compression_state *state)
> +{
> + z_stream *z = talloc_zero(mem_ctx, z_stream);
> + NDR_ERR_HAVE_NO_MEMORY(z);
> +
> + z->zalloc = ndr_zlib_alloc;
> + z->zfree  = ndr_zlib_free;
> + z->opaque = mem_ctx;
> +
> + state->mszip.z = z;
> + state->mszip.dict_size = 0;
> + /* pre-alloc dictionnary */
> + state->mszip.dict = talloc_array(mem_ctx, uint8_t, 0x8000);
> + NDR_ERR_HAVE_NO_MEMORY(state->mszip.dict);
> +
> + return NDR_ERR_SUCCESS;
> +}
> +
> +static void generic_mszip_free(struct ndr_compression_state *state)
> +{
> + if (state == NULL) {
> + return;
> + }
> +
> + TALLOC_FREE(state->mszip.z);
> + TALLOC_FREE(state->mszip.dict);
> +}
> +
> +
> +enum ndr_err_code ndr_pull_compression_state_init(struct ndr_pull *ndr,
> +  enum ndr_compression_alg compression_alg,
> +  struct ndr_compression_state **state)
> +{
> + struct ndr_compression_state *s;
> + int z_ret;
> +
> + s = talloc_zero(ndr, struct ndr_compression_state);
> + NDR_ERR_HAVE_NO_MEMORY(s);
> + s->type = compression_alg;
> +
> + switch (compression_alg) {
> + case NDR_COMPRESSION_MSZIP:
> + case NDR_COMPRESSION_XPRESS:
> + break;
> + case NDR_COMPRESSION_MSZIP_CAB:
> + NDR_CHECK(generic_mszip_init(ndr, s));
> + z_ret = inflateInit2(s->mszip.z, -MAX_WBITS);
> + if (z_ret != Z_OK) {
> + return ndr_pull_error(ndr, NDR_ERR_COMPRESSION,
> +      "zlib inflateinit2 error %s (%d) %s (PULL)",
> +      zError(z_ret), z_ret, s->mszip.z->msg);
> + }
> + break;
> + default:
> + return ndr_pull_error(ndr, NDR_ERR_COMPRESSION,
> +      "Bad compression algorithm %d (PULL)",
> +      compression_alg);
> + break;
> + }
> +
> + *state = s;
> +
> + return NDR_ERR_SUCCESS;
> +}
> +
> +void ndr_pull_compression_state_free(struct ndr_compression_state *state)
> +{
> + if (state == NULL) {
> + return;
> + }
> +
> + switch (state->type) {
> + case NDR_COMPRESSION_MSZIP:
> + case NDR_COMPRESSION_XPRESS:
> + break;
> + case NDR_COMPRESSION_MSZIP_CAB:
> + generic_mszip_free(state);
> + break;
> + default:
> + break;
> + }
> + TALLOC_FREE(state);
> +}
> +
> +enum ndr_err_code ndr_push_compression_state_init(struct ndr_push *ndr,
> +  enum ndr_compression_alg compression_alg,
> +  struct ndr_compression_state **state)
> +{
> + struct ndr_compression_state *s;
> + int z_ret;
> +
> + s = talloc_zero(ndr, struct ndr_compression_state);
> + NDR_ERR_HAVE_NO_MEMORY(s);
> + s->type = compression_alg;
> +
> + switch (compression_alg) {
> + case NDR_COMPRESSION_XPRESS:
> + case NDR_COMPRESSION_MSZIP:
> + break;
> + case NDR_COMPRESSION_MSZIP_CAB:
> + NDR_CHECK(generic_mszip_init(ndr, s));
> + z_ret = deflateInit2(s->mszip.z,
> +     Z_DEFAULT_COMPRESSION,
> +     Z_DEFLATED,
> +     -MAX_WBITS,
> +     8, /* memLevel */
> +     Z_DEFAULT_STRATEGY);
> + if (z_ret != Z_OK) {
> + return ndr_push_error(ndr, NDR_ERR_COMPRESSION,
> +      "zlib inflateinit2 error %s (%d) %s (PUSH)",
> +      zError(z_ret), z_ret, s->mszip.z->msg);
> + }
> + break;
> + default:
> + return ndr_push_error(ndr, NDR_ERR_COMPRESSION,
> +      "Bad compression algorithm %d (PUSH)",
> +      compression_alg);
> + break;
> + }
> +
> + *state = s;
> +
> + return NDR_ERR_SUCCESS;
> +}
> +
> +void ndr_push_compression_state_free(struct ndr_compression_state *state)
> +{
> + if (state == NULL) {
> + return;
> + }
> +
> + switch (state->type) {
> + case NDR_COMPRESSION_MSZIP:
> + case NDR_COMPRESSION_XPRESS:
> + break;
> + case NDR_COMPRESSION_MSZIP_CAB:
> + generic_mszip_free(state);
> + break;
> + default:
> + break;
> + }
> + TALLOC_FREE(state);
> +}
> diff --git a/librpc/ndr/ndr_compression.h b/librpc/ndr/ndr_compression.h
> index d682d4d3693..a67a73ed7d6 100644
> --- a/librpc/ndr/ndr_compression.h
> +++ b/librpc/ndr/ndr_compression.h
> @@ -51,6 +51,16 @@ enum ndr_err_code ndr_push_compression_end(struct ndr_push *subndr,
>    struct ndr_push *uncomndr,
>    enum ndr_compression_alg compression_alg,
>    ssize_t decompressed_len);
> +
> +enum ndr_err_code ndr_pull_compression_state_init(struct ndr_pull *ndr,
> +  enum ndr_compression_alg compression_alg,
> +  struct ndr_compression_state **state);
> +void ndr_pull_compression_state_free(struct ndr_compression_state *state);
> +enum ndr_err_code ndr_push_compression_state_init(struct ndr_push *ndr,
> +  enum ndr_compression_alg compression_alg,
> +  struct ndr_compression_state **state);
> +void ndr_push_compression_state_free(struct ndr_compression_state *state);
> +
>  #undef _PRINTF_ATTRIBUTE
>  #define _PRINTF_ATTRIBUTE(a1, a2)
>  
> --
> 2.12.3
>
>
> From cba168a4d104130d9513ba477c5bf6955e95ffe0 Mon Sep 17 00:00:00 2001
> From: Aurelien Aptel <[hidden email]>
> Date: Tue, 23 May 2017 15:31:44 +0200
> Subject: [PATCH 08/15] librpc: use DATA_BLOB in CFDATA structure
>
> Signed-off-by: Aurelien Aptel <[hidden email]>
> Reviewed-by: Guenther Deschner <[hidden email]>
> ---
>  librpc/idl/cab.idl            | 2 +-
>  librpc/ndr/ndr_cab.c          | 2 +-
>  source4/torture/ndr/cabinet.c | 6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/librpc/idl/cab.idl b/librpc/idl/cab.idl
> index 2174bce7334..9f3a02c231c 100644
> --- a/librpc/idl/cab.idl
> +++ b/librpc/idl/cab.idl
> @@ -117,7 +117,7 @@ import "misc.idl";
>  #if 0
>   uint8 abReserve[]; /* (optional) per-datablock reserved area */
>  #endif
> - uint8 ab[cbData]; /* compressed data bytes */
> + DATA_BLOB ab; /* compressed data bytes */
>   } CFDATA;
>  
>   typedef [nopush,nopull,public,flag(NDR_PAHEX|NDR_LITTLE_ENDIAN|NDR_NOALIGN)] struct {
> diff --git a/librpc/ndr/ndr_cab.c b/librpc/ndr/ndr_cab.c
> index 110206855be..2399a2248a6 100644
> --- a/librpc/ndr/ndr_cab.c
> +++ b/librpc/ndr/ndr_cab.c
> @@ -109,7 +109,7 @@ uint32_t ndr_cab_generate_checksum(const struct CFDATA *r)
>  {
>   uint32_t csumPartial;
>  
> - csumPartial = ndr_cab_compute_checksum(&r->ab[0], r->cbData, 0);
> + csumPartial = ndr_cab_compute_checksum(&r->ab.data[0], r->cbData, 0);
>  
>   return ndr_cab_compute_checksum((uint8_t *)discard_const(&r->cbData),
>   sizeof(r->cbData) + sizeof(r->cbUncomp),
> diff --git a/source4/torture/ndr/cabinet.c b/source4/torture/ndr/cabinet.c
> index 25fe373c842..790f7c37f11 100644
> --- a/source4/torture/ndr/cabinet.c
> +++ b/source4/torture/ndr/cabinet.c
> @@ -4171,7 +4171,7 @@ static bool cab_file_plain_check(struct torture_context *tctx,
>   blob = data_blob(NULL, r->cfdata[0].cbUncomp);
>   memset(blob.data, 'A', blob.length);
>  
> - torture_assert_mem_equal(tctx, r->cfdata[0].ab, blob.data, blob.length, "ab");
> + torture_assert_data_blob_equal(tctx, r->cfdata[0].ab, blob, "ab");
>  
>   return true;
>  }
> @@ -4235,7 +4235,7 @@ static bool cab_file_MSZIP_check(struct torture_context *tctx,
>   memset(blob.data, 'A', blob.length);
>  #if 0
>   /* once we have MSZIP compression working we can enable this test */
> - torture_assert_data_blob_equal(tctx, r->cfdata[0].mszip_data.ab, blob, "ab");
> + torture_assert_data_blob_equal(tctx, r->cfdata[0].ab, blob, "ab");
>  #endif
>   return true;
>  }
> @@ -4304,7 +4304,7 @@ static bool cab_file_LZX_check(struct torture_context *tctx,
>   memset(blob.data, 'A', blob.length);
>  #if 0
>   /* once we have LZX compression support we can enable this test */
> - torture_assert_data_blob_equal(tctx, r->cfdata[0].mszip_data.ab, blob, "ab");
> + torture_assert_data_blob_equal(tctx, r->cfdata[0].ab, blob, "ab");
>  #endif
>   return true;
>  }
> --
> 2.12.3
>
>
> From a6fee473f27d373fd3a0ca82980df79f842d9a23 Mon Sep 17 00:00:00 2001
> From: Aurelien Aptel <[hidden email]>
> Date: Tue, 23 May 2017 15:37:13 +0200
> Subject: [PATCH 09/15] librpc/ndr: remove unused ndr_cab_get_compression()
>  function
>
> Signed-off-by: Aurelien Aptel <[hidden email]>
> Reviewed-by: Guenther Deschner <[hidden email]>
> ---
>  librpc/ndr/ndr_cab.c | 9 ---------
>  librpc/ndr/ndr_cab.h | 1 -
>  2 files changed, 10 deletions(-)
>
> diff --git a/librpc/ndr/ndr_cab.c b/librpc/ndr/ndr_cab.c
> index 2399a2248a6..98800ebd7b5 100644
> --- a/librpc/ndr/ndr_cab.c
> +++ b/librpc/ndr/ndr_cab.c
> @@ -161,15 +161,6 @@ static bool ndr_size_cab_file(const struct cab_file *r, uint32_t *psize)
>   return true;
>  }
>  
> -enum cf_compress_type ndr_cab_get_compression(const struct cab_file *r)
> -{
> - if (r->cfheader.cFolders == 0) {
> - return CF_COMPRESS_NONE;
> - }
> -
> - return r->cffolders[0].typeCompress;
> -}
> -
>  _PUBLIC_ enum ndr_err_code ndr_push_cab_file(struct ndr_push *ndr, int ndr_flags, const struct cab_file *r)
>  {
>   uint32_t cntr_cffolders_0;
> diff --git a/librpc/ndr/ndr_cab.h b/librpc/ndr/ndr_cab.h
> index 79871530eb9..59dbc991442 100644
> --- a/librpc/ndr/ndr_cab.h
> +++ b/librpc/ndr/ndr_cab.h
> @@ -21,4 +21,3 @@
>  
>  uint32_t ndr_count_cfdata(const struct cab_file *r);
>  uint32_t ndr_cab_generate_checksum(const struct CFDATA *r);
> -enum cf_compress_type ndr_cab_get_compression(const struct cab_file *r);
> --
> 2.12.3
>
>
> From 590c403cc03141b3a681559d56834b2fc95415b8 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?G=C3=BCnther=20Deschner?= <[hidden email]>
> Date: Tue, 23 May 2017 15:48:42 +0200
> Subject: [PATCH 10/15] librpc/ndr: Use MAX_WBITS zlib define and change
>  memLevel in MSZIP code
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Guenther
>
> Signed-off-by: Günther Deschner <[hidden email]>
> ---
>  librpc/ndr/ndr_compression.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/librpc/ndr/ndr_compression.c b/librpc/ndr/ndr_compression.c
> index b0408d58a15..41328ea499c 100644
> --- a/librpc/ndr/ndr_compression.c
> +++ b/librpc/ndr/ndr_compression.c
> @@ -221,8 +221,8 @@ static enum ndr_err_code ndr_push_compression_mszip_chunk(struct ndr_push *ndrpu
>   z_ret = deflateInit2(z,
>       Z_DEFAULT_COMPRESSION,
>       Z_DEFLATED,
> -     -15,
> -     9,
> +     -MAX_WBITS,
> +     8, /* memLevel */
>       Z_DEFAULT_STRATEGY);
>   if (z_ret != Z_OK) {
>   return ndr_push_error(ndrpush, NDR_ERR_COMPRESSION,
> --
> 2.12.3
>
>
> From d88d09498b5e75d82bda7963b1dea74eba892e97 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <[hidden email]>
> Date: Wed, 21 Jun 2017 17:01:43 +0200
> Subject: [PATCH 11/15] librpc/ndr: Use correct value for max compression size
>
> Signed-off-by: Andreas Schneider <[hidden email]>
> Reviewed-by: Guenther Deschner <[hidden email]>
> ---
>  librpc/ndr/ndr_compression.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/librpc/ndr/ndr_compression.c b/librpc/ndr/ndr_compression.c
> index 41328ea499c..f6185b2ac86 100644
> --- a/librpc/ndr/ndr_compression.c
> +++ b/librpc/ndr/ndr_compression.c
> @@ -175,7 +175,11 @@ static enum ndr_err_code ndr_push_compression_mszip_chunk(struct ndr_push *ndrpu
>   uint32_t plain_chunk_size;
>   uint32_t plain_chunk_offset;
>   uint32_t max_plain_size = 0x00008000;
> - uint32_t max_comp_size = 0x00008000 + 2 + 12 /*TODO: what value do we really need here?*/;
> + /*
> + * The maximum compressed size of each MSZIP block is 32k + 12 bytes
> + * header size.
> + */
> + uint32_t max_comp_size = 0x00008000 + 12;
>   uint32_t tmp_offset;
>   int z_ret;
>  
> @@ -208,7 +212,7 @@ static enum ndr_err_code ndr_push_compression_mszip_chunk(struct ndr_push *ndrpu
>   z->total_in = 0;
>  
>   z->next_out = comp_chunk.data + 2;
> - z->avail_out = comp_chunk.length - 2;
> + z->avail_out = comp_chunk.length;
>   z->total_out = 0;
>  
>   if (!z->opaque) {
> --
> 2.12.3
>
>
> From d4e815379601301e3cf89344df7c09c6f8ca1591 Mon Sep 17 00:00:00 2001
> From: Aurelien Aptel <[hidden email]>
> Date: Tue, 23 May 2017 15:41:24 +0200
> Subject: [PATCH 12/15] librpc/ndr: simplify cabinet file size calculation
>
> Signed-off-by: Aurelien Aptel <[hidden email]>
> Reviewed-by: Guenther Deschner <[hidden email]>
> ---
>  librpc/ndr/ndr_cab.c | 53 +++-------------------------------------------------
>  1 file changed, 3 insertions(+), 50 deletions(-)
>
> diff --git a/librpc/ndr/ndr_cab.c b/librpc/ndr/ndr_cab.c
> index 98800ebd7b5..efd337d5daa 100644
> --- a/librpc/ndr/ndr_cab.c
> +++ b/librpc/ndr/ndr_cab.c
> @@ -116,57 +116,11 @@ uint32_t ndr_cab_generate_checksum(const struct CFDATA *r)
>   csumPartial);
>  }
>  
> -static bool ndr_size_cab_file(const struct cab_file *r, uint32_t *psize)
> -{
> - uint32_t size = 0;
> - int i;
> -
> - /* header */
> - size += 36;
> -
> - /* folder */
> - for (i = 0; i < r->cfheader.cFolders; i++) {
> - if (size + 8 < size) {
> - /* Integer wrap. */
> - return false;
> - }
> - size += 8;
> - }
> -
> - /* files */
> - for (i = 0; i < r->cfheader.cFiles; i++) {
> - uint32_t cfsize = ndr_size_CFFILE(&r->cffiles[i], 0);
> - if (size + cfsize < size) {
> - /* Integer wrap. */
> - return false;
> - }
> - size += cfsize;
> - }
> -
> - /* data */
> - for (i = 0; i < ndr_count_cfdata(r); i++) {
> - if (size + 8 < size) {
> - /* Integer wrap. */
> - return false;
> - }
> - size += 8;
> - if (size + r->cfdata[i].cbData < size) {
> - /* Integer wrap. */
> - return false;
> - }
> - size += r->cfdata[i].cbData;
> - }
> -
> - *psize = size;
> - return true;
> -}
> -
>  _PUBLIC_ enum ndr_err_code ndr_push_cab_file(struct ndr_push *ndr, int ndr_flags, const struct cab_file *r)
>  {
>   uint32_t cntr_cffolders_0;
>   uint32_t cntr_cffiles_0;
>   uint32_t cntr_cfdata_0;
> - uint32_t cab_size = 0;
>   {
>   uint32_t _flags_save_STRUCT = ndr->flags;
>   ndr_set_flags(&ndr->flags, LIBNDR_PRINT_ARRAY_HEX|LIBNDR_FLAG_LITTLE_ENDIAN|LIBNDR_FLAG_NOALIGN);
> @@ -199,10 +153,9 @@ _PUBLIC_ enum ndr_err_code ndr_push_cab_file(struct ndr_push *ndr, int ndr_flags
>   ndr->flags = _flags_save_STRUCT;
>   }
>  
> - if (ndr_size_cab_file(r, &cab_size) == false) {
> - return NDR_ERR_VALIDATE;
> - }
> - SIVAL(ndr->data, 8, cab_size);
> +
> + /* write total file size in header */
> + SIVAL(ndr->data, 8, ndr->offset);
>  
>   return NDR_ERR_SUCCESS;
>  }
> --
> 2.12.3
>
>
> From ce9bae110aa63bfa81e0cb5c8c60deac7db9876e Mon Sep 17 00:00:00 2001
> From: Aurelien Aptel <[hidden email]>
> Date: Tue, 23 May 2017 12:09:28 +0200
> Subject: [PATCH 13/15] librpc/ndr: add MSZIP compression for cabinet files
>
> Signed-off-by: Aurelien Aptel <[hidden email]>
> Reviewed-by: Guenther Deschner <[hidden email]>
> ---
>  librpc/ndr/ndr_cab.c          | 252 +++++++++++++++++++++++++++++++++++++--
>  librpc/ndr/ndr_compression.c  | 266 ++++++++++++++++++++++++++++++++++++++++++
>  source4/torture/ndr/cabinet.c |   5 +-
>  3 files changed, 508 insertions(+), 15 deletions(-)
>
> diff --git a/librpc/ndr/ndr_cab.c b/librpc/ndr/ndr_cab.c
> index efd337d5daa..837ed253065 100644
> --- a/librpc/ndr/ndr_cab.c
> +++ b/librpc/ndr/ndr_cab.c
> @@ -21,6 +21,9 @@
>  
>  #include "includes.h"
>  #include "librpc/gen_ndr/ndr_cab.h"
> +#include "librpc/ndr/ndr_compression.h"
> +
> +#define OFFSET_OF_FOLDER_COFFCABSTART(folder) (36 /* cfheader size */ + (size_t)(folder)*8)
>  
>  _PUBLIC_ void ndr_print_cf_time(struct ndr_print *ndr, const char *name, const struct cf_time *r)
>  {
> @@ -116,35 +119,175 @@ uint32_t ndr_cab_generate_checksum(const struct CFDATA *r)
>   csumPartial);
>  }
>  
> +/* Push all CFDATA of a folder.
> + *
> + * This works on a folder level because compression type is set per
> + * folder, and a compression state can be shared between CFDATA of the
> + * same folder.
> + *
> + * This is not a regular NDR func as we pass the compression type and
> + * the number of CFDATA as extra arguments
> + */
> +static enum ndr_err_code ndr_push_folder_cfdata(struct ndr_push *ndr,
> + const struct CFDATA *r,
> + enum cf_compress_type cab_ctype,
> + size_t num_cfdata)
> +{
> + size_t i;
> + enum ndr_compression_alg ndr_ctype = 0;
> +
> + ndr_set_flags(&ndr->flags, LIBNDR_PRINT_ARRAY_HEX|LIBNDR_FLAG_LITTLE_ENDIAN|LIBNDR_FLAG_NOALIGN);
> +
> + if (cab_ctype == CF_COMPRESS_MSZIP) {
> + ndr_ctype = NDR_COMPRESSION_MSZIP_CAB;
> + NDR_CHECK(ndr_push_compression_state_init(ndr, ndr_ctype, &ndr->cstate));
> + }
> +
> + for (i = 0; i < num_cfdata; i++, r++) {
> + uint32_t compressed_length = 0;
> + uint32_t csum, csumPartial;
> + size_t compressed_offset, csum_offset, data_offset;
> +
> + if (!r->ab.data) {
> + return ndr_push_error(ndr, NDR_ERR_LENGTH,
> +      "NULL uncompressed data blob");
> + }
> + if (r->ab.length != r->cbUncomp) {
> + return ndr_push_error(ndr, NDR_ERR_LENGTH,
> +      "Uncompressed data blob size != uncompressed data size field");
> + }
> +
> + /*
> + * checksum is a function of the size fields
> + * and the potentially compressed data bytes,
> + * which haven't been compressed yet so
> + * remember offset, write zeroes, fill out
> + * later
> + */
> + csum_offset = ndr->offset;
> + NDR_CHECK(ndr_push_uint32(ndr, NDR_SCALARS, 0));
> +
> + /*
> + * 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));
> +
> + data_offset = ndr->offset;
> +
> + switch (cab_ctype) {
> + case CF_COMPRESS_NONE:
> + /* just copy the data */
> + NDR_PUSH_NEED_BYTES(ndr, r->ab.length);
> + NDR_CHECK(ndr_push_bytes(ndr, r->ab.data, r->ab.length));
> + compressed_length = r->ab.length;
> + break;
> + case CF_COMPRESS_LZX:
> + /*
> + * we have not yet worked out the details of LZX
> + * compression
> + */
> + return NDR_ERR_COMPRESSION;
> +
> + case CF_COMPRESS_MSZIP: {
> + struct ndr_push *push_sub, *push_compress;
> +
> + /* compress via subcontext */
> + NDR_CHECK(ndr_push_subcontext_start(ndr, &push_sub, 0, -1));
> + push_sub->cstate = ndr->cstate;
> + NDR_CHECK(ndr_push_compression_start(push_sub, &push_compress, ndr_ctype, -1));
> + ndr_set_flags(&push_compress->flags, LIBNDR_FLAG_REMAINING);
> + NDR_CHECK(ndr_push_DATA_BLOB(push_compress, NDR_SCALARS, r->ab));
> + NDR_CHECK(ndr_push_compression_end(push_sub, push_compress, ndr_ctype, -1));
> + NDR_CHECK(ndr_push_subcontext_end(ndr, push_sub, 0, -1));
> + compressed_length = push_sub->offset;
> +
> + break;
> + }
> + default:
> + return NDR_ERR_BAD_SWITCH;
> + }
> +
> + /* we can now write the compressed size and the checksum */
> + SSVAL(ndr->data, compressed_offset, compressed_length);
> +
> + /*
> + * Create checksum over compressed data.
> + *
> + * The 8 bytes are the header size.
> + *
> + * We have already have written the checksum and set it to zero,
> + * earlier. So we know that after the checksum end the value
> + * for the compressed length comes the blob data.
> + *
> + * NDR already did all the checks for integer wraps.
> + */
> + csumPartial = ndr_cab_compute_checksum(&ndr->data[data_offset],
> +       compressed_length, 0);
> +
> + /*
> + * Checksum over header (compressed and uncompressed length).
> + *
> + * The first 4 bytes are the checksum size.
> + * The second 4 bytes are the size of the compressed and
> + * uncompressed length fields.
> + *
> + * NDR already did all the checks for integer wraps.
> + */
> + csum = ndr_cab_compute_checksum(&ndr->data[compressed_offset],
> + data_offset - compressed_offset,
> + csumPartial);
> +
> + SIVAL(ndr->data, csum_offset, csum);
> + }
> +
> + ndr_push_compression_state_free(ndr->cstate);
> + ndr->cstate = NULL;
> +
> + return NDR_ERR_SUCCESS;
> +}
> +
>  _PUBLIC_ enum ndr_err_code ndr_push_cab_file(struct ndr_push *ndr, int ndr_flags, const struct cab_file *r)
>  {
>   uint32_t cntr_cffolders_0;
>   uint32_t cntr_cffiles_0;
> - uint32_t cntr_cfdata_0;
> + size_t processed_cfdata = 0;
>   {
>   uint32_t _flags_save_STRUCT = ndr->flags;
>   ndr_set_flags(&ndr->flags, LIBNDR_PRINT_ARRAY_HEX|LIBNDR_FLAG_LITTLE_ENDIAN|LIBNDR_FLAG_NOALIGN);
>   NDR_PUSH_CHECK_FLAGS(ndr, ndr_flags);
> +
>   if (ndr_flags & NDR_SCALARS) {
> - uint32_t next_offset = 0;
> + uint32_t i;
>   NDR_CHECK(ndr_push_align(ndr, 4));
>   NDR_CHECK(ndr_push_CFHEADER(ndr, NDR_SCALARS, &r->cfheader));
>   for (cntr_cffolders_0 = 0; cntr_cffolders_0 < (r->cfheader.cFolders); cntr_cffolders_0++) {
>   NDR_CHECK(ndr_push_CFFOLDER(ndr, NDR_SCALARS, &r->cffolders[cntr_cffolders_0]));
>   }
>   for (cntr_cffiles_0 = 0; cntr_cffiles_0 < (r->cfheader.cFiles); cntr_cffiles_0++) {
> - uint32_t offset = ndr->offset + 4;
>   NDR_CHECK(ndr_push_CFFILE(ndr, NDR_SCALARS, &r->cffiles[cntr_cffiles_0]));
> - if (cntr_cffiles_0 > 0) {
> - next_offset += r->cffiles[cntr_cffiles_0 - 1].cbFile;
> - }
> - SIVAL(ndr->data, offset, next_offset);
>   }
>  #if 0
>   NDR_CHECK(ndr_push_uint32(ndr, NDR_SCALARS, ndr_count_cfdata(r)));
>  #endif
> - for (cntr_cfdata_0 = 0; cntr_cfdata_0 < (ndr_count_cfdata(r)); cntr_cfdata_0++) {
> - NDR_CHECK(ndr_push_CFDATA(ndr, NDR_SCALARS, &r->cfdata[cntr_cfdata_0]));
> +
> + /* write in the folder header the offset of its first data block */
> + for (i = 0; i < r->cfheader.cFolders; i++) {
> + size_t off = OFFSET_OF_FOLDER_COFFCABSTART(i);
> + /* check that the offset we want to
> + * write to is always inside our
> + * current push buffer
> + */
> + if (off >= ndr->offset) {
> + return ndr_push_error(ndr, NDR_ERR_OFFSET,
> +      "trying to write past current push buffer size");
> + }
> + SIVAL(ndr->data, off, ndr->offset);
> + NDR_CHECK(ndr_push_folder_cfdata(ndr, r->cfdata + processed_cfdata, r->cffolders[i].typeCompress, r->cffolders[i].cCFData));
> + processed_cfdata += r->cffolders[i].cCFData;
>   }
>   NDR_CHECK(ndr_push_trailer_align(ndr, 4));
>   }
> @@ -160,6 +303,87 @@ _PUBLIC_ enum ndr_err_code ndr_push_cab_file(struct ndr_push *ndr, int ndr_flags
>   return NDR_ERR_SUCCESS;
>  }
>  
> +
> +/* Pull all CFDATA of a folder.
> + *
> + * This works on a folder level because compression type is set per
> + * folder, and a compression state can be shared between CFDATA of the
> + * same folder.
> + *
> + * This is not a regular NDR func as we pass the compression type and
> + * the number of CFDATA as extra arguments
> + */
> +static enum ndr_err_code ndr_pull_folder_cfdata(struct ndr_pull *ndr,
> + struct CFDATA *r,
> + enum cf_compress_type cab_ctype,
> + size_t num_cfdata)
> +{
> + size_t i;
> + enum ndr_compression_alg ndr_ctype = 0;
> +
> + if (cab_ctype == CF_COMPRESS_MSZIP) {
> + ndr_ctype = NDR_COMPRESSION_MSZIP_CAB;
> + NDR_CHECK(ndr_pull_compression_state_init(ndr, NDR_COMPRESSION_MSZIP_CAB, &ndr->cstate));
> + }
> +
> + for (i = 0; i < num_cfdata; i++, r++) {
> + NDR_CHECK(ndr_pull_uint32(ndr, NDR_SCALARS, &r->csum));
> + NDR_CHECK(ndr_pull_uint16(ndr, NDR_SCALARS, &r->cbData));
> + NDR_CHECK(ndr_pull_uint16(ndr, NDR_SCALARS, &r->cbUncomp));
> +
> + switch (cab_ctype) {
> + case CF_COMPRESS_NONE:
> + /* just copy the data */
> + NDR_PULL_NEED_BYTES(ndr, r->cbUncomp);
> + r->ab = data_blob_talloc(ndr->current_mem_ctx,
> + ndr->data+ndr->offset,
> + r->cbUncomp);
> + if (r->ab.data == NULL) {
> + return ndr_pull_error(ndr, NDR_ERR_ALLOC,
> +      "failed to allocate buffer for uncompressed CFDATA block");
> + }
> + ndr->offset += r->cbUncomp;
> + break;
> +
> + case CF_COMPRESS_LZX:
> + /* just copy the data (LZX decompression not implemented yet) */
> + NDR_PULL_NEED_BYTES(ndr, r->cbData);
> + r->ab = data_blob_talloc(ndr->current_mem_ctx,
> + ndr->data+ndr->offset,
> + r->cbData);
> + if (r->ab.data == NULL) {
> + return ndr_pull_error(ndr, NDR_ERR_ALLOC,
> +      "failed to allocate buffer for LZX-compressed CFDATA block");
> + }
> + ndr->offset += r->cbData;
> + break;
> +
> + case CF_COMPRESS_MSZIP: {
> + struct ndr_pull *pull_sub, *pull_compress;
> + NDR_PULL_NEED_BYTES(ndr, r->cbData);
> + /* decompress via subcontext */
> + NDR_CHECK(ndr_pull_subcontext_start(ndr, &pull_sub, 0, r->cbData));
> + pull_sub->cstate = ndr->cstate;
> + NDR_CHECK(ndr_pull_compression_start(pull_sub, &pull_compress,
> +     ndr_ctype, r->cbUncomp, r->cbData));
> + ndr_set_flags(&pull_compress->flags, LIBNDR_FLAG_REMAINING);
> + NDR_CHECK(ndr_pull_DATA_BLOB(pull_compress, NDR_SCALARS, &r->ab));
> + NDR_CHECK(ndr_pull_compression_end(pull_sub, pull_compress, ndr_ctype, r->cbUncomp));
> + NDR_CHECK(ndr_pull_subcontext_end(ndr, pull_sub, 0, r->cbData));
> +
> + break;
> + }
> + default:
> + return NDR_ERR_BAD_SWITCH;
> + }
> + }
> +
> + ndr_pull_compression_state_free(ndr->cstate);
> + ndr->cstate = NULL;
> +
> + return NDR_ERR_SUCCESS;
> +}
> +
>  _PUBLIC_ enum ndr_err_code ndr_pull_cab_file(struct ndr_pull *ndr, int ndr_flags, struct cab_file *r)
>  {
>   uint32_t size_cffolders_0 = 0;
> @@ -169,7 +393,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_cab_file(struct ndr_pull *ndr, int ndr_flags
>   uint32_t cntr_cffiles_0;
>   TALLOC_CTX *_mem_save_cffiles_0 = NULL;
>   uint32_t size_cfdata_0 = 0;
> - uint32_t cntr_cfdata_0;
> + size_t processed_cfdata = 0;
>   TALLOC_CTX *_mem_save_cfdata_0 = NULL;
>   {
>   uint32_t _flags_save_STRUCT = ndr->flags;
> @@ -203,8 +427,12 @@ _PUBLIC_ enum ndr_err_code ndr_pull_cab_file(struct ndr_pull *ndr, int ndr_flags
>   NDR_PULL_ALLOC_N(ndr, r->cfdata, size_cfdata_0);
>   _mem_save_cfdata_0 = NDR_PULL_GET_MEM_CTX(ndr);
>   NDR_PULL_SET_MEM_CTX(ndr, r->cfdata, 0);
> - for (cntr_cfdata_0 = 0; cntr_cfdata_0 < (size_cfdata_0); cntr_cfdata_0++) {
> - NDR_CHECK(ndr_pull_CFDATA(ndr, NDR_SCALARS, &r->cfdata[cntr_cfdata_0]));
> + for (cntr_cffolders_0 = 0; cntr_cffolders_0 < (size_cffolders_0); cntr_cffolders_0++) {
> + NDR_CHECK(ndr_pull_folder_cfdata(ndr,
> + r->cfdata + processed_cfdata,
> + r->cffolders[cntr_cffolders_0].typeCompress,
> + r->cffolders[cntr_cffolders_0].cCFData));
> + processed_cfdata += r->cffolders[cntr_cffolders_0].cCFData;
>   }
>   NDR_PULL_SET_MEM_CTX(ndr, _mem_save_cfdata_0, 0);
>   NDR_CHECK(ndr_pull_trailer_align(ndr, 4));
> diff --git a/librpc/ndr/ndr_compression.c b/librpc/ndr/ndr_compression.c
> index f6185b2ac86..bdce4317a92 100644
> --- a/librpc/ndr/ndr_compression.c
> +++ b/librpc/ndr/ndr_compression.c
> @@ -47,6 +47,261 @@ static void  ndr_zlib_free(voidpf opaque, voidpf address)
>   talloc_free(address);
>  }
>  
> +static enum ndr_err_code ndr_pull_compression_mszip_cab_chunk(struct ndr_pull *ndrpull,
> +      struct ndr_push *ndrpush,
> +      struct ndr_compression_state *state,
> +      ssize_t decompressed_len,
> +      ssize_t compressed_len)
> +{
> + DATA_BLOB comp_chunk;
> + uint32_t comp_chunk_offset;
> + uint32_t comp_chunk_size;
> + DATA_BLOB plain_chunk;
> + uint32_t plain_chunk_offset;
> + uint32_t plain_chunk_size;
> + z_stream *z = state->mszip.z;
> + int z_ret;
> +
> + plain_chunk_size = decompressed_len;
> +
> + if (plain_chunk_size > 0x00008000) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "Bad MSZIP CAB plain chunk size %08X > 0x00008000 (PULL)",
> +      plain_chunk_size);
> + }
> +
> +
> + comp_chunk_size = compressed_len;
> +
> + DEBUG(9,("MSZIP CAB plain_chunk_size: %08X (%u) comp_chunk_size: %08X (%u)\n",
> + plain_chunk_size, plain_chunk_size, comp_chunk_size, comp_chunk_size));
> +
> + comp_chunk_offset = ndrpull->offset;
> + NDR_CHECK(ndr_pull_advance(ndrpull, comp_chunk_size));
> + comp_chunk.length = comp_chunk_size;
> + comp_chunk.data = ndrpull->data + comp_chunk_offset;
> +
> + plain_chunk_offset = ndrpush->offset;
> + NDR_CHECK(ndr_push_zero(ndrpush, plain_chunk_size));
> + plain_chunk.length = plain_chunk_size;
> + plain_chunk.data = ndrpush->data + plain_chunk_offset;
> +
> + if (comp_chunk.length < 2) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "Bad MSZIP CAB comp chunk size %u < 2 (PULL)",
> +      (unsigned int)comp_chunk.length);
> + }
> + /* CK = Chris Kirmse, official Microsoft purloiner */
> + if (comp_chunk.data[0] != 'C' ||
> +    comp_chunk.data[1] != 'K') {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "Bad MSZIP CAB invalid prefix [%c%c] != [CK]",
> +      comp_chunk.data[0], comp_chunk.data[1]);
> + }
> +
> + /*
> + * This is a MSZIP block. It is actually using the deflate
> + * algorithm which can be decompressed by zlib. zlib will try
> + * to decompress as much as it can in each run. If we provide
> + * all the input and enough room for the uncompressed output,
> + * one call is enough. It will loop over all the sub-blocks
> + * that make up a deflate block.
> + *
> + * See corresponding push function for more info.
> + */
> +
> + z->next_in = comp_chunk.data + 2;
> + z->avail_in = comp_chunk.length - 2;
> + z->next_out = plain_chunk.data;
> + z->avail_out = plain_chunk.length;
> +
> + /*
> + * Each MSZIP CDATA contains a complete deflate stream
> + * i.e. the stream starts and ends in the CFDATA but the
> + * _dictionnary_ is shared between all CFDATA of a CFFOLDER.
> + *
> + * When decompressing, the initial dictionnary of the first
> + * CDATA is empty. All other CFDATA use the previous CFDATA
> + * uncompressed output as dictionnary.
> + */
> +
> + if (state->mszip.dict_size) {
> + z_ret = inflateSetDictionary(z, state->mszip.dict, state->mszip.dict_size);
> + if (z_ret != Z_OK) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib inflateSetDictionary error %s (%d) %s (PULL)",
> +      zError(z_ret), z_ret, z->msg);
> + }
> + }
> +
> + z_ret = inflate(z, Z_FINISH);
> + if (z_ret == Z_OK) {
> + /*
> + * Z_OK here means there was no error but the stream
> + * hasn't been fully decompressed because there was
> + * not enough room for the output, which should not
> + * happen
> + */
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib inflate error not enough space for ouput (PULL)");
> + }
> + if (z_ret != Z_STREAM_END) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib inflate error %s (%d) %s (PULL)", zError(z_ret), z_ret, z->msg);
> + }
> +
> + if (z->total_out < plain_chunk.length) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib uncompressed output is smaller than expected (%lu < %zu) (PULL)",
> +      z->total_out, plain_chunk.length);
> + }
> +
> + /*
> + * Keep a copy of the output to set as dictionnary for the
> + * next decompression call.
> + *
> + * The input pointer seems to be still valid between calls, so
> + * we can just store that instead of copying the memory over
> + * the dict temp buffer.
> + */
> + state->mszip.dict = plain_chunk.data;
> + state->mszip.dict_size = plain_chunk.length;
> +
> + z_ret = inflateReset(z);
> + if (z_ret != Z_OK) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib inflateReset error %s (%d) %s (PULL)",
> +      zError(z_ret), z_ret, z->msg);
> + }
> +
> + return NDR_ERR_SUCCESS;
> +}
> +
> +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;
> +
> + if (ndrpull->data_size <= ndrpull->offset) {
> + return ndr_push_error(ndrpush, NDR_ERR_COMPRESSION,
> +      "strange NDR pull size and offset (integer overflow?)");
> +
> + }
> +
> + plain_chunk_size = MIN(max_plain_size, ndrpull->data_size - ndrpull->offset);
> + plain_chunk_offset = ndrpull->offset;
> + NDR_CHECK(ndr_pull_advance(ndrpull, plain_chunk_size));
> +
> + plain_chunk.data = ndrpull->data + plain_chunk_offset;
> + plain_chunk.length = plain_chunk_size;
> +
> + NDR_CHECK(ndr_push_expand(ndrpush, max_comp_size));
> +
> + comp_chunk.data = ndrpush->data + ndrpush->offset;
> + comp_chunk.length = max_comp_size;
> +
> + /* CK = Chris Kirmse, official Microsoft purloiner */
> + comp_chunk.data[0] = 'C';
> + comp_chunk.data[1] = 'K';
> +
> + z = state->mszip.z;
> + z->next_in = plain_chunk.data;
> + z->avail_in = plain_chunk.length;
> + z->total_in = 0;
> +
> + z->next_out = comp_chunk.data + 2;
> + z->avail_out = comp_chunk.length;
> + z->total_out = 0;
> +
> + /*
> + * See pull function for explanations of the MSZIP format.
> + *
> + * The CFDATA block contains a full deflate stream. Each stream
> + * uses the uncompressed input of the previous CFDATA in the
> + * same CFFOLDER as a dictionnary for the compression.
> + */
> +
> + if (state->mszip.dict_size) {
> + z_ret = deflateSetDictionary(z, state->mszip.dict, state->mszip.dict_size);
> + if (z_ret != Z_OK) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib deflateSetDictionary error %s (%d) %s (PUSH)",
> +      zError(z_ret), z_ret, z->msg);
> + }
> + }
> +
> + /*
> + * Z_FINISH should make deflate process all of the input in
> + * one call. If the stream is not finished there was an error
> + * e.g. not enough room to store the compressed output.
> + */
> + z_ret = deflate(z, Z_FINISH);
> + if (z_ret != Z_STREAM_END) {
> + return ndr_push_error(ndrpush, NDR_ERR_COMPRESSION,
> +      "zlib deflate error %s (%d) %s (PUSH)",
> +      zError(z_ret), z_ret, z->msg);
> + }
> +
> + if (z->avail_in) {
> + return ndr_push_error(ndrpush, NDR_ERR_COMPRESSION,
> +      "MSZIP not all avail_in[%u] bytes consumed (PUSH)",
> +      z->avail_in);
> + }
> +
> + comp_chunk_size = 2 + z->total_out;
> + if (comp_chunk_size < z->total_out) {
> + return ndr_push_error(ndrpush, NDR_ERR_COMPRESSION,
> +      "strange NDR push compressed size (integer overflow?)");
> + }
> +
> + z_ret = deflateReset(z);
> + if (z_ret != Z_OK) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib deflateReset error %s (%d) %s (PUSH)",
> +      zError(z_ret), z_ret, z->msg);
> + }
> +
> + if (plain_chunk.length > talloc_array_length(state->mszip.dict)) {
> + return ndr_pull_error(ndrpull, NDR_ERR_COMPRESSION,
> +      "zlib dict buffer is too big (PUSH)");
> + }
> +
> + /*
> + * Keep a copy of the input to set as dictionnary for the next
> + * compression call.
> + *
> + * Ideally we would just store the input pointer and length
> + * without copying but the memory gets invalidated between the
> + * calls, so we just copy to a dedicated buffer we now is
> + * still going to been valid for the lifetime of the
> + * compressions state object.
> + */
> + memcpy(state->mszip.dict, plain_chunk.data, plain_chunk.length);
> + state->mszip.dict_size = plain_chunk.length;
> +
> + DEBUG(9,("MSZIP comp plain_chunk_size: %08X (%u) comp_chunk_size: %08X (%u)\n",
> + (unsigned int)plain_chunk.length,
> + (unsigned int)plain_chunk.length,
> + comp_chunk_size, comp_chunk_size));
> +
> + ndrpush->offset += comp_chunk_size;
> + return NDR_ERR_SUCCESS;
> +}
> +
> +
>  static enum ndr_err_code ndr_pull_compression_mszip_chunk(struct ndr_pull *ndrpull,
>   struct ndr_push *ndrpush,
>   z_stream *z,
> @@ -411,6 +666,12 @@ enum ndr_err_code ndr_pull_compression_start(struct ndr_pull *subndr,
>   NDR_ERR_HAVE_NO_MEMORY(ndrpush);
>  
>   switch (compression_alg) {
> + case NDR_COMPRESSION_MSZIP_CAB:
> + NDR_CHECK(ndr_pull_compression_mszip_cab_chunk(subndr, ndrpush,
> +       subndr->cstate,
> +       decompressed_len,
> +       compressed_len));
> + break;
>   case NDR_COMPRESSION_MSZIP:
>   ZERO_STRUCT(z);
>   while (!last) {
> @@ -470,6 +731,7 @@ enum ndr_err_code ndr_push_compression_start(struct ndr_push *subndr,
>   struct ndr_push *uncomndr;
>  
>   switch (compression_alg) {
> + case NDR_COMPRESSION_MSZIP_CAB:
>   case NDR_COMPRESSION_MSZIP:
>   case NDR_COMPRESSION_XPRESS:
>   break;
> @@ -507,6 +769,10 @@ enum ndr_err_code ndr_push_compression_end(struct ndr_push *subndr,
>   ndrpull->offset = 0;
>  
>   switch (compression_alg) {
> + case NDR_COMPRESSION_MSZIP_CAB:
> + NDR_CHECK(ndr_push_compression_mszip_cab_chunk(subndr, ndrpull, subndr->cstate));
> + break;
> +
>   case NDR_COMPRESSION_MSZIP:
>   ZERO_STRUCT(z);
>   while (!last) {
> diff --git a/source4/torture/ndr/cabinet.c b/source4/torture/ndr/cabinet.c
> index 790f7c37f11..f197534bb4a 100644
> --- a/source4/torture/ndr/cabinet.c
> +++ b/source4/torture/ndr/cabinet.c
> @@ -4233,10 +4233,9 @@ static bool cab_file_MSZIP_check(struct torture_context *tctx,
>  
>   blob = data_blob(NULL, r->cfdata[0].cbUncomp);
>   memset(blob.data, 'A', blob.length);
> -#if 0
> - /* once we have MSZIP compression working we can enable this test */
> +
>   torture_assert_data_blob_equal(tctx, r->cfdata[0].ab, blob, "ab");
> -#endif
> +
>   return true;
>  }
>  
> --
> 2.12.3
>
>
> From b5d2b53792a8311687a11a7f34d9e5cf5076a68d Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?G=C3=BCnther=20Deschner?= <[hidden email]>
> Date: Tue, 23 May 2017 15:50:55 +0200
> Subject: [PATCH 14/15] s4-torture: point out why we cannot validate MSZIP
>  compressed files
>
> Guenther
>
> Signed-off-by: Guenther Deschner <[hidden email]>
> ---
>  source4/torture/ndr/cabinet.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/source4/torture/ndr/cabinet.c b/source4/torture/ndr/cabinet.c
> index f197534bb4a..8cdf4cb4930 100644
> --- a/source4/torture/ndr/cabinet.c
> +++ b/source4/torture/ndr/cabinet.c
> @@ -4318,5 +4318,12 @@ struct torture_suite *ndr_cabinet_suite(TALLOC_CTX *ctx)
>  
>   torture_suite_add_ndr_pull_validate_test(suite, cab_file, cab_file_plain_data, cab_file_plain_check);
>  
> + /*
> + * we cannot validate, as libz' compression routines currently create a
> + * slightly different result
> + */
> +
> + /* torture_suite_add_ndr_pull_validate_test(suite, cab_file, cab_file_MSZIP_data, cab_file_MSZIP_check); */
> +
>   return suite;
>  }
> --
> 2.12.3
>
>
> From 4fa9daee19bf7b2023fc2e59b2e4b97e251211ef Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <[hidden email]>
> Date: Tue, 11 Jul 2017 09:41:08 +0200
> Subject: [PATCH 15/15] s3:client: Only do krb5 auth in smbspool if cache
>  exists
>
> Signed-off-by: Andreas Schneider <[hidden email]>
> ---
>  source3/client/smbspool.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/source3/client/smbspool.c b/source3/client/smbspool.c
> index 49241c73bf1..c293e37149b 100644
> --- a/source3/client/smbspool.c
> +++ b/source3/client/smbspool.c
> @@ -503,6 +503,7 @@ smb_connect(const char *workgroup, /* I - Workgroup */
>   struct cli_state *cli; /* New connection */
>   char           *myname = NULL; /* Client name */
>   struct passwd  *pwd;
> + bool do_password_auth = false;
>  
>   /*
>           * Get the names and addresses of the client and server...
> @@ -517,7 +518,23 @@ smb_connect(const char *workgroup, /* I - Workgroup */
>   * behavior with 3.0.14a
>   */
>  
> - if (username && *username && !getenv("KRB5CCNAME")) {
> + if (username != NULL && username[0] != '\0') {
> + const char *env = getenv("KRB5CCNAME");
> +
> + if (env == NULL || env[0] == '\0') {
> + do_password_auth = true;
> + } else {
> + struct stat sb;
> + int rc;
> +
> + rc = stat(env, &sb);
> + if (rc == 0) {
> + do_password_auth = true;
> + }
> + }
> + }
> +
> + if (do_password_auth) {
>   cli = smb_complete_connection(myname, server, port, username,
>      password, workgroup, share, 0, need_auth);
>   if (cli) {
> --
> 2.12.3
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, Jul 19, 2017 at 04:08:19PM +0200, Aurélien Aptel wrote:

> Jeremy Allison <[hidden email]> writes:
> > In ndr_push_folder_cfdata()
> >
> > +                       size_t old_off = ndr->offset;
> > +                       /* just copy the data */
> >
> > Here you push the data.
> >
> > +                       NDR_CHECK(ndr_push_bytes(ndr, r->ab.data, r->ab.length));
> >
> > Then you do the overflow check. That's backwards.
>
> > ...
>
> > When you do a ndr_push_bytes(ndr, data, length) you *KNOW*
> > it's going to advance ndr->offset by length bytes.
> >
> > So in that case do the wrap checks *before* the push.
>
> I guess there's a misunderstanding, that's literally what you asked me
> to do previously:
>
> > 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).
>
> I also *know* NDR already checks for overflows with the *_NEED_BYTES().
>
> > Also, look at all the other wrap checks in Samba.
> > They should always look like:
> >
> > if (a + b < a) {
> > error
> > }
>
> I think I'll use an extra NEED_BYTES() call here which I think is even
> less error prone and was made just for this purpose. Sounds good?

LGTM !!!!! Reviewed-by: Jeremy Allison <[hidden email]>

Pushed. Thanks a *LOT* for your patience on this.

Jeremy.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, Jul 19, 2017 at 04:08:19PM +0200, Aurélien Aptel wrote:

The patch below was tagged onto the end of the previous set of MS-ZIP cab
patches.

Doesn't look like it belongs to me so I didn't review/push.

Andreas/Aurélien, if it does belong can you repost as a
separate reqested patch.

Looks like it also needs a bug number if you need this
back-porting.

Cheers,

        Jeremy.

> From 4fa9daee19bf7b2023fc2e59b2e4b97e251211ef Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <[hidden email]>
> Date: Tue, 11 Jul 2017 09:41:08 +0200
> Subject: [PATCH 15/15] s3:client: Only do krb5 auth in smbspool if cache
>  exists
>
> Signed-off-by: Andreas Schneider <[hidden email]>
> ---
>  source3/client/smbspool.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/source3/client/smbspool.c b/source3/client/smbspool.c
> index 49241c73bf1..c293e37149b 100644
> --- a/source3/client/smbspool.c
> +++ b/source3/client/smbspool.c
> @@ -503,6 +503,7 @@ smb_connect(const char *workgroup, /* I - Workgroup */
>   struct cli_state *cli; /* New connection */
>   char           *myname = NULL; /* Client name */
>   struct passwd  *pwd;
> + bool do_password_auth = false;
>  
>   /*
>           * Get the names and addresses of the client and server...
> @@ -517,7 +518,23 @@ smb_connect(const char *workgroup, /* I - Workgroup */
>   * behavior with 3.0.14a
>   */
>  
> - if (username && *username && !getenv("KRB5CCNAME")) {
> + if (username != NULL && username[0] != '\0') {
> + const char *env = getenv("KRB5CCNAME");
> +
> + if (env == NULL || env[0] == '\0') {
> + do_password_auth = true;
> + } else {
> + struct stat sb;
> + int rc;
> +
> + rc = stat(env, &sb);
> + if (rc == 0) {
> + do_password_auth = true;
> + }
> + }
> + }
> +
> + if (do_password_auth) {
>   cli = smb_complete_connection(myname, server, port, username,
>      password, workgroup, share, 0, need_auth);
>   if (cli) {
> --
> 2.12.3
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
On Wednesday, 19 July 2017 21:21:47 CEST Jeremy Allison wrote:

> On Wed, Jul 19, 2017 at 04:08:19PM +0200, Aurélien Aptel wrote:
>
> The patch below was tagged onto the end of the previous set of MS-ZIP cab
> patches.
>
> Doesn't look like it belongs to me so I didn't review/push.
>
> Andreas/Aurélien, if it does belong can you repost as a
> separate reqested patch.
>
> Looks like it also needs a bug number if you need this
> back-porting.

This just ended up somehow in my cabinet file branch.

It is part of my smbspool patchset which also adds tests for smbspool. However
autobuild stopped working with it.


Thanks Aurélien for your work!


        Andreas


--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCHES] MSZIP support for cabinet files

Samba - samba-technical mailing list
Thanks for merging.

Some updates about fuzzing: I've let AFL run for 4 days and it found no
crash so far which I know doesn't prove anything but conforts me that
the pull code is probably safe. A better test would be to fuzz that +
the thing that actually uses the resulting pulled cab structure.

--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Loading...