Layers - Several fixes around maximum_address and chunk sizes #1141
Conversation
…nsion. If the argument is i.e. "--output aaa" ... it returned ".aaa" (hidden filename in linux) then "-1.aaa", "-2.aaa", etc.
…e but never mentioned as a requirement
…nimum_address > maximum_address" which doesn't make sense to me.
|
I was wrong with e5a5b89, it should be fine. Anyway, I think it is a more common way to express this kind of check and is generally considered more readable. |
|
Tests: Vmem imageVol2$ python2 ../volatility2/vol.py imagecopy -f ../ubuntu2004lts64bit/ubuntu2004lts64bit-Snapshot35.vmem -O ubuntu2004lts64bit-Snapshot35.vmem.raw.vol2
Volatility Foundation Volatility Framework 2.6
Writing data (5.00 MB chunks): |.......................................................................................................|Vol3$ python3 ../volatility3/vol.py -f ../ubuntu2004lts64bit/ubuntu2004lts64bit-Snapshot35.vmem LayerWriter --output ubuntu2004lts64bit-Snapshot35.vmem.raw.vol3
Volatility 3 Framework 2.7.0
Progress: 100.00 PDB scanning finished
Status
Progress: 99.61 Writing layer primary
Layer has been written to /home/user/vol3-imagecopy/ubuntu2004lts64bit-Snapshot35.vmem.raw.vol3Test-rw------- 1 user user 536870912 May 15 2023 /media/user/ubuntu2004lts64bit/ubuntu2004lts64bit-Snapshot35.vmem
-rw-rw-r-- 1 user user 536870912 May 6 16:09 ubuntu2004lts64bit-Snapshot35.vmem.raw.vol2
-rw------- 1 user user 536870912 May 6 16:09 ubuntu2004lts64bit-Snapshot35.vmem.raw.vol3
$ sha1sum ubuntu2004lts64bit-Snapshot35.vmem.raw.vol2 ubuntu2004lts64bit-Snapshot35.vmem.raw.vol3
48206033995ab862fdf729ed4bf2a8bd104654ab ubuntu2004lts64bit-Snapshot35.vmem.raw.vol2
48206033995ab862fdf729ed4bf2a8bd104654ab ubuntu2004lts64bit-Snapshot35.vmem.raw.vol3Core imageVol2$ python2 ../volatility2/vol.py imagecopy -f ./ram-4.10.0-42.core -O ram-4.10.0-42.core.raw.vol2
Volatility Foundation Volatility Framework 2.6
Writing data (5.00 MB chunks): |...........................................................................................................................................................................................................................................|Vol3+ ./vol.py -f ./ram-4.10.0-42.core LayerWriter --output ram-4.10.0-42.core.raw.vol3
Volatility 3 Framework 2.7.0
Progress: 100.00 PDB scanning finished
Status
Progress: 99.98 Writing layer primary
Layer has been written to /home/user/vol3-imagecopy/ram-4.10.0-42.core.raw.vol3Test-rw-r--r-- 1 user user 1208166496 May 6 07:51 ./ram-4.10.0-42.core
-rw-rw-r-- 1 user user 4294967296 May 6 16:10 ram-4.10.0-42.core.raw.vol2
-rw------- 1 user user 4294967296 May 6 16:10 ram-4.10.0-42.core.raw.vol3
$ sha1sum ram-4.10.0-42.core.raw.vol2 ram-4.10.0-42.core.raw.vol3
36a504a0f1e203a6d6cb9bc1a4c0657fc3a47c97 ram-4.10.0-42.core.raw.vol2
36a504a0f1e203a6d6cb9bc1a4c0657fc3a47c97 ram-4.10.0-42.core.raw.vol3
|
There was a problem hiding this comment.
Happy with most of it, just the way of exposing the finally written filename. I'm happy to change the API if needed, or to use the preferred_filename value which was the original intention if it's not too confusing, but it needs to be one of those two options. Accessing a new uninitialized private variable is a no-no...
|
In future it might be easier to submit these as separate, smaller PRs, just so that the trivial ones are easy to put through, and don't get held up by any of the others. The |
|
Cool! I used commits for that. Each commit addresses a specific thing. I always try my best to keep them separated and organized. Sorry, I thought that way will also work for you. There are only two commits ( 6d22347 and d7aae3a) that are not ok. Would you be okay with cherry-picking the commits, or would you prefer if I reverted those ones? In summary: Ignore:Merge
Up to you but I think these improves the Python code readability. |
|
I think I'd prefer you to revert them, just so I can do it through the github UI easily, if that's ok? I'm ok with all the ones except the two you marked as ignore (but again, I'll want to check through the whole thing as one to make sure it makes sense, if that's ok)... |
|
Ok, reverted those commits and redid it using the $ ./vol.py -f ./ram-4.10.0-42.core LayerWriter --layer primary
Volatility 3 Framework 2.7.0
Progress: 100.00 PDB scanning finished
Status
Progress: 99.98 Writing layer primary
Layer has been written to primary.raw
$ ./vol.py -f ram-5.8.0-53.vmem LayerWriter
Volatility 3 Framework 2.7.0
Progress: 100.00 PDB scanning finished
Status
Progress: 99.61 Writing layer primary
Layer has been written to primary-1.raw
$ ./vol.py -f ram-5.8.0-53.vmem LayerWriter
Volatility 3 Framework 2.7.0
Progress: 100.00 PDB scanning finished
Status
Progress: 99.61 Writing layer primary
Layer has been written to primary-2.raw
$ rm primary.raw
$ ./vol.py -f ./ram-4.10.0-42.core LayerWriter --layer primary
Volatility 3 Framework 2.7.0
Progress: 100.00 PDB scanning finished
Status
Progress: 99.98 Writing layer primary
Layer has been written to primary.raw |
|
@ikelos Another thing I noticed and would like to check with you is the handling of the temporary file. Before renaming it to the final filename, it’s created in the current directory instead of the system’s temporary directory. This might have been intentional since these files are usually large, but I'm not entirely sure. Thoughts? |
|
Last but not least, do you have a way to test what I mentioned in the description? maybe @atcuno can help with that
diff --git a/volatility3/framework/layers/msf.py b/volatility3/framework/layers/msf.py
index 8d84a774..6ad6b1fa 100644
--- a/volatility3/framework/layers/msf.py
+++ b/volatility3/framework/layers/msf.py
@@ -253,8 +253,11 @@ class PdbMSFStream(linear.LinearlyMappedLayer):
@property
def maximum_address(self) -> int:
- return self.config.get(
- "maximum_size", len(self._pages) * self._pdb_layer.page_size
+ return (
+ self.config.get(
+ "maximum_size", len(self._pages) * self._pdb_layer.page_size
+ )
+ - 1
)
@property
|
|
Lemme know if you're happy for this to be merged as is, or if you want to throw in the fix for maximum_size/maximum_address? |
It is, because a path makes no sense if this is being handled by a web user interface, for example. The whole framework has to be seen through a UI agnostic lens. The way of returning files can be dealt with by a UI however it wants, but we can't make assumptions about the results being tied to a filesystem. Happy for the temporary file to be constructed in a temporary directory (although kept in memory may be bad depending on how much data someone wants to write to the file). I don't know enough about the test you're suggesting, but I'm happy to accept your changes if you say they're needed. Might be best to check with @atcuno, but I'm happier with the patch now. Thanks for making the requested changes... 5:) |
|
Yeah, probably better merge it as is, and we can always create a new ticket for the other changes. Thanks |
maximum_addressfixes in several layersI think there are still other places to fix but I haven't fixed everything as testing it will be massive. I think we should review all occurrences of
layer.maximum_addressand where we calculate the data size based on that value. There are other places where I suspect it's being used incorrectly. To mention just a few:len(self._pages) * self._pdb_layer.page_sizeshould be the next byte following themaximum_address.maximum_addressis in a while condition but the value is not included in the address range i.e.: lime, or avml. Technically looks wrong but I think these could still be ok because they are expecting more than 1 byte to read anyway. So it should mean that the layer/file is truncated/bad format/etc.CommandLine: file_handler_class_factory ->CLIFileHandler
For instance, if we call the LayerWriter plugin with --output aaa ... it creates a .aaa (hidden filename in linux). Next iterations using the same argument will generate -1.aaa, -2.aaa, etc. instead of aaa, aaa-1, and aaa-2 respectively.Stored the real filename created infile_handle._output_filename. That way we can know the final filename including both the "--output-dir" and the "--output" values and its "-1", "-2", etc (if used). Otherwise, the filename reported will be incorrect (and old version of this file), which could result in inaccurate analysis. Unfortunately, the final filename is determined when the file is closed, which might lead to some controversy regarding its implementation.file_handle.preferred_filename. Otherwise, the filename reported will be incorrect (an old version of this file), which could result in inaccurate analysis. The final filename will contain the layer name. i.e.primary.raw,primary-1.raw, etcLayerWriter plugin. Several fixes
- Fixed missing--outputargument and other bugs, plus some code improvements.