[PATCHES] MSZIP support for cabinet files

Jeremy Allison jra at samba.org
Tue Jul 18 23:51:03 UTC 2017


On Mon, Jul 17, 2017 at 05:55:43PM +0200, Aurélien Aptel wrote:
> Günther Deschner <gd at samba.org> 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 <aaptel at suse.com>
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;




More information about the samba-technical mailing list