From 592aca7ac48947ff264ff2f24980a22863c644fb Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 13 Jan 2022 16:48:01 +0100 Subject: [PATCH 1/6] 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 7ae51ee3f6f..56ba82142ed 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -277,6 +277,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.31.1 From 0c9e24ea2abb1882d74cf705dd4c692eb1705adb Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sat, 20 Nov 2021 16:36:42 +0100 Subject: [PATCH 2/6] 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 ec7a5fa99d5..24f65a37044 100644 --- a/source3/smbd/trans2.c +++ b/source3/smbd/trans2.c @@ -173,6 +173,16 @@ void aapl_force_zero_file_id(struct smbd_server_connection *sconn) 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[] = { @@ -180,6 +190,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.31.1 From d9cfe712fed17e0f031e3955a04a712a12a31c26 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 26 Nov 2021 07:19:32 +0100 Subject: [PATCH 3/6] CVE-2021-44142: libadouble: harden ad_unpack_xattrs() This ensures ad_unpack_xattrs() is only called for an ad_type of ADOUBLE_RSRC, which is used for parsing ._ AppleDouble sidecar files, and the buffer ad->ad_data is AD_XATTR_MAX_HDR_SIZE bytes large which is a prerequisite for all buffer out-of-bounds access checks in ad_unpack_xattrs(). BUG: https://bugzilla.samba.org/show_bug.cgi?id=14914 Signed-off-by: Ralph Boehme --- source3/modules/vfs_fruit.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index 56ba82142ed..8fd2eb09bc7 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -670,14 +670,27 @@ static bool ad_pack(struct adouble *ad) static bool ad_unpack_xattrs(struct adouble *ad) { struct ad_xattr_header *h = &ad->adx_header; + size_t bufsize = talloc_get_size(ad->ad_data); const char *p = ad->ad_data; uint32_t hoff; uint32_t i; + if (ad->ad_type != ADOUBLE_RSRC) { + return false; + } + if (ad_getentrylen(ad, ADEID_FINDERI) <= ADEDLEN_FINDERI) { return true; } + /* + * Ensure the buffer ad->ad_data was allocated by ad_alloc() for an + * ADOUBLE_RSRC type (._ AppleDouble file on-disk). + */ + if (bufsize != AD_XATTR_MAX_HDR_SIZE) { + return false; + } + /* 2 bytes padding */ hoff = ad_getentryoff(ad, ADEID_FINDERI) + ADEDLEN_FINDERI + 2; @@ -925,11 +938,12 @@ static bool ad_unpack(struct adouble *ad, const size_t nentries, ad->ad_eid[eid].ade_len = len; } - ok = ad_unpack_xattrs(ad); - if (!ok) { - return false; + if (ad->ad_type == ADOUBLE_RSRC) { + ok = ad_unpack_xattrs(ad); + if (!ok) { + return false; + } } - return true; } -- 2.31.1 From d5f8a6f423f6bfba706d57459d78046920d61ce5 Mon Sep 17 00:00:00 2001 From: Noel Power Date: Fri, 21 Jan 2022 14:52:53 +0000 Subject: [PATCH 4/6] vfs_fruit: CVE-2021-44142 tweak buffer size check --- source3/modules/vfs_fruit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index 8fd2eb09bc7..7144b55083f 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -687,7 +687,7 @@ static bool ad_unpack_xattrs(struct adouble *ad) * Ensure the buffer ad->ad_data was allocated by ad_alloc() for an * ADOUBLE_RSRC type (._ AppleDouble file on-disk). */ - if (bufsize != AD_XATTR_MAX_HDR_SIZE) { + if (bufsize < AD_DATASZ_DOT_UND || bufsize > AD_XATTR_MAX_HDR_SIZE) { return false; } -- 2.31.1 From ed9a8dcdb970962d0e9afbd52672e4295f3e31c6 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 25 Nov 2021 15:04:03 +0100 Subject: [PATCH 5/6] CVE-2021-44142: libadouble: add basic cmocka tests BUG: https://bugzilla.samba.org/show_bug.cgi?id=14914 Signed-off-by: Ralph Boehme [slow@samba.org: conflict due to missing test in selftest/tests.py] --- selftest/knownfail.d/samba.unittests.adouble | 3 + selftest/tests.py | 2 + source3/lib/test_adouble.c | 386 +++++++++++++++++++ source3/wscript_build | 5 + 4 files changed, 396 insertions(+) create mode 100644 selftest/knownfail.d/samba.unittests.adouble create mode 100644 source3/lib/test_adouble.c diff --git a/selftest/knownfail.d/samba.unittests.adouble b/selftest/knownfail.d/samba.unittests.adouble new file mode 100644 index 00000000000..8b0314f2fae --- /dev/null +++ b/selftest/knownfail.d/samba.unittests.adouble @@ -0,0 +1,3 @@ +^samba.unittests.adouble.parse_abouble_finderinfo2\(none\) +^samba.unittests.adouble.parse_abouble_finderinfo3\(none\) +^samba.unittests.adouble.parse_abouble_date2\(none\) diff --git a/selftest/tests.py b/selftest/tests.py index eabe71401fc..277d9040855 100644 --- a/selftest/tests.py +++ b/selftest/tests.py @@ -134,3 +134,5 @@ plantestsuite("wafsamba.duplicate_symbols", "none", [os.path.join(srcdir(), "bui if with_cmocka: plantestsuite("samba.unittests.krb5samba", "none", [os.path.join(bindir(), "default/testsuite/unittests/test_krb5samba")]) + plantestsuite("samba.unittests.adouble", "none", + [os.path.join(bindir(), "test_adouble")]) diff --git a/source3/lib/test_adouble.c b/source3/lib/test_adouble.c new file mode 100644 index 00000000000..1cebb875789 --- /dev/null +++ b/source3/lib/test_adouble.c @@ -0,0 +1,386 @@ +/* + * Unix SMB/CIFS implementation. + * + * Copyright (C) 2021 Ralph Boehme + * + * 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 . + */ + +#include "vfs_fruit.c" +#include + +static int setup_talloc_context(void **state) +{ + TALLOC_CTX *frame = talloc_stackframe(); + + *state = frame; + return 0; +} + +static int teardown_talloc_context(void **state) +{ + TALLOC_CTX *frame = *state; + + TALLOC_FREE(frame); + return 0; +} + +/* + * Basic and sane buffer. + */ +static uint8_t ad_basic[] = { + 0x00, 0x05, 0x16, 0x07, /* Magic */ + 0x00, 0x02, 0x00, 0x00, /* Version */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x02, /* Count */ + /* adentry 1: FinderInfo */ + 0x00, 0x00, 0x00, 0x09, /* eid: FinderInfo */ + 0x00, 0x00, 0x00, 0x32, /* offset */ + 0x00, 0x00, 0x00, 0x20, /* length */ + /* adentry 2: Resourcefork */ + 0x00, 0x00, 0x00, 0x02, /* eid: Resourcefork */ + 0x00, 0x00, 0x00, 0x52, /* offset */ + 0xff, 0xff, 0xff, 0x00, /* length */ + /* FinderInfo data: 32 bytes */ + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, +}; + +/* + * An empty FinderInfo entry. + */ +static uint8_t ad_finderinfo1[] = { + 0x00, 0x05, 0x16, 0x07, /* Magic */ + 0x00, 0x02, 0x00, 0x00, /* Version */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x02, /* Count */ + /* adentry 1: FinderInfo */ + 0x00, 0x00, 0x00, 0x09, /* eid: FinderInfo */ + 0x00, 0x00, 0x00, 0x52, /* off: points at end of buffer */ + 0x00, 0x00, 0x00, 0x00, /* len: 0, so off+len don't exceed bufferlen */ + /* adentry 2: Resourcefork */ + 0x00, 0x00, 0x00, 0x02, /* eid: Resourcefork */ + 0x00, 0x00, 0x00, 0x52, /* offset */ + 0xff, 0xff, 0xff, 0x00, /* length */ + /* FinderInfo data: 32 bytes */ + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, +}; + +/* + * A dangerous FinderInfo with correct length exceeding buffer by one byte. + */ +static uint8_t ad_finderinfo2[] = { + 0x00, 0x05, 0x16, 0x07, /* Magic */ + 0x00, 0x02, 0x00, 0x00, /* Version */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x02, /* Count */ + /* adentry 1: FinderInfo */ + 0x00, 0x00, 0x00, 0x09, /* eid: FinderInfo */ + 0x00, 0x00, 0x00, 0x33, /* off: points at beginng of data + 1 */ + 0x00, 0x00, 0x00, 0x20, /* len: 32, so off+len exceeds bufferlen by 1 */ + /* adentry 2: Resourcefork */ + 0x00, 0x00, 0x00, 0x02, /* eid: Resourcefork */ + 0x00, 0x00, 0x00, 0x52, /* offset */ + 0xff, 0xff, 0xff, 0x00, /* length */ + /* FinderInfo data: 32 bytes */ + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, +}; + +static uint8_t ad_finderinfo3[] = { + 0x00, 0x05, 0x16, 0x07, /* Magic */ + 0x00, 0x02, 0x00, 0x00, /* Version */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x02, /* Count */ + /* adentry 1: FinderInfo */ + 0x00, 0x00, 0x00, 0x09, /* eid: FinderInfo */ + 0x00, 0x00, 0x00, 0x33, /* off: points at beginng of data + 1 */ + 0x00, 0x00, 0x00, 0x1f, /* len: 31, so off+len don't exceed buf */ + /* adentry 2: Resourcefork */ + 0x00, 0x00, 0x00, 0x02, /* eid: Resourcefork */ + 0x00, 0x00, 0x00, 0x52, /* offset */ + 0xff, 0xff, 0xff, 0x00, /* length */ + /* FinderInfo data: 32 bytes */ + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, +}; + +/* + * A dangerous name entry. + */ +static uint8_t ad_name[] = { + 0x00, 0x05, 0x16, 0x07, /* Magic */ + 0x00, 0x02, 0x00, 0x00, /* Version */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x02, /* Count */ + /* adentry 1: FinderInfo */ + 0x00, 0x00, 0x00, 0x09, /* eid: FinderInfo */ + 0x00, 0x00, 0x00, 0x32, /* offset */ + 0x00, 0x00, 0x00, 0x20, /* length */ + /* adentry 2: Name */ + 0x00, 0x00, 0x00, 0x03, /* eid: Name */ + 0x00, 0x00, 0x00, 0x52, /* off: points at end of buffer */ + 0x00, 0x00, 0x00, 0x01, /* len: 1, so off+len exceeds bufferlen */ + /* FinderInfo data: 32 bytes */ + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, +}; + +/* + * A empty ADEID_FILEDATESI entry. + */ +static uint8_t ad_date1[] = { + 0x00, 0x05, 0x16, 0x07, /* Magic */ + 0x00, 0x02, 0x00, 0x00, /* Version */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x02, /* Count */ + /* adentry 1: FinderInfo */ + 0x00, 0x00, 0x00, 0x09, /* eid: FinderInfo */ + 0x00, 0x00, 0x00, 0x32, /* offset */ + 0x00, 0x00, 0x00, 0x20, /* length */ + /* adentry 2: Dates */ + 0x00, 0x00, 0x00, 0x08, /* eid: dates */ + 0x00, 0x00, 0x00, 0x52, /* off: end of buffer */ + 0x00, 0x00, 0x00, 0x00, /* len: 0, empty entry, valid */ + /* FinderInfo data: 32 bytes */ + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, +}; + +/* + * A dangerous ADEID_FILEDATESI entry, invalid length. + */ +static uint8_t ad_date2[] = { + 0x00, 0x05, 0x16, 0x07, /* Magic */ + 0x00, 0x02, 0x00, 0x00, /* Version */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x00, 0x00, 0x00, /* Filler */ + 0x00, 0x02, /* Count */ + /* adentry 1: FinderInfo */ + 0x00, 0x00, 0x00, 0x09, /* eid: FinderInfo */ + 0x00, 0x00, 0x00, 0x32, /* offset */ + 0x00, 0x00, 0x00, 0x20, /* length */ + /* adentry 2: Dates */ + 0x00, 0x00, 0x00, 0x08, /* eid: dates */ + 0x00, 0x00, 0x00, 0x43, /* off: FinderInfo buf but one byte short */ + 0x00, 0x00, 0x00, 0x0f, /* len: 15, so off+len don't exceed bufferlen */ + /* FinderInfo data: 32 bytes */ + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, +}; + +static struct adouble *parse_adouble(TALLOC_CTX *mem_ctx, + uint8_t *adbuf, + size_t adsize, + off_t filesize) +{ + struct adouble *ad = NULL; + bool ok; + + ad = talloc_zero(mem_ctx, struct adouble); + ad->ad_data = talloc_zero_size(ad, adsize); + assert_non_null(ad); + + memcpy(ad->ad_data, adbuf, adsize); + + ok = ad_unpack(ad, 2, filesize); + if (!ok) { + return NULL; + } + + return ad; +} + +static void parse_abouble_basic(void **state) +{ + TALLOC_CTX *frame = *state; + struct adouble *ad = NULL; + char *p = NULL; + + ad = parse_adouble(frame, ad_basic, sizeof(ad_basic), 0xffffff52); + assert_non_null(ad); + + p = ad_get_entry(ad, ADEID_FINDERI); + assert_non_null(p); + + return; +} + +static void parse_abouble_finderinfo1(void **state) +{ + TALLOC_CTX *frame = *state; + struct adouble *ad = NULL; + char *p = NULL; + + ad = parse_adouble(frame, + ad_finderinfo1, + sizeof(ad_finderinfo1), + 0xffffff52); + assert_non_null(ad); + + p = ad_get_entry(ad, ADEID_FINDERI); + assert_null(p); + + return; +} + +static void parse_abouble_finderinfo2(void **state) +{ + TALLOC_CTX *frame = *state; + struct adouble *ad = NULL; + + ad = parse_adouble(frame, + ad_finderinfo2, + sizeof(ad_finderinfo2), + 0xffffff52); + assert_null(ad); + + return; +} + +static void parse_abouble_finderinfo3(void **state) +{ + TALLOC_CTX *frame = *state; + struct adouble *ad = NULL; + + ad = parse_adouble(frame, + ad_finderinfo3, + sizeof(ad_finderinfo3), + 0xffffff52); + assert_null(ad); + + return; +} + +static void parse_abouble_name(void **state) +{ + TALLOC_CTX *frame = *state; + struct adouble *ad = NULL; + + ad = parse_adouble(frame, ad_name, sizeof(ad_name), 0x52); + assert_null(ad); + + return; +} + +static void parse_abouble_date1(void **state) +{ + TALLOC_CTX *frame = *state; + struct adouble *ad = NULL; + char *p = NULL; + + ad = parse_adouble(frame, ad_date1, sizeof(ad_date1), 0x52); + assert_non_null(ad); + + p = ad_get_entry(ad, ADEID_FILEDATESI); + assert_null(p); + + return; +} + +static void parse_abouble_date2(void **state) +{ + TALLOC_CTX *frame = *state; + struct adouble *ad = NULL; + + ad = parse_adouble(frame, ad_date2, sizeof(ad_date2), 0x52); + assert_null(ad); + + return; +} + +int main(int argc, char *argv[]) +{ + int rc; + const struct CMUnitTest tests[] = { + cmocka_unit_test(parse_abouble_basic), + cmocka_unit_test(parse_abouble_finderinfo1), + cmocka_unit_test(parse_abouble_finderinfo2), + cmocka_unit_test(parse_abouble_finderinfo3), + cmocka_unit_test(parse_abouble_name), + cmocka_unit_test(parse_abouble_date1), + cmocka_unit_test(parse_abouble_date2), + }; + + cmocka_set_message_output(CM_OUTPUT_SUBUNIT); + + rc = cmocka_run_group_tests(tests, + setup_talloc_context, + teardown_talloc_context); + + return rc; +} diff --git a/source3/wscript_build b/source3/wscript_build index 92b95841bcf..cfd1b584e3e 100755 --- a/source3/wscript_build +++ b/source3/wscript_build @@ -1093,6 +1093,11 @@ bld.SAMBA3_SUBSYSTEM('SPOOLSSD', ''') ########################## BINARIES ################################# +if bld.CONFIG_SET('HAVE_CMOCKA'): + bld.SAMBA3_BINARY('test_adouble', + source='lib/test_adouble.c', + deps='smbd_base STRING_REPLACE cmocka', + install=False) bld.SAMBA3_BINARY('smbd/smbd', source='smbd/server.c smbd/smbd_cleanupd.c', -- 2.31.1 From 6dd0f863108cab92e97de2e4d283cd07a3c07caf Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 13 Jan 2022 17:03:02 +0100 Subject: [PATCH 6/6] CVE-2021-44142: libadouble: harden parsing code BUG: https://bugzilla.samba.org/show_bug.cgi?id=14914 Signed-off-by: Ralph Boehme --- selftest/knownfail.d/samba.unittests.adouble | 3 - source3/modules/vfs_fruit.c | 116 ++++++++++++++++--- 2 files changed, 101 insertions(+), 18 deletions(-) delete mode 100644 selftest/knownfail.d/samba.unittests.adouble diff --git a/selftest/knownfail.d/samba.unittests.adouble b/selftest/knownfail.d/samba.unittests.adouble deleted file mode 100644 index 8b0314f2fae..00000000000 --- a/selftest/knownfail.d/samba.unittests.adouble +++ /dev/null @@ -1,3 +0,0 @@ -^samba.unittests.adouble.parse_abouble_finderinfo2\(none\) -^samba.unittests.adouble.parse_abouble_finderinfo3\(none\) -^samba.unittests.adouble.parse_abouble_date2\(none\) diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index 7144b55083f..b7079892194 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -483,6 +483,95 @@ static AfpInfo *afpinfo_new(TALLOC_CTX *ctx); static ssize_t afpinfo_pack(const AfpInfo *ai, char *buf); static AfpInfo *afpinfo_unpack(TALLOC_CTX *ctx, const void *data); +/* + * 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 * @@ -490,8 +579,15 @@ static AfpInfo *afpinfo_unpack(TALLOC_CTX *ctx, const void *data); **/ 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; @@ -555,7 +651,6 @@ static int ad_setdate(struct adouble *ad, unsigned int dateoff, uint32_t date) return 0; } - /** * Map on-disk AppleDouble id to enumerated id **/ @@ -875,20 +970,11 @@ static bool ad_unpack(struct adouble *ad, const size_t nentries, 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.31.1