From 1bb482b9e03521057db77aa0923a69463ca2901e Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 21 Jan 2022 17:52:52 +0000 Subject: [PATCH 1/5] vfs_fruit: replace unsafe ad_entry macro with a function BUG: https://bugzilla.samba.org/show_bug.cgi?id=12427 Signed-off-by: Ralph Boehme Reviewed-by: Uri Simchoni (cherry picked from commit 3d5bf4b85f3ca120206a12b3d102aef2ead33082) --- source3/modules/vfs_fruit.c | 105 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 85 insertions(+), 20 deletions(-) diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index cb0d2848fa3..01269bd8f74 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -342,7 +342,6 @@ typedef enum {ADOUBLE_META, ADOUBLE_RSRC} adouble_type_t; #define ad_getentryoff(ad,eid) ((ad)->ad_eid[(eid)].ade_off) #define ad_setentrylen(ad,eid,len) ((ad)->ad_eid[(eid)].ade_len = (len)) #define ad_setentryoff(ad,eid,off) ((ad)->ad_eid[(eid)].ade_off = (off)) -#define ad_entry(ad,eid) ((ad)->ad_data + ad_getentryoff((ad),(eid))) struct ad_entry { size_t ade_off; @@ -413,6 +412,23 @@ static int ad_write(struct adouble *ad, const char *path); static int adouble_path(TALLOC_CTX *ctx, const char *path_in, char **path_out); /** + * Return a pointer to an AppleDouble entry + * + * Returns NULL if the entry is not present + **/ +static char *ad_get_entry(const struct adouble *ad, int eid) +{ + off_t off = ad_getentryoff(ad, eid); + size_t len = ad_getentrylen(ad, eid); + + if (off == 0 || len == 0) { + return NULL; + } + + return ad->ad_data + off; +} + +/** * Get a date **/ static int ad_getdate(const struct adouble *ad, @@ -420,18 +436,19 @@ static int ad_getdate(const struct adouble *ad, uint32_t *date) { bool xlate = (dateoff & AD_DATE_UNIX); + char *p = NULL; dateoff &= AD_DATE_MASK; - if (!ad_getentryoff(ad, ADEID_FILEDATESI)) { + p = ad_get_entry(ad, ADEID_FILEDATESI); + if (p == NULL) { return -1; } if (dateoff > AD_DATE_ACCESS) { return -1; } - memcpy(date, - ad_entry(ad, ADEID_FILEDATESI) + dateoff, - sizeof(uint32_t)); + + memcpy(date, p + dateoff, sizeof(uint32_t)); if (xlate) { *date = AD_DATE_TO_UNIX(*date); @@ -445,9 +462,11 @@ static int ad_getdate(const struct adouble *ad, static int ad_setdate(struct adouble *ad, unsigned int dateoff, uint32_t date) { bool xlate = (dateoff & AD_DATE_UNIX); + char *p = NULL; - if (!ad_getentryoff(ad, ADEID_FILEDATESI)) { - return 0; + p = ad_get_entry(ad, ADEID_FILEDATESI); + if (p == NULL) { + return -1; } dateoff &= AD_DATE_MASK; @@ -459,7 +478,7 @@ static int ad_setdate(struct adouble *ad, unsigned int dateoff, uint32_t date) return -1; } - memcpy(ad_entry(ad, ADEID_FILEDATESI) + dateoff, &date, sizeof(date)); + memcpy(p + dateoff, &date, sizeof(date)); return 0; } @@ -943,6 +962,9 @@ static ssize_t ad_header_read_rsrc(struct adouble *ad, const char *path) if ((mode == O_RDWR) && (ad_getentrylen(ad, ADEID_FINDERI) > ADEDLEN_FINDERI)) { + char *p_ad = NULL; + char *p_meta_ad = NULL; + rc = ad_convert(ad, fd); if (rc != 0) { rc = -1; @@ -972,9 +994,18 @@ static ssize_t ad_header_read_rsrc(struct adouble *ad, const char *path) goto exit; } - memcpy(ad_entry(meta_ad, ADEID_FINDERI), - ad_entry(ad, ADEID_FINDERI), - ADEDLEN_FINDERI); + p_ad = ad_get_entry(ad, ADEID_FINDERI); + if (p_ad == NULL) { + rc = -1; + goto exit; + } + p_meta_ad = ad_get_entry(meta_ad, ADEID_FINDERI); + if (p_meta_ad == NULL) { + rc = -1; + goto exit; + } + + memcpy(p_meta_ad, p_ad, ADEDLEN_FINDERI); rc = ad_write(meta_ad, path); if (rc != 0) { @@ -1559,8 +1590,15 @@ static bool empty_finderinfo(const struct adouble *ad) { char emptybuf[ADEDLEN_FINDERI] = {0}; + char *fi = NULL; + + fi = ad_get_entry(ad, ADEID_FINDERI); + if (fi == NULL) { + DBG_ERR("Missing FinderInfo in struct adouble [%p]\n", ad); + return false; + } if (memcmp(emptybuf, - ad_entry(ad, ADEID_FINDERI), + fi, ADEDLEN_FINDERI) == 0) { return true; } @@ -1973,23 +2011,32 @@ static NTSTATUS readdir_attr_macmeta(struct vfs_handle_struct *handle, ad = ad_get(talloc_tos(), handle, smb_fname->base_name, ADOUBLE_META); if (ad) { + char *p = ad_get_entry(ad, ADEID_FINDERI); + + if (p == NULL) { + DBG_ERR("No ADEID_FINDERI for [%s]\n", + smb_fname->base_name); + status = NT_STATUS_INVALID_PARAMETER; + goto out; + } + if (S_ISREG(smb_fname->st.st_ex_mode)) { /* finder_type */ memcpy(&attr_data->attr_data.aapl.finder_info[0], - ad_entry(ad, ADEID_FINDERI), 4); + p, 4); /* finder_creator */ memcpy(&attr_data->attr_data.aapl.finder_info[0] + 4, - ad_entry(ad, ADEID_FINDERI) + 4, 4); + p + 4, 4); } /* finder_flags */ memcpy(&attr_data->attr_data.aapl.finder_info[0] + 8, - ad_entry(ad, ADEID_FINDERI) + 8, 2); + p + 8, 2); /* finder_ext_flags */ memcpy(&attr_data->attr_data.aapl.finder_info[0] + 10, - ad_entry(ad, ADEID_FINDERI) + 24, 2); + p + 24, 2); /* creation date */ date_added = convert_time_t_to_uint32_t( @@ -2000,6 +2047,7 @@ static NTSTATUS readdir_attr_macmeta(struct vfs_handle_struct *handle, } } +out: TALLOC_FREE(ad); return status; } @@ -2679,6 +2727,7 @@ static ssize_t fruit_pread(vfs_handle_struct *handle, if (ad->ad_type == ADOUBLE_META) { char afpinfo_buf[AFP_INFO_SIZE]; + char *p = NULL; size_t to_return; /* @@ -2707,9 +2756,16 @@ static ssize_t fruit_pread(vfs_handle_struct *handle, goto exit; } - memcpy(&ai->afpi_FinderInfo[0], - ad_entry(ad, ADEID_FINDERI), - ADEDLEN_FINDERI); + p = ad_get_entry(ad, ADEID_FINDERI); + if (p == NULL) { + DBG_ERR("No ADEID_FINDERI for [%s]\n", + fsp->fsp_name->base_name); + rc = -1; + goto exit; + } + + memcpy(&ai->afpi_FinderInfo[0], p, ADEDLEN_FINDERI); + len = afpinfo_pack(ai, afpinfo_buf); if (len != AFP_INFO_SIZE) { rc = -1; @@ -2797,6 +2853,7 @@ static ssize_t fruit_pwrite(vfs_handle_struct *handle, } if (ad->ad_type == ADOUBLE_META) { + char * p = NULL; if (n != AFP_INFO_SIZE || offset != 0) { DEBUG(1, ("unexpected offset=%jd or size=%jd\n", (intmax_t)offset, (intmax_t)n)); @@ -2808,7 +2865,15 @@ static ssize_t fruit_pwrite(vfs_handle_struct *handle, rc = -1; goto exit; } - memcpy(ad_entry(ad, ADEID_FINDERI), + p = ad_get_entry(ad, ADEID_FINDERI); + if (p == NULL) { + DBG_ERR("No ADEID_FINDERI for [%s]\n", + fsp->fsp_name->base_name); + rc = -1; + goto exit; + } + + memcpy(p, &ai->afpi_FinderInfo[0], ADEDLEN_FINDERI); if (empty_finderinfo(ad)) { /* Discard metadata */ -- 2.12.3 From 306869688b42cdaf59e0a5474d033d73a04e2950 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 13 Jan 2022 16:48:01 +0100 Subject: [PATCH 2/5] CVE-2021-44142: libadouble: add defines for icon lengths From https://www.ietf.org/rfc/rfc1740.txt BUG: https://bugzilla.samba.org/show_bug.cgi?id=14914 Signed-off-by: Ralph Boehme --- source3/modules/vfs_fruit.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index 01269bd8f74..4148e86b5df 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -258,6 +258,8 @@ typedef enum {ADOUBLE_META, ADOUBLE_RSRC} adouble_type_t; #define ADEDLEN_MACFILEI 4 #define ADEDLEN_PRODOSFILEI 8 #define ADEDLEN_MSDOSFILEI 2 +#define ADEDLEN_ICONBW 128 +#define ADEDLEN_ICONCOL 1024 #define ADEDLEN_DID 4 #define ADEDLEN_PRIVDEV 8 #define ADEDLEN_PRIVINO 8 -- 2.12.3 From 8d03fc781354e7abb0361afbc5ea4d6f26a1d12e Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sat, 20 Nov 2021 16:36:42 +0100 Subject: [PATCH 3/5] CVE-2021-44142: smbd: add Netatalk xattr used by vfs_fruit to the list of private Samba xattrs This is an internal xattr that should not be user visible. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14914 Signed-off-by: Ralph Boehme [slow@samba.org: conflict due to changed includes in source3/smbd/trans2.c] --- source3/smbd/trans2.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c index b65f5814fab..698689750d4 100644 --- a/source3/smbd/trans2.c +++ b/source3/smbd/trans2.c @@ -154,6 +154,16 @@ uint64_t get_FileIndex(connection_struct *conn, const SMB_STRUCT_STAT *psbuf) Refuse to allow clients to overwrite our private xattrs. ****************************************************************************/ +/* + * Taken from vfs_fruit.c + */ +#define NETATALK_META_XATTR "org.netatalk.Metadata" +#if defined(HAVE_ATTROPEN) +#define AFPINFO_EA_NETATALK NETATALK_META_XATTR +#else +#define AFPINFO_EA_NETATALK "user." NETATALK_META_XATTR +#endif + bool samba_private_attr_name(const char *unix_ea_name) { static const char * const prohibited_ea_names[] = { @@ -161,6 +171,7 @@ bool samba_private_attr_name(const char *unix_ea_name) SAMBA_XATTR_DOS_ATTRIB, SAMBA_XATTR_MARKER, XATTR_NTACL_NAME, + AFPINFO_EA_NETATALK, NULL }; -- 2.12.3 From a8183661cc40f3e91bbfa3aa18d8253e3acf6058 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 13 Jan 2022 17:03:02 +0100 Subject: [PATCH 4/5] CVE-2021-44142: libadouble: harden parsing code BUG: https://bugzilla.samba.org/show_bug.cgi?id=14914 Signed-off-by: Ralph Boehme --- source3/modules/vfs_fruit.c | 117 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 102 insertions(+), 15 deletions(-) diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index 4148e86b5df..e7a30d7604b 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -413,6 +413,95 @@ static struct adouble *ad_init(TALLOC_CTX *ctx, vfs_handle_struct *handle, static int ad_write(struct adouble *ad, const char *path); static int adouble_path(TALLOC_CTX *ctx, const char *path_in, char **path_out); +/* + * All entries besides FinderInfo 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, which we will fixup that in + * ad_convert(). The first 32 bytes however must also be part of the buffer. + * + * The resource fork is never accessed directly by the ad_data buf. + */ +static bool ad_entry_check_size(uint32_t eid, + size_t bufsize, + uint32_t off, + uint32_t got_len) +{ + struct { + off_t expected_len; + bool fixed_size; + bool minimum_size; + } ad_checks[] = { + [ADEID_DFORK] = {-1, false, false}, /* not applicable */ + [ADEID_RFORK] = {-1, false, false}, /* no limit */ + [ADEID_NAME] = {ADEDLEN_NAME, false, false}, + [ADEID_COMMENT] = {ADEDLEN_COMMENT, false, false}, + [ADEID_ICONBW] = {ADEDLEN_ICONBW, true, false}, + [ADEID_ICONCOL] = {ADEDLEN_ICONCOL, false, false}, + [ADEID_FILEI] = {ADEDLEN_FILEI, true, false}, + [ADEID_FILEDATESI] = {ADEDLEN_FILEDATESI, true, false}, + [ADEID_FINDERI] = {ADEDLEN_FINDERI, false, true}, + [ADEID_MACFILEI] = {ADEDLEN_MACFILEI, true, false}, + [ADEID_PRODOSFILEI] = {ADEDLEN_PRODOSFILEI, true, false}, + [ADEID_MSDOSFILEI] = {ADEDLEN_MSDOSFILEI, true, false}, + [ADEID_SHORTNAME] = {ADEDLEN_SHORTNAME, false, false}, + [ADEID_AFPFILEI] = {ADEDLEN_AFPFILEI, true, false}, + [ADEID_DID] = {ADEDLEN_DID, true, false}, + [ADEID_PRIVDEV] = {ADEDLEN_PRIVDEV, true, false}, + [ADEID_PRIVINO] = {ADEDLEN_PRIVINO, true, false}, + [ADEID_PRIVSYN] = {ADEDLEN_PRIVSYN, true, false}, + [ADEID_PRIVID] = {ADEDLEN_PRIVID, true, false}, + }; + + if (eid >= ADEID_MAX) { + return false; + } + if (got_len == 0) { + /* Entry present, but empty, allow */ + return true; + } + if (ad_checks[eid].expected_len == 0) { + /* + * Shouldn't happen: implicitly initialized to zero because + * explicit initializer missing. + */ + return false; + } + if (ad_checks[eid].expected_len == -1) { + /* Unused or no limit */ + return true; + } + if (ad_checks[eid].fixed_size) { + if (ad_checks[eid].expected_len != got_len) { + /* Wrong size fo fixed size entry. */ + return false; + } + } else { + if (ad_checks[eid].minimum_size) { + if (got_len < ad_checks[eid].expected_len) { + /* + * Too small for variable sized entry with + * minimum size. + */ + return false; + } + } else { + if (got_len > ad_checks[eid].expected_len) { + /* Too big for variable sized entry. */ + return false; + } + } + } + if (off + got_len < off) { + /* wrap around */ + return false; + } + if (off + got_len > bufsize) { + /* overflow */ + return false; + } + return true; +} + /** * Return a pointer to an AppleDouble entry * @@ -420,8 +509,15 @@ static int adouble_path(TALLOC_CTX *ctx, const char *path_in, char **path_out); **/ static char *ad_get_entry(const struct adouble *ad, int eid) { + size_t bufsize = talloc_get_size(ad->ad_data); off_t off = ad_getentryoff(ad, eid); size_t len = ad_getentrylen(ad, eid); + bool valid; + + valid = ad_entry_check_size(eid, bufsize, off, len); + if (!valid) { + return NULL; + } if (off == 0 || len == 0) { return NULL; @@ -485,7 +581,6 @@ static int ad_setdate(struct adouble *ad, unsigned int dateoff, uint32_t date) return 0; } - /** * Map on-disk AppleDouble id to enumerated id **/ @@ -629,6 +724,7 @@ static bool ad_unpack(struct adouble *ad, const int nentries, size_t filesize) /* now, read in the entry bits */ for (i = 0; i < adentries; i++) { + bool ok; eid = RIVAL(ad->ad_data, AD_HEADER_LEN + (i * AD_ENTRY_LEN)); eid = get_eid(eid); off = RIVAL(ad->ad_data, AD_HEADER_LEN + (i * AD_ENTRY_LEN) + 4); @@ -650,20 +746,11 @@ static bool ad_unpack(struct adouble *ad, const int nentries, size_t filesize) return false; } - /* - * All entries besides FinderInfo 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)) { - DEBUG(1, ("bogus eid %d: off: %" PRIu32 ", len: %" PRIu32 "\n", - eid, off, len)); + ok = ad_entry_check_size(eid, bufsize, off, len); + if (!ok) { + DBG_ERR("bogus eid [%"PRIu32"] bufsize [%zu] " + "off [%"PRIu32"] len [%"PRIu32"]\n", + eid, bufsize, off, len); return false; } -- 2.12.3 From 660d6a200961e39bfcfe093c1e37a9b622a7636a Mon Sep 17 00:00:00 2001 From: Noel Power Date: Fri, 21 Jan 2022 18:52:43 +0000 Subject: [PATCH 5/5] squash: Adjust previous patch to cater for non handling of emedded xattrs --- source3/modules/vfs_fruit.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index e7a30d7604b..c65520c1ea9 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -495,6 +495,16 @@ static bool ad_entry_check_size(uint32_t eid, /* wrap around */ return false; } + /* + * This version of samba doesn't + * support marshalled xattrs for FinderInfo + * but the buffer *must* support the first 32 bytes + * any extra length of this entry will be discarded (along with + * the embedded xattrs) + */ + if (eid == ADEID_FINDERI) { + got_len = ADEDLEN_FINDERI; + } if (off + got_len > bufsize) { /* overflow */ return false; -- 2.12.3