Skip to content

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. 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>
  • Loading branch information
@anodos325 @slowfranklin
anodos325 authored and slowfranklin committed Jul 12, 2022
1 parent 0f2f30d commit 9d0c212
Show file tree
Hide file tree
Showing 9 changed files with 224 additions and 59 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 @@ -1426,6 +1426,7 @@ int getdirparams(const AFPObj *obj,
struct maccess ma;
struct adouble ad;
char *data, *l_nameoff = NULL, *utf_nameoff = NULL;
char *ade = NULL;
int bit = 0, isad = 0;
uint32_t aint;
uint16_t ashort;
Expand Down Expand Up @@ -1520,7 +1521,10 @@ int getdirparams(const AFPObj *obj,

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 @@ -1744,6 +1748,7 @@ int setdirparams(struct vol *vol, struct path *path, uint16_t d_bitmap, char *bu
struct timeval tv;

char *upath;
char *ade = NULL;
struct dir *dir;
int bit, isad = 0;
int cdate, bdate;
Expand Down Expand Up @@ -1905,6 +1910,8 @@ int setdirparams(struct vol *vol, struct path *path, uint16_t d_bitmap, char *bu
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 @@ -1915,10 +1922,10 @@ int setdirparams(struct vol *vol, struct path *path, uint16_t d_bitmap, char *bu
* 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
35 changes: 28 additions & 7 deletions etc/afpd/file.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,7 @@ int getmetadata(const AFPObj *obj,
{
char *data, *l_nameoff = NULL, *upath;
char *utf_nameoff = NULL;
char *ade = NULL;
int bit = 0;
uint32_t aint;
cnid_t id = 0;
Expand Down Expand Up @@ -497,7 +498,10 @@ int getmetadata(const AFPObj *obj,
}
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 @@ -576,8 +580,19 @@ int getmetadata(const AFPObj *obj,
10.3 clients freak out. */

aint = st->st_mode;
if (adp) {
memcpy(fdType, ad_entry( adp, ADEID_FINDERI ), 4 );
/*
* ad_open() does not initialize adouble header
* for symlinks. Hence this should be skipped to
* avoid AFP_ASSERT here. Decision was made to
* not alter ad_open() behavior so that
* improper ops on symlink adoubles will be
* more visible (assert).
*/
if (adp && (ad_meta_fileno(adp) != AD_SYMLINK)) {
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 @@ -839,6 +854,7 @@ int setfilparams(const AFPObj *obj, 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 */
uint16_t ashort, bshort, oshort;
uint32_t aint;
Expand Down Expand Up @@ -1042,7 +1058,9 @@ int setfilparams(const AFPObj *obj, 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 @@ -1053,17 +1071,20 @@ int setfilparams(const AFPObj *obj, 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) {
setfilunixmode(vol, path, upriv);
}
break;
case FILPBIT_PDINFO :
ade = ad_entry(adp, ADEID_FINDERI);
AFP_ASSERT(ade != NULL);

if (obj->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 );
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 @@ -308,6 +308,7 @@ static int getvolparams(const AFPObj *obj, uint16_t bitmap, struct vol *vol, str
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 @@ -331,8 +332,10 @@ static int getvolparams(const AFPObj *obj, uint16_t bitmap, struct vol *vol, str
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
9 changes: 8 additions & 1 deletion etc/cnid_dbd/cmd_dbd_scanvol.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,7 @@ static int read_addir(void)
static cnid_t check_cnid(const char *name, cnid_t did, struct stat *st, int adfile_ok)
{
int adflags = ADFLAGS_HF;
int err;
cnid_t db_cnid, ad_cnid;
struct adouble ad;

Expand Down Expand Up @@ -602,7 +603,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);
err = ad_setid( &ad, st->st_dev, st->st_ino, db_cnid, did, stamp);
if (err == -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(&ad, ADFLAGS_HF);
}
Expand Down

0 comments on commit 9d0c212

Please sign in to comment.