Revisit CVE-2022-23121, CVE-2022-23123 regression fixes by @andychen-syno and @anodos325 #1008
Conversation
dgsga
commented
May 19, 2024
dgsga
commented
- Added guard check before access ad_entry()
- Allow zero length entry in compliance with AppleDouble specification
- Avoid setting adouble entries on symlinks
|
@rdmark, this commit implements @andychen-syno's original 2022 CVE patches along with @anodos325's patch to avoid setting adouble entries on symlinks. This gives the best of both worlds with a much cleaner log, especially on classic Mac clients. |
…syno and @anodos325 - Added guard check before access ad_entry() - Allow zero length entry in compliance with AppleDouble specification - Avoid setting adouble entries on symlinks
|
|
@dgsga Sorry it's taking a while to wrap my head around this changeset. I trust your judgement of course, but I also want to internalize the logic changes. |
|
No worries, if you find any flaws please feed back. This PR is definitely optional! |
| @@ -1342,8 +1341,8 @@ int getdirparams(const AFPObj *obj, | |||
| break; | |||
|
|
|||
| case DIRPBIT_FINFO : | |||
| if ( isad && (ade = ad_entry(&ad, ADEID_FINDERI)) != NULL) { | |||
There was a problem hiding this comment.
Doesn’t removing the null check for ad entry open up for invalid, and potentially insecure, AD data being stored?
| * improper ops on symlink adoubles will be | ||
| * more visible (assert). | ||
| */ | ||
| if (adp && (ad_meta_fileno(adp) != AD_SYMLINK) |
There was a problem hiding this comment.
I think disallowing symlinks is generally safer than allowing them. Is there a specific use case you have in mind, or perhaps an explicit line in the AD spec that allows this?
|
|
||
| if (adp->ad_vers != AD_VERSION_EA) { |
There was a problem hiding this comment.
Interesting that the check for EA style metadata was removed here. Otherwise the block’s logic seems unchanged. Is DID a valid concept for EA metadata? (I’m asking out of curiosity)
|
Reverted this PR to draft as want to do more research... |
