add handling for cases where ad_entry() returns NULL #174

anodos325 · 2022-04-11T20:43:08Z

With recent CVE fixes, ad_enty() may now return NULL. This
commit adds basic error handling for these cases and asserting
where such a return is totally unexpected. In case of
ad_getid() and ad_forcegetid(), return CNID_INVALID rather
than 0 to clarify for future people investigating this that
a 0 here is an indication of error.

In case of new_ad_header(), the valid_data_len of the
adouble data may still be zero. This causes subsequent
ad_entry() calls within new_ad_header() to fail. As such,
overwrite valid_data-Len with AD_DATASZ2 or AD_DATASZ_EA
depending on how adouble data is stored on disk.

Another side-effect of the fix is that ad_entry() for
ADEID_DID on ea-backed adouble data will return NULL. In
this case, add explicit check for the backend before
attempting to get the DID.

Signed-off-by: Andrew Walker awalker@ixsystems.com

rdmark · 2022-04-11T21:02:00Z

@anodos325 I've been working on backporting the recent CVE patches to the Netatalk 2.2 codebase ( #173 ) but held off on the fix for CVE-2022-23123 due to the issue you're addressing with this PR. Would you mind if I attempted backporting your code as well, or would you like to do it yourself?

anodos325 · 2022-04-11T21:12:16Z

Go for it. I haven't been testing Netatalk 2.2.

anodos325 · 2022-04-11T21:19:11Z

Come to think of it, I also haven't tested appledouble=v2 since we don't expose that in FreeNAS.

anodos325 · 2022-04-11T23:01:16Z

Type "apropos word" to search for commands related to "word"...
Reading symbols from /usr/local/sbin/afpd...
(No debugging symbols found in /usr/local/sbin/afpd)
[New LWP 100655]
Core was generated by `/usr/local/sbin/afpd -d -F /usr/local/etc/afp.conf'.
Program terminated with signal SIGABRT, Aborted.
#0  0x000000080076f4aa in thr_kill () from /lib/libc.so.7
(gdb) bt
#0  0x000000080076f4aa in thr_kill () from /lib/libc.so.7
#1  0x000000080076d904 in raise () from /lib/libc.so.7
#2  0x00000008006e36e9 in abort () from /lib/libc.so.7
#3  0x0000000800307d66 in fault_report (sig=11) at fault.c:139
#4  0x0000000800307c03 in sig_fault (sig=11) at fault.c:147
#5  0x000000080059ec60 in ?? () from /lib/libthr.so.3
#6  0x000000080059e22f in ?? () from /lib/libthr.so.3
#7  <signal handler called>
#8  0x00000008002dd1f4 in new_ad_header (ad=0x7fffffffe108, path=0x80100e1b0 "/zroot/TEST", stp=0x0, adflags=1292) at ad_open.c:370
#9  0x00000008002dce03 in ad_open_hf_ea (path=0x80100e1b0 "/zroot/TEST", adflags=1292, mode=438, ad=0x7fffffffe108) at ad_open.c:1251
#10 0x00000008002d9ee4 in ad_open_hf (path=0x80100e1b0 "/zroot/TEST", adflags=1292, mode=438, ad=0x7fffffffe108) at ad_open.c:1288
#11 0x00000008002d95de in ad_open (ad=0x7fffffffe108, path=0x80100e1b0 "/zroot/TEST", adflags=1292) at ad_open.c:1998

(gdb) frame 8
#8  0x00000008002dd1f4 in new_ad_header (ad=0x7fffffffe108, path=0x80100e1b0 "/zroot/TEST", stp=0x0, adflags=1292) at ad_open.c:370
370	    memcpy(ad_entry(ad, ADEID_FINDERI) + FINDERINFO_FRTYPEOFF,"\0\0\0\0", 4);
(gdb) p ad.valid_data_len 
$6 = 0
(gdb) p ad.ad_eid
$7 = {{ade_off = 0, ade_len = 0}, {ade_off = 0, ade_len = 0}, {ade_off = 82, ade_len = 0}, {ade_off = 0, ade_len = 0}, {ade_off = 154, ade_len = 0}, 
  {ade_off = 0, ade_len = 0}, {ade_off = 0, ade_len = 0}, {ade_off = 0, ade_len = 0}, {ade_off = 354, ade_len = 16}, {ade_off = 122, ade_len = 32}, {
    ade_off = 0, ade_len = 0}, {ade_off = 0, ade_len = 0}, {ade_off = 0, ade_len = 0}, {ade_off = 0, ade_len = 0}, {ade_off = 370, ade_len = 4}, {
    ade_off = 0, ade_len = 0}, {ade_off = 374, ade_len = 0}, {ade_off = 382, ade_len = 0}, {ade_off = 390, ade_len = 0}, {ade_off = 398, 
    ade_len = 0}}
(gdb)

Sample crash. No xattrs on dir being shared via Netatalk. ad_entry(ad, ADEID_FINDERI) returns NULL here because ad->valid_data_len is 0 (due to creating new ad metadata xattr.

libatalk/adouble/ad_attr.c

anodos325 · 2022-04-12T15:00:04Z

Okay. I started testing appledouble = v2 and that also appears somewhat broken, e.g. parse_entries() is failing. Will make another push later today with fix for that. Once everyone is happy, with merge request, I'll squash into single commit.

With recent CVE fixes, ad_enty() may now return NULL. This commit adds basic error handling for these cases and asserting where such a return is totally unexpected. In case of ad_getid() and ad_forcegetid(), return CNID_INVALID rather than 0 to clarify for future people investigating this that a 0 here is an indication of error. In case of new_ad_header(), the valid_data_len of the adouble data may still be zero. This causes subsequent ad_entry() calls within new_ad_header() to fail. As such, overwrite valid_data-Len with AD_DATASZ2 or AD_DATASZ_EA depending on how adouble data is stored on disk. Another side-effect of the fix is that ad_entry() for ADEID_DID on ea-backed adouble data will return NULL. In this case, add explicit check for the backend before attempting to get the DID. Signed-off-by: Andrew Walker <awalker@ixsystems.com>

anodos325 · 2022-04-12T20:25:31Z

Okay @rdmark I believe this is now done. I tested case with both adouble = ea and adouble = v2, and also verified that resource fork reads / writes work as expected.

One key change from initial fix is that I had to initialize valid_data_len in new_ad_header() so that cases where new_ad_header() is immediately followed by ad_flush() will succeed.

rdmark · 2022-04-13T02:33:11Z

@anodos325 Thank you for working on this! I'll review each intermediate commit and merge relevant code with my local branch.

libatalk/adouble/ad_attr.c

rdmark · 2022-04-13T07:07:25Z

This PR adopted to the 2.2 branch in c754d46

With recent CVE fixes, ad_enty() may now return NULL. This commit adds basic error handling for these cases and asserting where such a return is totally unexpected. In case of ad_getid() and ad_forcegetid(), return CNID_INVALID rather than 0 to clarify for future people investigating this that a 0 here is an indication of error. In case of new_ad_header(), the valid_data_len of the adouble data may still be zero. Since we're initializing fresh here with OS-provided data from lstat() call, temporarily override the size check prior to calling ad_entry() (otherwise we will get NULL value unexpectedly here). Once new header is generated, reset original value of valid_data_len. Signed-off-by: Andrew Walker <awalker@ixsystems.com>

libatalk/adouble/ad_attr.c

anodos325 · 2022-04-22T19:20:15Z

Can you maybe give me a concrete example of how to reproduce the crash? We can communicate via email if you want. awalker@ixsystems.com

anodos325 · 2022-04-22T19:24:26Z

Maybe seeing the afp configuration would be helpful.

rdmark · 2022-04-23T02:17:36Z

@anodos325 Sorry, I didn't have time to further troubleshoot. It was a busy week at work and no time/energy for tinkering with Netatalk! The upcoming weekend seems pretty free though, so I'm going to double down on trying to figure out this issue.

Anyhow, I shot you an email just now. Actually, I sent you another email to that address about a week ago. Perhaps they're getting stuck in your spam filter?

knight-of-ni · 2022-04-28T13:15:18Z

As most readers are probably aware, there are multiple reports afpd segfaults with the latest 3.1.13 release of netatalk. The following seems to be the best description of what is going on: https://sourceforge.net/p/netatalk/mailman/message/37632074/

This also seems to be the very thing this pull request addresses i.e. ad_entry() returns a null pointer.

Has anyone tested this and can confirm whether or not this resolves the afpd segfault?

anodos325 · 2022-04-28T13:28:04Z

As most readers are probably aware, there are multiple reports afpd segfaults with the latest 3.1.13 release of netatalk. The following seems to be the best description of what is going on: https://sourceforge.net/p/netatalk/mailman/message/37632074/

This also seems to be the very thing this pull request addresses i.e. ad_entry() returns a null pointer.

Has anyone tested this and can confirm whether or not this resolves the afpd segfault?

We're currently using this in TrueNAS 12.0-U8.1.

It passed our basic testing, but there have been two user reports of crashes (which isn't a large number, and might be some odd edge-case).

I've requested corefile from affected users, but not much traction yet. I've been a bit too busy to investigate further.

If you want to test build with this patch you can try running this in a VM: https://download.freenas.org/12.0/STABLE/U8.1/

The download dir there also contains a tar file with debug symbols so that you can investigate any crashes and ping me if something happens.

from Netatalk/netatalk#174 might prevent crashes without downgrading to vulnerable version 3.1.12 Signed-off-by: Šimon Bořek <simon.borek@nic.cz>

knight-of-ni · 2022-05-01T12:14:44Z

Thanks @anodos325 I'll test this as soon as I am able.

Rpm users can use these experimental packages I built on copr:
https://copr.fedorainfracloud.org/coprs/kni/netatalk/

I have also mentioned this thread in redhat Bugzilla to potentially bring in more testers:
https://bugzilla.redhat.com/show_bug.cgi?id=2074586

ghost · 2022-05-02T14:12:34Z

I can confirm that this pull requests compiles and fixes the segfaults. I will do some more detailed testing and report back as soon as possible.

knight-of-ni · 2022-05-03T12:25:05Z

Got another data point from a centos user. Looks like there might be an issue with setuid after building with this pr:
#175 (comment)

anodos325 · 2022-05-03T18:15:41Z

Got another data point from a centos user. Looks like there might be an issue with setuid after building with this pr: #175 (comment)

That's interesting. It's an assertion due to failure to unbecome_root()

static uid_t saved_uid = -1;

/*
 * seteuid(0) and back, if either fails and panic != 0 we PANIC
 */
void become_root(void)
{
    if (getuid() == 0) {
        saved_uid = geteuid();
        if (seteuid(0) != 0)
            AFP_PANIC("Can't seteuid(0)");
    }
}

void unbecome_root(void)
{
    if (getuid() == 0) {
        if (saved_uid == -1 || seteuid(saved_uid) < 0)
            AFP_PANIC("Can't seteuid back");
        saved_uid = -1;
    }
}

I'm disinclined to this this is related to this particular PR. The two calls are pretty much back to back:

int ad_metadata(const char *name, int flags, struct adouble *adp)
{
    int   ret, err, oflags;

    /* Sanitize flags */
    oflags = (flags & (ADFLAGS_CHECK_OF | ADFLAGS_DIR)) | ADFLAGS_HF | ADFLAGS_RDONLY;

    if ((ret = ad_open(adp, name, oflags)) < 0 && errno == EACCES) {
        become_root();
        ret = ad_open(adp, name, oflags);
        unbecome_root();
        err = errno;
        errno = err;
    }

    return ret;
}

ghost · 2022-05-04T08:30:05Z

I did some more detailed testing with this pr and cannot reproduce the "unbecome_root" issue. This may be due to different configuration of netatalk.
Performance and behaviour looks good to me with one exemption. Trying to copy a symbolic link via afp to the server or changing to a directory containing a symbolic link fails with a panic of afpd. This definitely worked on 3.1.12 and earlier versions.

Build and tested on Ubuntu 18.04.6

May 04 10:25:18.849877 afpd[22970] {fault.c:123} (severe:Default): ===============================================================
May 04 10:25:18.850004 afpd[22970] {fault.c:124} (severe:Default): INTERNAL ERROR: Signal 11 in pid 22970 (3.1.13)
May 04 10:25:18.850025 afpd[22970] {fault.c:125} (severe:Default): ===============================================================
May 04 10:25:18.850457 afpd[22970] {fault.c:96} (severe:Default): PANIC: internal error
May 04 10:25:18.850525 afpd[22970] {fault.c:97} (severe:Default): BACKTRACE: 10 stack frames:
May 04 10:25:18.850544 afpd[22970] {fault.c:103} (severe:Default):  #0 /usr/lib/x86_64-linux-gnu/libatalk.so.18(netatalk_panic+0x35) [0x7f02d415e775]
May 04 10:25:18.850560 afpd[22970] {fault.c:103} (severe:Default):  #1 /usr/lib/x86_64-linux-gnu/libatalk.so.18(+0x3d8bc) [0x7f02d415e8bc]
May 04 10:25:18.850575 afpd[22970] {fault.c:103} (severe:Default):  #2 /lib/x86_64-linux-gnu/libc.so.6(+0x3ef10) [0x7f02d3743f10]
May 04 10:25:18.850601 afpd[22970] {fault.c:103} (severe:Default):  #3 /usr/sbin/afpd(+0x1fa50) [0x55ba51c45a50]
May 04 10:25:18.850616 afpd[22970] {fault.c:103} (severe:Default):  #4 /usr/sbin/afpd(setfilparams+0x4c2) [0x55ba51c476a2]
May 04 10:25:18.850631 afpd[22970] {fault.c:103} (severe:Default):  #5 /usr/sbin/afpd(afp_setfilparams+0xca) [0x55ba51c47d6a]
May 04 10:25:18.850646 afpd[22970] {fault.c:103} (severe:Default):  #6 /usr/sbin/afpd(afp_over_dsi+0x5a6) [0x55ba51c33a06]
May 04 10:25:18.850660 afpd[22970] {fault.c:103} (severe:Default):  #7 /usr/sbin/afpd(main+0xab3) [0x55ba51c31903]
May 04 10:25:18.850674 afpd[22970] {fault.c:103} (severe:Default):  #8 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xe7) [0x7f02d3726c87]
May 04 10:25:18.850688 afpd[22970] {fault.c:103} (severe:Default):  #9 /usr/sbin/afpd(_start+0x2a) [0x55ba51c31b8a]

dgsga · 2022-05-05T19:08:39Z

With the latest patchset I get a segfault when opening a shared folder on a Mac OS 9 client:

May 04 20:03:11.258046 afpd[17291] {ad_open.c:800} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube"): invalid metadata EA
May 04 20:03:11.258215 afpd[17291] {ad_open.c:818} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube"): deleted invalid metadata EA
May 04 20:03:14.078812 afpd[17291] {ad_open.c:800} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube"): invalid metadata EA
May 04 20:03:14.080045 afpd[17291] {ad_open.c:818} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube"): deleted invalid metadata EA
May 04 20:03:14.091927 afpd[17291] {ad_open.c:800} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube"): invalid metadata EA
May 04 20:03:14.093060 afpd[17291] {ad_open.c:818} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube"): deleted invalid metadata EA
May 04 20:03:14.097629 afpd[17291] {ad_open.c:800} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube"): invalid metadata EA
May 04 20:03:14.098151 afpd[17291] {ad_open.c:818} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube"): deleted invalid metadata EA
May 04 20:03:14.099250 afpd[17291] {ad_open.c:800} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube"): invalid metadata EA
May 04 20:03:14.099752 afpd[17291] {ad_open.c:818} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube"): deleted invalid metadata EA
May 04 20:03:14.345729 afpd[17291] {ad_open.c:800} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube/."): invalid metadata EA
May 04 20:03:14.346749 afpd[17291] {ad_open.c:818} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube/."): deleted invalid metadata EA
May 04 20:03:14.429006 afpd[17291] {ad_open.c:800} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube/Icon
"): invalid metadata EA
May 04 20:03:14.429685 afpd[17291] {ad_open.c:818} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube/Icon
"): deleted invalid metadata EA
May 04 20:03:14.651111 afpd[17291] {ad_open.c:800} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube/."): invalid metadata EA
May 04 20:03:14.652130 afpd[17291] {ad_open.c:818} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube/."): deleted invalid metadata EA
May 04 20:03:14.748245 afpd[17291] {ad_open.c:800} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube/Network Trash Folder/Trash Can #2"): invalid metadata EA
May 04 20:03:14.748906 afpd[17291] {ad_open.c:818} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube/Network Trash Folder/Trash Can #2"): deleted invalid metadata EA
May 04 20:03:14.750822 afpd[17291] {ad_open.c:800} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube/Network Trash Folder/Trash Can Usage Map"): invalid metadata EA
May 04 20:03:14.767290 afpd[17291] {ad_open.c:818} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube/Network Trash Folder/Trash Can Usage Map"): deleted invalid metadata EA
May 04 20:03:15.575305 afpd[17291] {ad_attr.c:123} (warn:ad): ad_setid: failed to set ADEID_PRIVDEV

May 04 20:03:15.577108 afpd[17291] {ad_open.c:800} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube"): invalid metadata EA
May 04 20:03:15.577952 afpd[17291] {ad_open.c:818} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube"): deleted invalid metadata EA
May 04 20:03:15.623613 afpd[17291] {ad_open.c:800} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube/."): invalid metadata EA
May 04 20:03:15.624192 afpd[17291] {ad_open.c:818} (error:ad): ad_header_read_ea("/Volumes/Igraine/Sync/Cube/."): deleted invalid metadata EA
May 04 20:03:22.181686 afpd[17291] {ad_attr.c:123} (warn:ad): ad_setid: failed to set ADEID_PRIVDEV

May 04 20:03:22.182750 afpd[17291] {ad_attr.c:177} (warn:ad): ad_getid: failed to retrieve ADEID_PRIVDEV

May 04 20:03:22.184417 afpd[17291] {fault.c:123} (severe:Default): ===============================================================
May 04 20:03:22.184948 afpd[17291] {fault.c:124} (severe:Default): INTERNAL ERROR: Signal 11 in pid 17291 (3.2.1)
May 04 20:03:22.185438 afpd[17291] {fault.c:125} (severe:Default): ===============================================================

I've been trying to troubleshoot it but no joy so far... The 'Network Trash Folder' is automatically created by Mac OS 9 on a network share. Is there a way to identify exactly where the ad_setid() and ad_getid ADEID_PRIVDEV errors occur (which files are affected)?

anodos325 · 2022-05-05T19:24:50Z

With the latest patchset I get a segfault when opening a shared folder on a Mac OS 9 client:

Does that mean it worked with the first patchset?

I've been trying to troubleshoot it but no joy so far... The 'Network Trash Folder' is automatically created by Mac OS 9 on a network share. Is there a way to identify exactly where the ad_setid() and ad_getid ADEID_PRIVDEV errors occur (which files are affected)?

Sure. I can see about improving log message there. Otherwise, you can try our latest nightly for TrueNAS 13 in a VM. https://download.freenas.org/13.0/MASTER/202205050459/x64/

I have compiled without optimizations and in debug mode. You might have to alter default corefile path to capture:
kern.corefile=/var/db/system/cores/%N_%P.core

The .debug.tgz file contains debug symbols and can be used to investigate coredump.

ghost · 2022-05-06T14:07:34Z

@anodos325,
my team and I tested the last patch extensively and we were unable to break it. As said earlier, build and tested on Ubuntu 18.04.6. On client side we tested the latest supported versions of MacOS 10, 11 and 12. As the last version of MacOS 9 is now 20 years old and we have no hardware laying around to run it, we skipped this one. ;)
Excellent job!

dgsga · 2022-05-08T12:15:23Z

With the latest patchset I get a segfault when opening a shared folder on a Mac OS 9 client:

Does that mean it worked with the first patchset?

Unfortunately not. Strangely enough all of the errors and the segfault disappears if the fixed size boolean parameter for ADEID_PRIVDEV in ad_entry_check_size () is set to false. I have no idea why, my skills in C are basic to say the least... I wonder if this alteration opens up the CVE vulnerability all over again?

dgsga · 2022-05-08T12:28:07Z

As the last version of MacOS 9 is now 20 years old and we have no hardware laying around to run it, we skipped this one.

Still like to tinker with my G4 cube... ;)

anodos325 · 2022-05-10T21:15:14Z

As the last version of MacOS 9 is now 20 years old and we have no hardware laying around to run it, we skipped this one.

Still like to tinker with my G4 cube... ;)

Well, if you can build without optimizations, crash it and get corefile, then give me results of bt full I might be able to see where issue is and fix it. I don't have any hardware that old lying around. For that matter if you crash it in TrueNAS 13 I already have debug symbols and can investigate there.

ghost · 2022-05-11T07:06:17Z

Got some bad news today. We tested an upgrade scenario from 3.1.12 to 3.1.13. On the first mount of the volumes we got these messages in netatalk.log:

May 10 15:24:12.134732 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_online/.DS_Store"): invalid metadata EA
May 10 15:24:12.134848 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_online/.DS_Store"): deleted invalid metadata EA
May 10 15:24:12.181840 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12"): invalid metadata EA
May 10 15:24:12.181990 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12"): deleted invalid metadata EA
May 10 15:24:12.773729 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/Images"): invalid metadata EA
May 10 15:24:12.773884 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/Images"): deleted invalid metadata EA
May 10 15:24:12.774073 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/.DS_Store"): invalid metadata EA
May 10 15:24:12.774146 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/.DS_Store"): deleted invalid metadata EA
May 10 15:24:12.774283 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/example"): invalid metadata EA
May 10 15:24:12.774317 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/example"): deleted invalid metadata EA
May 10 15:24:13.097143 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/example/example.gif"): invalid metadata EA
May 10 15:24:13.097279 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/example/example.gif"): deleted invalid metadata EA
May 10 15:24:13.097507 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/example/example.jpg"): invalid metadata EA
May 10 15:24:13.097542 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/example/example.jpg"): deleted invalid metadata EA

Consequence was, that all Finder Tags were no longer shown in the Finder window and MacOS Alias files were broken. I can imagine, that more will not work properly if all metadata EA is invalidated. "dbd -f" is not able to repair this and even a downgrade to 3.1.12 will not fix this. So once you upgrade all metadata is lost and you have to stress your backup strategy.

What feels strange is, metadata is not completely lost. The Finder window does not show it, but if you open file information (Command + I) all Finder Tags are shown. If you add another Finder Tag all others also appear in the Finder window. Regarding MacOS Alias files I had no luck.

Any idea?

anodos325 · 2022-05-11T10:59:20Z

Got some bad news today. We tested an upgrade scenario from 3.1.12 to 3.1.13. On the first mount of the volumes we got these messages in netatalk.log:

May 10 15:24:12.134732 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_online/.DS_Store"): invalid metadata EA
May 10 15:24:12.134848 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_online/.DS_Store"): deleted invalid metadata EA
May 10 15:24:12.181840 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12"): invalid metadata EA
May 10 15:24:12.181990 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12"): deleted invalid metadata EA
May 10 15:24:12.773729 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/Images"): invalid metadata EA
May 10 15:24:12.773884 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/Images"): deleted invalid metadata EA
May 10 15:24:12.774073 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/.DS_Store"): invalid metadata EA
May 10 15:24:12.774146 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/.DS_Store"): deleted invalid metadata EA
May 10 15:24:12.774283 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/example"): invalid metadata EA
May 10 15:24:12.774317 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/example"): deleted invalid metadata EA
May 10 15:24:13.097143 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/example/example.gif"): invalid metadata EA
May 10 15:24:13.097279 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/example/example.gif"): deleted invalid metadata EA
May 10 15:24:13.097507 afpd[27120] {ad_open.c:803} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/example/example.jpg"): invalid metadata EA
May 10 15:24:13.097542 afpd[27120] {ad_open.c:821} (error:ad): ad_header_read_ea("/storage/testserver-vr_production/netatalk_3.1.12/example/example.jpg"): deleted invalid metadata EA

Consequence was, that all Finder Tags were no longer shown in the Finder window and MacOS Alias files were broken. I can imagine, that more will not work properly if all metadata EA is invalidated. "dbd -f" is not able to repair this and even a downgrade to 3.1.12 will not fix this. So once you upgrade all metadata is lost and you have to stress your backup strategy.

What feels strange is, metadata is not completely lost. The Finder window does not show it, but if you open file information (Command + I) all Finder Tags are shown. If you add another Finder Tag all others also appear in the Finder window. Regarding MacOS Alias files I had no luck.

Any idea?

I tried to reproduce your issue by writing tags (color and others) via finder in 3.1.12, upgraded and they were still visible in 3.1.13. I'll need a better procedure or more configuration details. You can email me at awalker@ixsystems.com (if you don't want to share info here).

The place where you're failing indicates that parse_entries() has failed and metadata is flagged as invalid. Since apparently impact is significant, I can make it netatalk_panic() in this edge-case (with an exhortation to user to provide the core file to the package developer).

ghost · 2022-05-11T12:15:54Z

Lucky you, the procedure for us was the same. We set up a new server and installed Netatalk 3.1.12, put some example files to it and added Finder Tags (red, yellow and green). We also placed a macOS Alias file in that folder. After upgrading to 3.1.13 all tags are not shown anymore and macOS Alias is not recognized as an alias anymore.
Our configuration may be a bit different to normal setups, but no problem to share it here in public.

Server OS is Ubuntu 18.04.6 with Kernel 5.4
Filesystem for Netatalk shared volumes is BTRFS - mount options: noatime, space_cache=v2
Compile options of Netatalk:
--with-init-style=debian-systemd --without-libevent --without-tdb --with-cracklib --enable-krbV-uam --with-pam-confdir=/etc/pam.d --with-dbus-daemon=/usr/bin/dbus-daemon --with-dbus-sysconf-dir=/etc/dbus-1/system.d --with-tracker-pkgconfig-version=1.0
Featureset Netatalk:

afpd has been compiled with support for these features:

          AFP versions:	2.2 3.0 3.1 3.2 3.3 3.4
         CNID backends:	dbd last tdb mysql
      Zeroconf support:	Avahi
  TCP wrappers support:	Yes
         Quota support:	Yes
   Admin group support:	Yes
    Valid shell checks:	Yes
      cracklib support:	Yes
            EA support:	ad | sys
           ACL support:	Yes
          LDAP support:	Yes
         D-Bus support:	Yes
     Spotlight support:	No
         DTrace probes:	Yes

Configuration of Netatalk:

;;;;;;;;;;;;;;;;;;;;;;;;
; Global configuration ;
;;;;;;;;;;;;;;;;;;;;;;;;

[Global]
; Log
log file = /var/log/netatalk.log
;log level = default:maxdebug

; Disable spotlight to save performance
spotlight = no

; Enable client polling to improve snappiness of Apple Finder (Tags)
client polling = yes

; Enable afpstats for administrator entertainment
afpstats = yes

;;;;;;;;;;;;;;;;;;;
; Volume defaults ;
;;;;;;;;;;;;;;;;;;;

[defaults]
file perm = 0666
directory perm = 0775
umask = 2000
spotlight = no
search db = yes


;;;;;;;;;;;;;;;;;;
; Shared Volumes ;
;;;;;;;;;;;;;;;;;;

[testserver-vr_production]
path = /storage/testserver-vr_production
vol preset = defaults

What is possible out of standard in our configuration:

Since BTRFS supports extended attributes metadata is stored in EA but not in .AppleDouble files in our setup.
Using BTRFS at all. ;)
client polling = yes
search db = yes

We are using dbd as cnid backend, but that should be very common in my opinion.

I would be happy to provide you more information if you need some.

In some edge cases the comment in finder info xattr may be zero-length. This means that ad_entry() will return NULL with latest CVE fixes. This means we need alternate validation for comment. Since the ad_entry handling changed significantly in 3.12->3.13 update, AFP_ASSERT() when metadata is flagged as invalid rather than attempting to remove it. This will give us an opportunity to fix any other issues introduced by the original changeset without affecting user metadata.

anodos325 · 2022-05-11T14:33:32Z

Okay. I just pushed a fix for what I could reproduce. It was choking on zero-length comment in xattr. I also made it so that netatalk will assert rather than remove a problematic xattr. Obviously this opens possibility of local user DOS of server by writing an invalid xattr, but protects against mucking up file metadata if there's another issue in original CVE fix or this patchset.

ghost · 2022-05-17T06:56:29Z

Thanks @anodos325, that fixed our issue with the upgrade. :)
We will upgrade the first production server later this week and see how it works.

knight-of-ni · 2022-05-18T13:41:05Z

Posting the following here for visibility.

I have received feedback that this PR has had some success on the redhat/fedora side. Thus, I have gone ahead and built new Netatalk packages, patched with this PR. If there are any new or lingering issues, I'd expect to receive additional feedback in about 2 weeks.

This commit backports pending PR: - Netatalk/netatalk#174 To fix issues with segfaults described here: - openwrt#18571

This commit backports pending PR, which solves segfaults: - Netatalk/netatalk#174 To fix issues with segfaults described here: - openwrt#18571 - Netatalk/netatalk#175 Signed-off-by: Šimon Bořek <simon.borek@nic.cz>

This commit backports pending PR, which solves segfaults: - Netatalk/netatalk#174 To fix issues with segfaults described here: - #18571 - Netatalk/netatalk#175 Signed-off-by: Šimon Bořek <simon.borek@nic.cz>

Not yet merged fix for 3.1.13 version exists which seems to be already in use in TrueNAS 12.0-U8.1 as stated in Netatalk/netatalk#174 (comment) .

This commit backports pending PR, which solves segfaults: - Netatalk/netatalk#174 To fix issues with segfaults described here: - openwrt#18571 - Netatalk/netatalk#175 Signed-off-by: Šimon Bořek <simon.borek@nic.cz> (cherry picked from commit ab76857)

This fixes a core dump in netatalk.

Merge Netatalk/netatalk#174 to prevent runtime breakage.

slowfranklin · 2022-07-12T04:15:51Z

Pushed, thanks!

This commit backports pending PR, which solves segfaults: - Netatalk/netatalk#174 To fix issues with segfaults described here: - openwrt#18571 - Netatalk/netatalk#175 Signed-off-by: Šimon Bořek <simon.borek@nic.cz>

This commit backports pending PR, which solves segfaults: - Netatalk/netatalk#174 To fix issues with segfaults described here: - openwrt#18571 - Netatalk/netatalk#175 Signed-off-by: Šimon Bořek <simon.borek@nic.cz> (cherry picked from commit ab76857)

rdmark reviewed Apr 12, 2022

View reviewed changes

libatalk/adouble/ad_attr.c Outdated Show resolved Hide resolved

rdmark reviewed Apr 12, 2022

View reviewed changes

libatalk/adouble/ad_attr.c Outdated Show resolved Hide resolved

anodos325 force-pushed the fix-post-cve-fix-crash branch from b3fb7b7 to 687106e Compare April 12, 2022 20:22

rdmark reviewed Apr 13, 2022

View reviewed changes

libatalk/adouble/ad_attr.c Show resolved Hide resolved

rdmark reviewed Apr 13, 2022

View reviewed changes

libatalk/adouble/ad_attr.c Show resolved Hide resolved

rdmark reviewed Apr 18, 2022

View reviewed changes

libatalk/adouble/ad_attr.c Show resolved Hide resolved

BKPepe mentioned this pull request Apr 20, 2022

Revert "netatalk: update to version 3.1.13" openwrt/packages#18337

Merged

knight-of-ni mentioned this pull request May 3, 2022

Netatalk 3.1.13 segfaults in afpd #175

Closed

BKPepe pushed a commit to BKPepe/packages that referenced this pull request Jun 3, 2022

patches/packages: netatalk: backport pending PR to fix segfaults

cede7b5

This commit backports pending PR: - Netatalk/netatalk#174 To fix issues with segfaults described here: - openwrt#18571

BKPepe mentioned this pull request Jun 6, 2022

patches/packages: netatalk: backport pending PR to fix segfaults openwrt/packages#18687

Merged

pjones added a commit to pjones/tilde that referenced this pull request Jun 19, 2022

netatalk: Apply patch from Netatalk/netatalk#174

cbb5ae0

This fixes a core dump in netatalk.

bob-beck pushed a commit to openbsd/ports that referenced this pull request Jul 3, 2022

SECURITY update to netatalk-3.1.13.

75d5455

Merge Netatalk/netatalk#174 to prevent runtime breakage.

rdmark mentioned this pull request Jul 5, 2022

Fix CVE-2022-23121, CVE-2022-23123 regression #178

Merged

slowfranklin closed this Jul 12, 2022

raindev mentioned this pull request Oct 29, 2022

netatalk: patch afpd segfault NixOS/nixpkgs#198453

Closed

13 tasks

whorfin mentioned this pull request Dec 5, 2022

netatalk3-3.1.13nb3 is broken TritonDataCenter/pkgsrc#348

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add handling for cases where ad_entry() returns NULL #174

add handling for cases where ad_entry() returns NULL #174

anodos325 commented Apr 11, 2022 •

edited

rdmark commented Apr 11, 2022

anodos325 commented Apr 11, 2022

anodos325 commented Apr 11, 2022

anodos325 commented Apr 11, 2022 •

edited

anodos325 commented Apr 12, 2022

anodos325 commented Apr 12, 2022

rdmark commented Apr 13, 2022

rdmark commented Apr 13, 2022

anodos325 commented Apr 22, 2022

anodos325 commented Apr 22, 2022

rdmark commented Apr 23, 2022

knight-of-ni commented Apr 28, 2022

anodos325 commented Apr 28, 2022 •

edited

knight-of-ni commented May 1, 2022 •

edited

ghost commented May 2, 2022

knight-of-ni commented May 3, 2022

anodos325 commented May 3, 2022 •

edited

ghost commented May 4, 2022 •

edited by ghost

dgsga commented May 5, 2022 •

edited

anodos325 commented May 5, 2022 •

edited

ghost commented May 6, 2022

dgsga commented May 8, 2022 •

edited

dgsga commented May 8, 2022 •

edited

anodos325 commented May 10, 2022 •

edited

ghost commented May 11, 2022 •

edited by ghost

anodos325 commented May 11, 2022

ghost commented May 11, 2022

anodos325 commented May 11, 2022

ghost commented May 17, 2022

knight-of-ni commented May 18, 2022

slowfranklin commented Jul 12, 2022

add handling for cases where ad_entry() returns NULL #174

add handling for cases where ad_entry() returns NULL #174

Conversation

anodos325 commented Apr 11, 2022 • edited

rdmark commented Apr 11, 2022

anodos325 commented Apr 11, 2022

anodos325 commented Apr 11, 2022

anodos325 commented Apr 11, 2022 • edited

anodos325 commented Apr 12, 2022

anodos325 commented Apr 12, 2022

rdmark commented Apr 13, 2022

rdmark commented Apr 13, 2022

anodos325 commented Apr 22, 2022

anodos325 commented Apr 22, 2022

rdmark commented Apr 23, 2022

knight-of-ni commented Apr 28, 2022

anodos325 commented Apr 28, 2022 • edited

knight-of-ni commented May 1, 2022 • edited

ghost commented May 2, 2022

knight-of-ni commented May 3, 2022

anodos325 commented May 3, 2022 • edited

ghost commented May 4, 2022 • edited by ghost

dgsga commented May 5, 2022 • edited

anodos325 commented May 5, 2022 • edited

ghost commented May 6, 2022

dgsga commented May 8, 2022 • edited

dgsga commented May 8, 2022 • edited

anodos325 commented May 10, 2022 • edited

ghost commented May 11, 2022 • edited by ghost

anodos325 commented May 11, 2022

ghost commented May 11, 2022

anodos325 commented May 11, 2022

ghost commented May 17, 2022

knight-of-ni commented May 18, 2022

slowfranklin commented Jul 12, 2022

anodos325 commented Apr 11, 2022 •

edited

anodos325 commented Apr 11, 2022 •

edited

anodos325 commented Apr 28, 2022 •

edited

knight-of-ni commented May 1, 2022 •

edited

anodos325 commented May 3, 2022 •

edited

ghost commented May 4, 2022 •

edited by ghost

dgsga commented May 5, 2022 •

edited

anodos325 commented May 5, 2022 •

edited

dgsga commented May 8, 2022 •

edited

dgsga commented May 8, 2022 •

edited

anodos325 commented May 10, 2022 •

edited

ghost commented May 11, 2022 •

edited by ghost