-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add handling for cases where ad_entry() returns NULL #174
Conversation
@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? |
Go for it. I haven't been testing Netatalk 2.2. |
Come to think of it, I also haven't tested |
Sample crash. No xattrs on dir being shared via Netatalk. |
Okay. I started testing |
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 <[email protected]>
b3fb7b7
to
687106e
Compare
Okay @rdmark I believe this is now done. I tested case with both One key change from initial fix is that I had to initialize valid_data_len in |
@anodos325 Thank you for working on this! I'll review each intermediate commit and merge relevant code with my local branch. |
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 <[email protected]>
Can you maybe give me a concrete example of how to reproduce the crash? We can communicate via email if you want. |
Maybe seeing the afp configuration would be helpful. |
@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? |
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 <[email protected]>
from Netatalk/netatalk#174 might prevent crashes without downgrading to vulnerable version 3.1.12 Signed-off-by: Šimon Bořek <[email protected]>
Thanks @anodos325 I'll test this as soon as I am able. Rpm users can use these experimental packages I built on copr: I have also mentioned this thread in redhat Bugzilla to potentially bring in more testers: |
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. |
Got another data point from a centos user. Looks like there might be an issue with setuid after building with this pr: |
That's interesting. It's an assertion due to failure to unbecome_root()
I'm disinclined to this this is related to this particular PR. The two calls are pretty much back to back:
|
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. Build and tested on Ubuntu 18.04.6
|
Does that mean it worked with the first patchset?
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: The .debug.tgz file contains debug symbols and can be used to investigate coredump. |
@anodos325, |
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? |
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 |
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:
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 [email protected] (if you don't want to share info here). The place where you're failing indicates that |
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. Server OS is Ubuntu 18.04.6 with Kernel 5.4
Configuration of Netatalk:
What is possible out of standard in our configuration:
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.
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. |
Thanks @anodos325, that fixed our issue with the upgrade. :) |
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]> (cherry picked from commit ab76857)
This fixes a core dump in netatalk.
Merge Netatalk/netatalk#174 to prevent runtime breakage.
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 <[email protected]>
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 <[email protected]> (cherry picked from commit ab76857)
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 [email protected]