Open arepi-nemui opened 2 months ago
ikelos
commented
2 months ago This was a commit made by @gcmoreira , hopefully he can diagnose what's now happening here. I've reverted it for now, pending review to see what problem it was causing. It might be it was too restrictive, or there may be more off-by-one issues in the codebase.
Thanks for providing the memory image that caused the problems, that should help a lot in discovering the issue...
gcmoreira
commented
2 months ago Hm that's weird, now I'm even more curious why the test cases didn't fail. @ikelos Any idea?
@ikelos and @arepi-nemui
Anyway, this is why I thought that if is dead code, not matter what the values are.
>>> 1 > 5 > 10
False
>>> 1 > 100 > 10
False
>>> 1 > -1 > 10
FalseAnd this is what we want to detect to raise the exception:
>>> not (1 <= 5 <= 10)
False
>>> not (1 <= 100 <= 10)
True
>>> not (1 <= -1 <= 10)
TrueSo, I think here there is bug somewhere else that was never detected because of that funny if.
Setting a breakpoint in that line I got this:
>> hex(self.minimum_address), hex(offset), hex(self.maximum_address)
('0x0', '0xffffc60795389024', '0xffffffffffff')So, @ikelos the fix I proposed was actually correct.
The problem is that offset is a canonical address. If instead we use the line below it works as expected:
>> not (self.minimum_address <= self.decanonicalize(offset) <= self.maximum_address)
FalseUnfortunately, it seems that _translate_entry is called with both canonical and not canonical addresses.
It looks like this will work for all cases, but it needs to be tested properly.
>> not (self.minimum_address <= offset <= self.canonicalize(self.maximum_address))
FalseI will need more time to figure out how to properly fix this. Feel free to continue investigating this if you'd like.
atcuno
commented
2 months ago if translate_entry is the broken code path for all cases then it should either normalize offsets sent in relative t self.maximum_address or the form Gus sent with self.canonicalize(self.maximum_address) should be used. It feels like offset should be normalized by translate_entry before its used versus trying to enforce a long term meaning to self.maximum_address.
ikelos
commented
1 month ago Ugh, I'm really tired of the canonicalize/non-canonicalize issue. We made it so that pointers are always valid addresses in a space (by masking them to the address space's limit) then people want to compare them to random bytes they found somewhere, which included the high bits. If we make the change, someone will complain that they've got the other type of address and it says it's wrong, or they were expecting volatility to throw an exception or they constructed an address that's correct, but volatility says it's invalid because they didn't bother with the high bits.
There is no right way of doing this, the way we currently handle it is established volatility behaviour. We can change it, but someone will need to test every core plugin carefully and it will never get changed back (meaning someone somewhere will always end up writing code that needs to handle one or the other of these situations). If you want to take that on and agree to handle all tickets arising from it, by all means, I'll support whichever route people want to take, but my time is far more scarce these days, so I'd be happier to just sort it in this plugin rather than making a major change to core.
I'd be happy with:
>> not (self.minimum_address <= (offset & self.address_mask) <= self.maximum_address)
Seems to be what we care about, that the masked address fits in the possible size of the address space?
We have all the tools to do the right calculations, canonicalization just seems to be a distraction...
gcmoreira
commented
1 month ago @ikelos Yeah, it makes sense. I think the line you suggested is the safest way to go with this.
Could you change that or do you want me to create a PR for this?
gcmoreira
commented
1 month ago Regarding canonical addresses, the canonical prefix doesn't look right to me.
>>> hex(self._canonical_prefix)
'0xffff000000000000'Unless I'm missing something, it should be 0xffff800000000000.
From the Intel® 64 and IA-32 Architectures Software Developer’s Manual:
A canonical address must have bits 63 through 48 set to zeros or ones (depending on whether bit 47 is a zero or one)
Which makes the higher half start on 0xffff800000000000.
So IMO the canonical mask is missing a -1.
>>> hex(self._mask(
(1 << self._bits_per_register) - 1,
self._bits_per_register,
self._maxvirtaddr - 1,
))
'0xffff800000000000'Am I missing something here?
ikelos
commented
1 month ago The top bits must follow the highest actual bit. So do de-canonicalize it, you mask the bits that were set to the same as the highest actual bit, not the highest actual bit itself. If you have an address 0xffff800000001234, masking it by the one you proposed would give you 0x1234, not 0x800000001234. Kernel space would disappear... 5;P
ikelos
commented
1 month ago The point is bits 0-47 are valid, and in your mask, only bits 0-46 make it through...
ikelos
commented
1 month ago Ok, I've implemented this in a pull request but to avoid apply and having to revert it if something goes wrong, please could you test this branch @arepi-nemui? Also @gcmoreira if you could give it a look over and make sure it does what you'd originally intended, I'd appreciate it... 5:)
Describe the bug After commit e5a5b895771b655d21c36689c33a534034c31e36, volatility is no longer able to run various modules on a memory dump from a Windows 10 system, such as windows.hashdump.Hashdump and windows.info.Info.
Context Volatility Version: Commit e5a5b895771b655d21c36689c33a534034c31e36 and later Operating System: Linux Python Version: 3.12.3 Suspected Operating System: Windows 10 Command:
python vol.py -f memory.dmp -vvv windows.hashdump.HashdumpTo Reproduce Steps to reproduce the behavior:
Example output
Additional information memory.dmp: https://www.mediafire.com/file/c0v7xmu0f6kq2lr/memory.dmp.gz/file