sleuthkit-developers

[sleuthkit-developers] TskAuto::isFATSystemFiles does not seem like it can be correct as coded

[Edward D. <eld...@tr...>]

The code for TskAuto::isFATSystemFiles is:

uint8_t
TskAuto::isFATSystemFiles(TSK_FS_FILE *a_fs_file)
{
     if (a_fs_file && a_fs_file->fs_info && a_fs_file->name) {
         FATFS_INFO *fatfs = (FATFS_INFO*)a_fs_file->fs_info;
         TSK_INUM_T addr = a_fs_file->name->meta_addr;
         if ((addr == fatfs->mbr_virt_inum) ||
             (addr == fatfs->fat1_virt_inum) ||
             (addr == fatfs->fat2_virt_inum && fatfs->numfat == 2)) {
             return 1;
         }
     }

     return 0;
}

This code blindly casts a pointer to a TSK_FS_INFO struct to a pointer 
to a FATFS_INFO struct and then tries to access data in the FATFS_INFO 
struct. I am showing this leading to an access violation in some code I 
am developing using TSK. Shouldn't the code instead be:

uint8_t
TskAuto::isFATSystemFiles(TSK_FS_FILE *a_fs_file)
{
     if (a_fs_file && a_fs_file->fs_info && a_fs_file->name
         && TSK_FS_TYPE_ISFAT(a_fs_file->fs_info->ftype)) {
         FATFS_INFO *fatfs = (FATFS_INFO*)a_fs_file->fs_info;
         TSK_INUM_T addr = a_fs_file->name->meta_addr;
         if ((addr == fatfs->mbr_virt_inum) ||
             (addr == fatfs->fat1_virt_inum) ||
             (addr == fatfs->fat2_virt_inum && fatfs->numfat == 2)) {
             return 1;
         }
     }

     return 0;
}

In other words shouldn't the code be checking for the fact that the file 
type is FAT before trying to cast the TSK_FS_INFO pointer to a 
FATFS_INFO pointer ?

I am not cognizant of TSK code but I am a C++ expert and the code does 
not look like it can be correct as is ( besides leading to an access 
violation <g> ).

Eddie Diener

Re: [sleuthkit-developers] TskAuto::isFATSystemFiles does not seem like it can be correct as coded

[Brian C. <ca...@sl...>]

Hey Eddie,

You are correct.  There should be that check in place.  I think our other places do the check at a higher level, but it is certainly safer to do it in the method.  Can you do a pull request for this?

thanks,
brian


> On Jul 20, 2016, at 5:12 PM, Edward Diener <eld...@tr...> wrote:
> 
> The code for TskAuto::isFATSystemFiles is:
> 
> uint8_t
> TskAuto::isFATSystemFiles(TSK_FS_FILE *a_fs_file)
> {
>     if (a_fs_file && a_fs_file->fs_info && a_fs_file->name) {
>         FATFS_INFO *fatfs = (FATFS_INFO*)a_fs_file->fs_info;
>         TSK_INUM_T addr = a_fs_file->name->meta_addr;
>         if ((addr == fatfs->mbr_virt_inum) ||
>             (addr == fatfs->fat1_virt_inum) ||
>             (addr == fatfs->fat2_virt_inum && fatfs->numfat == 2)) {
>             return 1;
>         }
>     }
> 
>     return 0;
> }
> 
> This code blindly casts a pointer to a TSK_FS_INFO struct to a pointer 
> to a FATFS_INFO struct and then tries to access data in the FATFS_INFO 
> struct. I am showing this leading to an access violation in some code I 
> am developing using TSK. Shouldn't the code instead be:
> 
> uint8_t
> TskAuto::isFATSystemFiles(TSK_FS_FILE *a_fs_file)
> {
>     if (a_fs_file && a_fs_file->fs_info && a_fs_file->name
>         && TSK_FS_TYPE_ISFAT(a_fs_file->fs_info->ftype)) {
>         FATFS_INFO *fatfs = (FATFS_INFO*)a_fs_file->fs_info;
>         TSK_INUM_T addr = a_fs_file->name->meta_addr;
>         if ((addr == fatfs->mbr_virt_inum) ||
>             (addr == fatfs->fat1_virt_inum) ||
>             (addr == fatfs->fat2_virt_inum && fatfs->numfat == 2)) {
>             return 1;
>         }
>     }
> 
>     return 0;
> }
> 
> In other words shouldn't the code be checking for the fact that the file 
> type is FAT before trying to cast the TSK_FS_INFO pointer to a 
> FATFS_INFO pointer ?
> 
> I am not cognizant of TSK code but I am a C++ expert and the code does 
> not look like it can be correct as is ( besides leading to an access 
> violation <g> ).
> 
> Eddie Diener
> 
> ------------------------------------------------------------------------------
> What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
> patterns at an interface-level. Reveals which users, apps, and protocols are 
> consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
> J-Flow, sFlow and other flows. Make informed decisions using capacity planning
> reports.http://sdm.link/zohodev2dev
> _______________________________________________
> sleuthkit-developers mailing list
> sle...@li...
> https://lists.sourceforge.net/lists/listinfo/sleuthkit-developers