sleuthkit-developers

[sleuthkit-developers] sleuthkit bug in fs_data.c

[David C. <da...@in...>]

G'Day Brian,

I believe I have found a bug in sleuthkit.

in fs_data.c:

fs_data_getnew_attr can create unused 'holes' in attr lists. At least
two functions, fs_data_lookup and fs_data_lookup_noid do not jump the
holes which can cause them to return early without finding the correct
FS_DATA element to return.
 
This does not seem to effect sleuthkit (though I havent done much
testing), but it does effect pyflag. Probably because we live for longer
and perform more walks etc than the sk tools. As such there is more
opportunities for structure reuse and hence more chances to hit this
bug.
 
My solution for now is to change the loops in fs_data_lookup and
fs_data_lookup_noid to jump the holes (patch attached), though another
possibility is to modify fs_data_getnew_attr so that it does not create
holes in the first place. This may be more complicated and undesirable
though.

My patch *only* fixes the loops in fs_data. A quick grep through the
code reveals several more similar loops which could cause trouble
(though through luck, they probably dont!).

Let me know what you think or if you need a better description of the
problem, I can fix up the rest for you and send another patch if you
like.

Thanks,
David Collett

Re: [sleuthkit-developers] sleuthkit bug in fs_data.c

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

Indeed you are correct.  I just fixed it (the next version will 
hopefully be out next week once I track down an elusive NTFS compression 
bug).

Where else did you see this issue?  I think fs_data is the only 
structure that is reused.

thanks,
brian

David Collett wrote:
> G'Day Brian,
> 
> I believe I have found a bug in sleuthkit.
> 
> in fs_data.c:
> 
> fs_data_getnew_attr can create unused 'holes' in attr lists. At least
> two functions, fs_data_lookup and fs_data_lookup_noid do not jump the
> holes which can cause them to return early without finding the correct
> FS_DATA element to return.
>  
> This does not seem to effect sleuthkit (though I havent done much
> testing), but it does effect pyflag. Probably because we live for longer
> and perform more walks etc than the sk tools. As such there is more
> opportunities for structure reuse and hence more chances to hit this
> bug.
>  
> My solution for now is to change the loops in fs_data_lookup and
> fs_data_lookup_noid to jump the holes (patch attached), though another
> possibility is to modify fs_data_getnew_attr so that it does not create
> holes in the first place. This may be more complicated and undesirable
> though.
> 
> My patch *only* fixes the loops in fs_data. A quick grep through the
> code reveals several more similar loops which could cause trouble
> (though through luck, they probably dont!).
> 
> Let me know what you think or if you need a better description of the
> problem, I can fix up the rest for you and send another patch if you
> like.
> 
> Thanks,
> David Collett
> 
> 
> ------------------------------------------------------------------------
> 
> diff -ruN sleuthkit-2.06/src/fstools/fs_data.c sleuthkit-2.06-dave/src/fstools/fs_data.c
> --- sleuthkit-2.06/src/fstools/fs_data.c	2006-09-02 02:09:15.000000000 +1000
> +++ sleuthkit-2.06-dave/src/fstools/fs_data.c	2006-11-17 18:10:59.615647500 +1100
> @@ -242,9 +242,11 @@
>  	return NULL;
>      }
>  
> -    while ((fs_data) && (fs_data->flags & FS_DATA_INUSE) &&
> -	((fs_data->type != type) || (fs_data->id != id)))
> -	fs_data = fs_data->next;
> +    while (fs_data) {
> +        if((fs_data->flags & FS_DATA_INUSE) && (fs_data->type == type) && (fs_data->id == id))
> +            break;
> +        fs_data = fs_data->next;
> +    }
>  
>      if ((!fs_data) || (fs_data->type != type) || (fs_data->id != id)) {
>  	return NULL;
> @@ -280,8 +282,8 @@
>       * lowest id of the given type (if more than one exists) 
>       */
>  
> -    while ((fs_data) && (fs_data->flags & FS_DATA_INUSE)) {
> -	if (fs_data->type == type) {
> +    while (fs_data) {
> +	if ((fs_data->flags & FS_DATA_INUSE) && fs_data->type == type) {
>  
>  	    /* replace existing if new is lower */
>  	    if ((!fs_data_ret) || (fs_data_ret->id > fs_data->id))
> 
> 
> ------------------------------------------------------------------------
> 
> -------------------------------------------------------------------------
> Take Surveys. Earn Cash. Influence the Future of IT
> Join SourceForge.net's Techsay panel and you'll get the chance to share your
> opinions on IT & business topics through brief surveys - and earn cash
> http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> sleuthkit-developers mailing list
> sle...@li...
> https://lists.sourceforge.net/lists/listinfo/sleuthkit-developers

Re: [sleuthkit-developers] sleuthkit bug in fs_data.c

[David C. <da...@in...>]

Hi Brian,

On Fri, Nov 17, 2006 at 05:13:28PM -0500, Brian Carrier wrote:
> Indeed you are correct.  I just fixed it (the next version will 
> hopefully be out next week once I track down an elusive NTFS compression 
> bug).

I think I've seen this too, on friday afternoon, I didnt get much time
to look into it. glibc detecting a double free in ntfs_uncompress_done??

Let me know this week if you want me to test your solution.

> Where else did you see this issue?  I think fs_data is the only 
> structure that is reused.

If you grep the sources in src/fstools for FS_DATA_INUSE, you will see a few
more while loops with the same structure e.g.:

...
dcalc_lib.c:151:        while ((fs_data) && (fs_data->flags & FS_DATA_INUSE)) {
dls_lib.c:178:  while ((fs_data) && (fs_data->flags & FS_DATA_INUSE)) {
fls_lib.c:137:      while ((fs_data) && (fs_data->flags & FS_DATA_INUSE)) {
...
(there are more)

These loops will also quit early if they see a hole right? If your
solution was to change fs_data_getnew_attr, then they dont matter, but
if you used my patch, these loops will have to be fixed too.

Thanks,
Dave

> David Collett wrote:
> >G'Day Brian,
> >
> >I believe I have found a bug in sleuthkit.
> >
> >in fs_data.c:
> >
> >fs_data_getnew_attr can create unused 'holes' in attr lists. At least
> >two functions, fs_data_lookup and fs_data_lookup_noid do not jump the
> >holes which can cause them to return early without finding the correct
> >FS_DATA element to return.
> > 
> >This does not seem to effect sleuthkit (though I havent done much
> >testing), but it does effect pyflag. Probably because we live for longer
> >and perform more walks etc than the sk tools. As such there is more
> >opportunities for structure reuse and hence more chances to hit this
> >bug.
> > 
> >My solution for now is to change the loops in fs_data_lookup and
> >fs_data_lookup_noid to jump the holes (patch attached), though another
> >possibility is to modify fs_data_getnew_attr so that it does not create
> >holes in the first place. This may be more complicated and undesirable
> >though.
> >
> >My patch *only* fixes the loops in fs_data. A quick grep through the
> >code reveals several more similar loops which could cause trouble
> >(though through luck, they probably dont!).
> >
> >Let me know what you think or if you need a better description of the
> >problem, I can fix up the rest for you and send another patch if you
> >like.
> >
> >Thanks,
> >David Collett
> >
> >
> >------------------------------------------------------------------------
> >
> >diff -ruN sleuthkit-2.06/src/fstools/fs_data.c 
> >sleuthkit-2.06-dave/src/fstools/fs_data.c
> >--- sleuthkit-2.06/src/fstools/fs_data.c	2006-09-02 
> >02:09:15.000000000 +1000
> >+++ sleuthkit-2.06-dave/src/fstools/fs_data.c	2006-11-17 
> >18:10:59.615647500 +1100
> >@@ -242,9 +242,11 @@
> > 	return NULL;
> >     }
> > 
> >-    while ((fs_data) && (fs_data->flags & FS_DATA_INUSE) &&
> >-	((fs_data->type != type) || (fs_data->id != id)))
> >-	fs_data = fs_data->next;
> >+    while (fs_data) {
> >+        if((fs_data->flags & FS_DATA_INUSE) && (fs_data->type == type) && 
> >(fs_data->id == id))
> >+            break;
> >+        fs_data = fs_data->next;
> >+    }
> > 
> >     if ((!fs_data) || (fs_data->type != type) || (fs_data->id != id)) {
> > 	return NULL;
> >@@ -280,8 +282,8 @@
> >      * lowest id of the given type (if more than one exists) 
> >      */
> > 
> >-    while ((fs_data) && (fs_data->flags & FS_DATA_INUSE)) {
> >-	if (fs_data->type == type) {
> >+    while (fs_data) {
> >+	if ((fs_data->flags & FS_DATA_INUSE) && fs_data->type == type) {
> > 
> > 	    /* replace existing if new is lower */
> > 	    if ((!fs_data_ret) || (fs_data_ret->id > fs_data->id))
> >
> >
> >------------------------------------------------------------------------
> >
> >-------------------------------------------------------------------------
> >Take Surveys. Earn Cash. Influence the Future of IT
> >Join SourceForge.net's Techsay panel and you'll get the chance to share 
> >your
> >opinions on IT & business topics through brief surveys - and earn cash
> >http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
> >
> >
> >------------------------------------------------------------------------
> >
> >_______________________________________________
> >sleuthkit-developers mailing list
> >sle...@li...
> >https://lists.sourceforge.net/lists/listinfo/sleuthkit-developers

Re: [sleuthkit-developers] sleuthkit bug in fs_data.c

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

David Collett wrote:
> Hi Brian,
> 
> On Fri, Nov 17, 2006 at 05:13:28PM -0500, Brian Carrier wrote:
>> Indeed you are correct.  I just fixed it (the next version will 
>> hopefully be out next week once I track down an elusive NTFS compression 
>> bug).
> 
> I think I've seen this too, on friday afternoon, I didnt get much time
> to look into it. glibc detecting a double free in ntfs_uncompress_done??
> 
> Let me know this week if you want me to test your solution.

My initial fix for this problem turned into an infinite loop.  It 
appears to happen when the file is unallocated and the clusters 
allocated to the file have been reused.  The new data screws up the 
compression algorithm somehow...  If you have a file that causes that 
message though, I could use your help debugging this.


>> Where else did you see this issue?  I think fs_data is the only 
>> structure that is reused.
> 
> If you grep the sources in src/fstools for FS_DATA_INUSE, you will see a few
> more while loops with the same structure e.g.:
> 
> ...
> dcalc_lib.c:151:        while ((fs_data) && (fs_data->flags & FS_DATA_INUSE)) {
> dls_lib.c:178:  while ((fs_data) && (fs_data->flags & FS_DATA_INUSE)) {
> fls_lib.c:137:      while ((fs_data) && (fs_data->flags & FS_DATA_INUSE)) {
> ...
> (there are more)

Oh, I get it now.  I thought you meant there were other structures that 
had a similar design problem.  I think I will both update the loops and 
update the _alloc methods so that the used attributes are moved to the 
head of the list.

thanks,
brian