Skip to content
This repository has been archived by the owner on Oct 25, 2024. It is now read-only.

Commit

Permalink
add handling for cases where ad_entry() returns NULL
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@anodos325 @rdmark
anodos325 authored and rdmark committed Apr 13, 2022
1 parent 06ba3b5 commit c754d46
Show file tree
Hide file tree
Showing 7 changed files with 195 additions and 48 deletions.
15 changes: 11 additions & 4 deletions etc/afpd/directory.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1486,6 +1486,7 @@ int getdirparams(const struct vol *vol,
struct maccess ma;
struct adouble ad;
char *data, *l_nameoff = NULL, *utf_nameoff = NULL;
char *ade = NULL;
int bit = 0, isad = 0;
u_int32_t aint;
u_int16_t ashort;
Expand Down Expand Up @@ -1579,7 +1580,10 @@ int getdirparams(const struct vol *vol,

case DIRPBIT_FINFO :
if ( isad ) {
memcpy( data, ad_entry( &ad, ADEID_FINDERI ), 32 );
ade = ad_entry(&ad, ADEID_FINDERI);
AFP_ASSERT(ade != NULL);

memcpy( data, ade, 32 );
} else { /* no appledouble */
memset( data, 0, 32 );
/* dot files are by default visible */
Expand Down Expand Up @@ -1805,6 +1809,7 @@ int setdirparams(struct vol *vol, struct path *path, u_int16_t d_bitmap, char *b
struct timeval tv;

char *upath;
char *ade = NULL;
struct dir *dir;
int bit, isad = 1;
int cdate, bdate;
Expand Down Expand Up @@ -1996,6 +2001,8 @@ int setdirparams(struct vol *vol, struct path *path, u_int16_t d_bitmap, char *b
fflags &= htons(~FINDERINFO_ISHARED);
memcpy(finder_buf + FINDERINFO_FRFLAGOFF, &fflags, sizeof(uint16_t));
/* #2802236 end */
ade = ad_entry(&ad, ADEID_FINDERI);
AFP_ASSERT(ade != NULL);

if ( dir->d_did == DIRDID_ROOT ) {
/*
Expand All @@ -2006,10 +2013,10 @@ int setdirparams(struct vol *vol, struct path *path, u_int16_t d_bitmap, char *b
* behavior one sees when mounting above another mount
* point.
*/
memcpy( ad_entry( &ad, ADEID_FINDERI ), finder_buf, 10 );
memcpy( ad_entry( &ad, ADEID_FINDERI ) + 14, finder_buf + 14, 18 );
memcpy( ade, finder_buf, 10 );
memcpy( ade + 14, finder_buf + 14, 18 );
} else {
memcpy( ad_entry( &ad, ADEID_FINDERI ), finder_buf, 32 );
memcpy( ade, finder_buf, 32 );
}
}
break;
Expand Down
26 changes: 20 additions & 6 deletions etc/afpd/file.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ int getmetadata(struct vol *vol,
{
char *data, *l_nameoff = NULL, *upath;
char *utf_nameoff = NULL;
char *ade = NULL;
int bit = 0;
u_int32_t aint;
cnid_t id = 0;
Expand Down Expand Up @@ -498,7 +499,10 @@ int getmetadata(struct vol *vol,
}
else {
if ( adp ) {
memcpy(fdType, ad_entry( adp, ADEID_FINDERI ), 4 );
ade = ad_entry(adp, ADEID_FINDERI);
AFP_ASSERT(ade != NULL);

memcpy(fdType, ade, 4);

if ( memcmp( fdType, "TEXT", 4 ) == 0 ) {
achar = '\x04';
Expand Down Expand Up @@ -575,7 +579,10 @@ int getmetadata(struct vol *vol,

aint = st->st_mode;
if (adp) {
memcpy(fdType, ad_entry( adp, ADEID_FINDERI ), 4 );
ade = ad_entry(adp, ADEID_FINDERI);
AFP_ASSERT(ade != NULL);

memcpy(fdType, ade, 4);
if ( memcmp( fdType, "slnk", 4 ) == 0 ) {
aint |= S_IFLNK;
}
Expand Down Expand Up @@ -854,6 +861,7 @@ int setfilparams(struct vol *vol,
struct extmap *em;
int bit, isad = 1, err = AFP_OK;
char *upath;
char *ade = NULL;
u_char achar, *fdType, xyy[4]; /* uninitialized, OK 310105 */
u_int16_t ashort, bshort, oshort;
u_int32_t aint;
Expand Down Expand Up @@ -1050,7 +1058,9 @@ int setfilparams(struct vol *vol,
ad_setdate(adp, AD_DATE_BACKUP, bdate);
break;
case FILPBIT_FINFO :
if (default_type( ad_entry( adp, ADEID_FINDERI ))
ade = ad_entry(adp, ADEID_FINDERI);
AFP_ASSERT(ade != NULL);
if (default_type(ade)
&& (
((em = getextmap( path->m_name )) &&
!memcmp(finder_buf, em->em_type, sizeof( em->em_type )) &&
Expand All @@ -1061,7 +1071,7 @@ int setfilparams(struct vol *vol,
)) {
memcpy(finder_buf, ufinderi, 8 );
}
memcpy(ad_entry( adp, ADEID_FINDERI ), finder_buf, 32 );
memcpy(ade, finder_buf, 32 );
break;
case FILPBIT_UNIXPR :
if (upriv_bit) {
Expand All @@ -1070,8 +1080,12 @@ int setfilparams(struct vol *vol,
break;
case FILPBIT_PDINFO :
if (afp_version < 30) { /* else it's UTF8 name */
memcpy(ad_entry( adp, ADEID_FINDERI ), fdType, 4 );
memcpy(ad_entry( adp, ADEID_FINDERI ) + 4, "pdos", 4 );
ade = ad_entry(adp, ADEID_FINDERI);
if (ade == NULL) {
return -1;
}
memcpy(ade, fdType, 4);
memcpy(ade + 4, "pdos", 4);
break;
}
/* fallthrough */
Expand Down
7 changes: 5 additions & 2 deletions etc/afpd/volume.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1707,6 +1707,7 @@ static int getvolparams( u_int16_t bitmap, struct vol *vol, struct stat *st, cha
VolSpace xbfree, xbtotal; /* extended bytes */
char *data, *nameoff = NULL;
char *slash;
char *ade = NULL;

LOG(log_debug, logtype_afpd, "getvolparams: Volume '%s'", vol->v_localname);

Expand All @@ -1727,8 +1728,10 @@ static int getvolparams( u_int16_t bitmap, struct vol *vol, struct stat *st, cha
slash = vol->v_path;
if (ad_getentryoff(&ad, ADEID_NAME)) {
ad_setentrylen( &ad, ADEID_NAME, strlen( slash ));
memcpy(ad_entry( &ad, ADEID_NAME ), slash,
ad_getentrylen( &ad, ADEID_NAME ));
ade = ad_entry(&ad, ADEID_NAME);
AFP_ASSERT(ade != NULL);

memcpy(ade, slash, ad_getentrylen( &ad, ADEID_NAME ));
}
vol_setdate(vol->v_vid, &ad, st->st_mtime);
ad_flush(&ad);
Expand Down
8 changes: 7 additions & 1 deletion etc/cnid_dbd/cmd_dbd_scanvol.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,13 @@ static cnid_t check_cnid(const char *name, cnid_t did, struct stat *st, int adfi
cwdbuf, name, strerror(errno));
return CNID_INVALID;
}
ad_setid( &ad, st->st_dev, st->st_ino, db_cnid, did, stamp);
ret = ad_setid( &ad, st->st_dev, st->st_ino, db_cnid, did, stamp);
if (ret == -1) {
dbd_log(LOGSTD, "Error setting new CNID, malformed adouble: '%s/%s'",
cwdbuf, name);
ad_close(&ad, ADFLAGS_HF);
return CNID_INVALID;
}
ad_flush(&ad);
ad_close_metadata(&ad);
}
Expand Down
Loading

5 comments on commit c754d46

@rdmark
Copy link
Owner

@rdmark rdmark commented on c754d46 Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgsga Have you found a pattern with which your crashes occur? On my fork (2.x) I have seen recurring errors with dsi_stream when connecting over DSI specifically (however for whatever reason I haven't seen those crashes on Netatalk 2.2 proper.) DDP works perfectly. Is this what you're seeing as well?

@rdmark
Copy link
Owner

@rdmark rdmark commented on c754d46 Apr 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgsga Oh wait, if your fork is 3.1 based you shouldn't be using my patch above. It's a backport for the 2.2 codebase specifically. Please use the patch in Netatalk#174 !

@rdmark
Copy link
Owner

@rdmark rdmark commented on c754d46 Apr 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I suggest you report this over at the PR, ideally with reproduction steps for upstream 3.1? Andrew is probably the best person to address remaining issues.

@anodos325
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that I could induce the crash by trying to change the default network share icon in MacOS 9 to a colour one i.e. by changing a resource fork. So I changed these lines in ad_open.c:

if (!eid || eid > ADEID_MAX || off >= valid_data_len || ((eid != ADEID_RFORK) && (off + len > valid_data_len))) {...

to:

if (!eid || eid > ADEID_MAX || ((eid != ADEID_RFORK) && (off >= valid_data_len)) || ((eid != ADEID_RFORK) && (off + len > valid_data_len))) {...

This seems to have fixed the crashes for me in the 3.1-based fork.

Is that the same fix that you see at the bottom of my patchset here:
https://github.com/Netatalk/Netatalk/pull/174/files

@anodos325
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is! Somehow my initial application of the PR to my fork came up with a different result for that line of code...

That patchset evolved some as I was testing (and finding bugs). At the conclusion I did a final force-push that squashed all the changes. You probably grabbed an earlier revision.

Please sign in to comment.