From b7a523aa014cacb0c41791f7d5a80e2b18e35755 Mon Sep 17 00:00:00 2001 From: Ralph Boehme 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 --- 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