Attachment 10909 Details for Bug 11125 – Fix handling of broken AppleDouble files

[patch] Fix handling of broken AppleDouble files

fruit.patch (text/plain), 4.69 KB, created by Ralph Böhme on 2015-03-25 14:33:49 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ralph Böhme

Created: 2015-03-25 14:33:49 UTC

Size: 4.69 KB

patch

obsolete

>From b7a523aa014cacb0c41791f7d5a80e2b18e35755 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Mon, 2 Mar 2015 18:15:06 +0100
>Subject: [PATCH] vfs_fruit: enhance handling of malformed AppleDouble files
>
>Trying for fixup a broken AppleDouble file with a resourcefork entry
>offset + length > filesystem resulted in a crashing memmove() in
>ad_convert().
>
>Add a specific safety check that stats the ._ file and limits the
>resource fork length to the filesize.
>
>While we're at it, now that we know the filesize in ad_unpack(), add
>additional checks that verify this.
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=11125
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>---
> source3/modules/vfs_fruit.c | 79 +++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 73 insertions(+), 6 deletions(-)
>
>diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c
>index fbee321..28578df 100644
>--- a/source3/modules/vfs_fruit.c
>+++ b/source3/modules/vfs_fruit.c
>@@ -569,7 +569,7 @@ static bool ad_pack(struct adouble *ad)
> /**
>  * Unpack an AppleDouble blob into a struct adoble
>  **/
>-static bool ad_unpack(struct adouble *ad, const int nentries)
>+static bool ad_unpack(struct adouble *ad, const int nentries, size_t filesize)
> {
> 	size_t bufsize = talloc_get_size(ad->ad_data);
> 	int adentries, i;
>@@ -612,11 +612,26 @@ static bool ad_unpack(struct adouble *ad, const int nentries)
> 			return false;
> 		}
> 
>+		/*
>+		 * All entries other then the resource fork are
>+		 * expected to be read into the ad_data buffer, so
>+		 * ensure the specified offset is within that bound
>+		 */
> 		if ((off > bufsize) && (eid != ADEID_RFORK)) {
> 			DEBUG(1, ("bogus eid %d: off: %" PRIu32 ", len: %" PRIu32 "\n",
> 				  eid, off, len));
> 			return false;
> 		}
>+
>+		/*
>+		 * All entries besides FiderInfo and resource fork
>+		 * must fit into the buffer. FinderInfo is special as
>+		 * it may be larger then the default 32 bytes (if it
>+		 * contains marshalled xattrs), but we will fixup that
>+		 * in ad_convert(). And the resource fork is never
>+		 * accessed directly by the ad_data buf (also see
>+		 * comment above) anyway.
>+		 */
> 		if ((eid != ADEID_RFORK) &&
> 		    (eid != ADEID_FINDERI) &&
> 		    ((off + len) > bufsize)) {
>@@ -625,6 +640,48 @@ static bool ad_unpack(struct adouble *ad, const int nentries)
> 			return false;
> 		}
> 
>+		/*
>+		 * That would be obviously broken
>+		 */
>+		if (off > filesize) {
>+			DEBUG(1, ("bogus eid %d: off: %" PRIu32 ", len: %" PRIu32 "\n",
>+				  eid, off, len));
>+			return false;
>+		}
>+
>+		/*
>+		 * Check for any entry that has its end beyond the
>+		 * filesize.
>+		 */
>+		if (off + len < off) {
>+			DEBUG(1, ("offset wrap in eid %d: off: %" PRIu32
>+				  ", len: %" PRIu32 "\n",
>+				  eid, off, len));
>+			return false;
>+
>+		}
>+		if (off + len > filesize) {
>+			/*
>+			 * If this is the resource fork entry, we fix
>+			 * up the length, for any other entry we bail
>+			 * out.
>+			 */
>+			if (eid != ADEID_RFORK) {
>+				DEBUG(1, ("bogus eid %d: off: %" PRIu32
>+					  ", len: %" PRIu32 "\n",
>+					  eid, off, len));
>+				return false;
>+			}
>+
>+			/*
>+			 * Fixup the resource fork entry by limiting
>+			 * the size to entryoffset - filesize.
>+			 */
>+			len = filesize - off;
>+			DEBUG(1, ("Limiting ADEID_RFORK: off: %" PRIu32
>+				  ", len: %" PRIu32 "\n", off, len));
>+		}
>+
> 		ad->ad_eid[eid].ade_off = off;
> 		ad->ad_eid[eid].ade_len = len;
> 	}
>@@ -659,9 +716,11 @@ static int ad_convert(struct adouble *ad, int fd)
> 		goto exit;
> 	}
> 
>-	memmove(map + ad_getentryoff(ad, ADEID_FINDERI) + ADEDLEN_FINDERI,
>-		map + ad_getentryoff(ad, ADEID_RFORK),
>-		ad_getentrylen(ad, ADEID_RFORK));
>+	if (ad_getentrylen(ad, ADEID_RFORK) > 0) {
>+		memmove(map + ad_getentryoff(ad, ADEID_FINDERI) + ADEDLEN_FINDERI,
>+			map + ad_getentryoff(ad, ADEID_RFORK),
>+			ad_getentrylen(ad, ADEID_RFORK));
>+	}
> 
> 	ad_setentrylen(ad, ADEID_FINDERI, ADEDLEN_FINDERI);
> 	ad_setentryoff(ad, ADEID_RFORK,
>@@ -719,7 +778,7 @@ static ssize_t ad_header_read_meta(struct adouble *ad, const char *path)
> 	}
> 
> 	/* Now parse entries */
>-	ok = ad_unpack(ad, ADEID_NUM_XATTR);
>+	ok = ad_unpack(ad, ADEID_NUM_XATTR, AD_DATASZ_XATTR);
> 	if (!ok) {
> 		DEBUG(2, ("invalid AppleDouble metadata xattr\n"));
> 		errno = EINVAL;
>@@ -847,8 +906,16 @@ static ssize_t ad_header_read_rsrc(struct adouble *ad, const char *path)
> 			goto exit;
> 		}
> 
>+		/* FIXME: direct sys_fstat(), we don't have an fsp */
>+		rc = sys_fstat(fd, &sbuf,
>+			       lp_fake_directory_create_times(
>+				       SNUM(ad->ad_handle->conn)));
>+		if (rc != 0) {
>+			goto exit;
>+		}
>+
> 		/* Now parse entries */
>-		ok = ad_unpack(ad, ADEID_NUM_DOT_UND);
>+		ok = ad_unpack(ad, ADEID_NUM_DOT_UND, sbuf.st_ex_size);
> 		if (!ok) {
> 			DEBUG(1, ("invalid AppleDouble ressource %s\n", path));
> 			errno = EINVAL;
>-- 
>2.1.0
>

Flags:

vl: review+

Actions: View

Attachments on bug 11125: 10801 | 10803 | 10806 | 10908 | 10909 | 10913