volatility3 icon indicating copy to clipboard operation
volatility3 copied to clipboard

Fix a bug with _get_do_path() in LinuxUtilities

Open paulkermann opened this issue 3 years ago • 5 comments

_do_get_path uses dentry.path(), which returned the name of dentry (d_name). Later, dentry.path() was changed to return a full path, but _do_get_path wasn't altered accordignly.

This PR just extract the last component from the path returned by dentry.path(), so it will behave the same way as before.

paulkermann avatar Jul 13 '22 10:07 paulkermann

It looks like the change happened in commit e8fa6e1e7faac7ac1b70284a0d14e4a5ef30ceba and we didn't bump any version numbers at that time. @atcuno, can you please advise the best way of dealing with this inconsistency?

ikelos avatar Jul 13 '22 10:07 ikelos

@gcmoreira you may be interested in this too

ikelos avatar Jul 13 '22 10:07 ikelos

That makes sense. As __dentry_path wasn't returning the dentry path but the dentry name. Instead of building the path for then split it, it would be better to simply do:

dname = dentry.d_name.name_as_str()

that's the dentry name (dname) string, but please double-check it.

However, something I noticed is that function adds some extra code that is not even in the kernel counterparts __d_path() and prepend_path(), and based on my experience that made it hard to reuse and also lead to incorrect dentry paths. Unfortunately, many other parts of the code depend on this function and I wanted the PR be self-contained.

Having said that, I would like to completely replace _do_get_path() at some point. Mountinfo plugin implements its own version which mimics the linux kernel prepend_path(). That's why it wasn't affected and also the reason why I missed that case.

All the above was already clarified in the PR description.

  • Adds a new _do_get_path() implementation based on the Linux kernel source code. The current implementation in LinuxUtilities, which is a legacy of Volatility 2, is incorrect and leads to inaccurate information. Unfortunately, I was unable to fix that one because it was adapted with some hacks/workaround to provide same extra functionality to other parts of the framework which made hard to extend.
  • Replaced linux/extension dentry->path() method with a implementation which mimics the Linux kernel __dentry_path() function. The old one didn't return the path but just the dentry name.

gcmoreira avatar Jul 14 '22 03:07 gcmoreira

@atcuno This is still pending your ok, please?

ikelos avatar Jul 31 '22 16:07 ikelos

Ok, so after discussion today it sounds as though there's development going on to rework the core of this, but I don't want it to hold up the 2.4.0 release. Are we ok releasing with this known issue, or should we apply this and then potentially have unidentified bugs due to the change, until we get the major shift in place? @atcuno @gcmoreira

ikelos avatar Sep 21 '22 20:09 ikelos

@ikelos after reading the current code and some users of the different paths into, @gcmoreira's comment above to use:

dname = dentry.d_name.name_as_str

still stands as the correct way. This will get just the name of the file instead of the path, versus changing a function meant to return the path. I believe this can be closed now.

atcuno avatar Mar 10 '23 20:03 atcuno

Thanks very much for looking into it @atcuno, marking as closed.

ikelos avatar Mar 10 '23 22:03 ikelos