Issue 327070043: [plaso] Add amcache parser #740 (Closed)
|
Created:
6 years, 8 months ago by rbdebeer Modified:
6 years, 7 months ago CC:
Joachim Metz, romaing, kiddi, log2timeline-dev_googlegroups.com, aaronp Visibility:
Public. |
Description[plaso] Add amcache parser #740
Patch Set 1 #
Total comments: 26
Patch Set 2 : Process codereview comments and fix bugs #
Total comments: 9
Patch Set 3 : Process codereview comments #
Total comments: 12
Patch Set 4 : Process more codereview comments. #
MessagesTotal messages: 14
https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py File plaso/parsers/amcache.py (right): https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:3: add "from __future__ import unicode_literals" and drop all 'u' prefixes on strings https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:16: """Amcache event data. +blank line https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:18: amcachedatetime (str): last modified time of amcache file referenced This doesn't need to be in the eventdata, it's in the event instead. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:20: sha1 (str): sha1 of file Why not extract all the data from the key? Product name, language code etc.? It looks pretty trivial to do so. It also like it would be useful to include the volume guid, ntfs file id and sequence number as well. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:42: _SOURCE_APPEND = u': Amcache Entries' Remove this - source_append is going away, and this value doesn't do anything anyway, you'd need to add is an attribute to the event data. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:47: _AMCACHE_ROOT_KEY = "Root\\File" What about Root\\Programs? It also looks like it might have some useful data. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:50: #TODO Add GetFormatSpecification when issues are fixed with adding Please open a github issue for this, and reference it in the todo https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:66: except IOError as exception: #pylint: disable=unused-variable you can just "except IOError" here, and avoid the pylint directive. Please also add a comment about why this error is ignored, referencing the TODO above. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:83: for amcache_key in range(amcache_keys): This seems a little wierd - why not "for amcache_key in volume_key.sub_keys"? https://codereview.appspot.com/327070043/diff/1/tests/parsers/amcache.py File tests/parsers/amcache.py (right): https://codereview.appspot.com/327070043/diff/1/tests/parsers/amcache.py#newc... tests/parsers/amcache.py:4: Add "from __future__ import unicode_iterals", and then drop all the 'u' prefixes from strings. https://codereview.appspot.com/327070043/diff/1/tests/parsers/amcache.py#newc... tests/parsers/amcache.py:17: @shared_test_lib.skipUnlessHasTestFile([u'Amcache.hve']) Please also add a test for parsing a non-matching file - ideally a non-amcache registry file. https://codereview.appspot.com/327070043/diff/1/tests/parsers/amcache.py#newc... tests/parsers/amcache.py:33: expected_full_path = (u'c:\\users\\user\\appdata\\local\\microsoft\\' Line break after ( https://codereview.appspot.com/327070043/diff/1/tests/parsers/amcache.py#newc... tests/parsers/amcache.py:37: expected_sha1 = u'0000818b581a471c1c6833839d35a9d6f3544f6a9c92' This doesn't look like a sha1 - 43 characters long, due to leading zeros. I recommend stripping the extra zeros off here, or renaming the field.
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
Comments processed, Root/File key information extended and Root/Program key functionality added. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py File plaso/parsers/amcache.py (right): https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:3: On 2017/08/09 10:38:17, onager wrote: > add "from __future__ import unicode_literals" and drop all 'u' prefixes on > strings Done. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:16: """Amcache event data. On 2017/08/09 10:38:17, onager wrote: > +blank line Done. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:18: amcachedatetime (str): last modified time of amcache file referenced On 2017/08/09 10:38:17, onager wrote: > This doesn't need to be in the eventdata, it's in the event instead. Done. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:20: sha1 (str): sha1 of file On 2017/08/09 10:38:17, onager wrote: > Why not extract all the data from the key? Product name, language code etc.? It > looks pretty trivial to do so. > > It also like it would be useful to include the volume guid, ntfs file id and > sequence number as well. The 3 values extracted are always there, the other values are optional. Added extra amcache info from "File" key to events. Created new events if info was a timestamp. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:42: _SOURCE_APPEND = u': Amcache Entries' On 2017/08/09 10:38:17, onager wrote: > Remove this - source_append is going away, and this value doesn't do anything > anyway, you'd need to add is an attribute to the event data. Done. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:47: _AMCACHE_ROOT_KEY = "Root\\File" On 2017/08/09 10:38:17, onager wrote: > What about Root\\Programs? It also looks like it might have some useful data. It maps to the control panel -> installed programs entries. Parsing those out would be easy. I'll add some code. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:66: except IOError as exception: #pylint: disable=unused-variable On 2017/08/09 10:38:17, onager wrote: > you can just "except IOError" here, and avoid the pylint directive. > > Please also add a comment about why this error is ignored, referencing the TODO > above. Done. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:66: except IOError as exception: #pylint: disable=unused-variable On 2017/08/09 10:38:17, onager wrote: > you can just "except IOError" here, and avoid the pylint directive. > > Please also add a comment about why this error is ignored, referencing the TODO > above. Done. https://codereview.appspot.com/327070043/diff/1/plaso/parsers/amcache.py#newc... plaso/parsers/amcache.py:83: for amcache_key in range(amcache_keys): On 2017/08/09 10:38:17, onager wrote: > This seems a little wierd - why not "for amcache_key in volume_key.sub_keys"? Done. https://codereview.appspot.com/327070043/diff/1/tests/parsers/amcache.py File tests/parsers/amcache.py (right): https://codereview.appspot.com/327070043/diff/1/tests/parsers/amcache.py#newc... tests/parsers/amcache.py:4: On 2017/08/09 10:38:17, onager wrote: > Add "from __future__ import unicode_iterals", and then drop all the 'u' prefixes > from strings. Done. https://codereview.appspot.com/327070043/diff/1/tests/parsers/amcache.py#newc... tests/parsers/amcache.py:17: @shared_test_lib.skipUnlessHasTestFile([u'Amcache.hve']) On 2017/08/09 10:38:17, onager wrote: > Please also add a test for parsing a non-matching file - ideally a non-amcache > registry file. Added test for SYSTEM. https://codereview.appspot.com/327070043/diff/1/tests/parsers/amcache.py#newc... tests/parsers/amcache.py:33: expected_full_path = (u'c:\\users\\user\\appdata\\local\\microsoft\\' On 2017/08/09 10:38:17, onager wrote: > Line break after ( Done. https://codereview.appspot.com/327070043/diff/1/tests/parsers/amcache.py#newc... tests/parsers/amcache.py:37: expected_sha1 = u'0000818b581a471c1c6833839d35a9d6f3544f6a9c92' On 2017/08/09 10:38:17, onager wrote: > This doesn't look like a sha1 - 43 characters long, due to leading zeros. I > recommend stripping the extra zeros off here, or renaming the field. Stripped off 4 leading zero's in parser.
Sign in to reply to this message.
https://codereview.appspot.com/327070043/diff/20001/plaso/formatters/amcache.py File plaso/formatters/amcache.py (right): https://codereview.appspot.com/327070043/diff/20001/plaso/formatters/amcache.... plaso/formatters/amcache.py:33: """Formatter for an Amcache greePrograms Windows Registry event.""" Typo? https://codereview.appspot.com/327070043/diff/20001/plaso/parsers/amcache.py File plaso/parsers/amcache.py (right): https://codereview.appspot.com/327070043/diff/20001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:162: amcache_datetime = am_entry.get_value_by_name( Suggestion - refactor to events = _ProcessAMCacheFileKey(am_entry), or something similar. It'll reduce the nesting here, and it easier to test and follow. https://codereview.appspot.com/327070043/diff/20001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:170: if am_entry.get_value_by_name(self._AMCACHE_PRODUCTNAME) is not None: How about: product_value = am_entry.get_value_by_name(self._AMCACHE_PRODUCTNAME) if productname: event_data.productname = product_value.get_data_as_string() It's a little less verbose. https://codereview.appspot.com/327070043/diff/20001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:225: for am_entry in root_program_key.sub_keys: See above for suggestion about refactoring this.
Sign in to reply to this message.
https://codereview.appspot.com/327070043/diff/20001/plaso/formatters/amcache.py File plaso/formatters/amcache.py (right): https://codereview.appspot.com/327070043/diff/20001/plaso/formatters/amcache.... plaso/formatters/amcache.py:33: """Formatter for an Amcache greePrograms Windows Registry event.""" On 2017/08/22 13:13:41, onager wrote: > Typo? Done. https://codereview.appspot.com/327070043/diff/20001/plaso/parsers/amcache.py File plaso/parsers/amcache.py (right): https://codereview.appspot.com/327070043/diff/20001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:170: if am_entry.get_value_by_name(self._AMCACHE_PRODUCTNAME) is not None: On 2017/08/22 13:13:42, onager wrote: > How about: > > product_value = am_entry.get_value_by_name(self._AMCACHE_PRODUCTNAME) > if productname: > event_data.productname = product_value.get_data_as_string() > > It's a little less verbose. Seems to introduce an extra local var for each call, still feels wrong to me. How about I remote the "is not None" in each comparison. This will call the __nonzero()__ method on each return value. Makes the code a bit more terse and should give us the result we want (as __bool()__ is nonexistent on the return value so __len()__ will be called which gives us the result we want).
Sign in to reply to this message.
https://codereview.appspot.com/327070043/diff/20001/plaso/parsers/amcache.py File plaso/parsers/amcache.py (right): https://codereview.appspot.com/327070043/diff/20001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:170: if am_entry.get_value_by_name(self._AMCACHE_PRODUCTNAME) is not None: On 2017/08/23 12:25:47, rbdebeer wrote: > On 2017/08/22 13:13:42, onager wrote: > > How about: > > > > product_value = am_entry.get_value_by_name(self._AMCACHE_PRODUCTNAME) > > if productname: > > event_data.productname = product_value.get_data_as_string() > > > > It's a little less verbose. > > Seems to introduce an extra local var for each call, still feels wrong to me. > How about I remote the "is not None" in each comparison. This will call the > __nonzero()__ method on each return value. Makes the code a bit more terse and > should give us the result we want (as __bool()__ is nonexistent on the return > value so __len()__ will be called which gives us the result we want). Not sure why you're concerned about the local var - the current proposal has 2 calls to get_value_by_name which is much slower (and more visually noisy).
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/327070043/diff/20001/plaso/parsers/amcache.py File plaso/parsers/amcache.py (right): https://codereview.appspot.com/327070043/diff/20001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:162: amcache_datetime = am_entry.get_value_by_name( On 2017/08/22 13:13:41, onager wrote: > Suggestion - refactor to events = _ProcessAMCacheFileKey(am_entry), or something > similar. It'll reduce the nesting here, and it easier to test and follow. Done. https://codereview.appspot.com/327070043/diff/20001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:170: if am_entry.get_value_by_name(self._AMCACHE_PRODUCTNAME) is not None: On 2017/08/23 12:32:26, onager wrote: > On 2017/08/23 12:25:47, rbdebeer wrote: > > On 2017/08/22 13:13:42, onager wrote: > > > How about: > > > > > > product_value = am_entry.get_value_by_name(self._AMCACHE_PRODUCTNAME) > > > if productname: > > > event_data.productname = product_value.get_data_as_string() > > > > > > It's a little less verbose. > > > > Seems to introduce an extra local var for each call, still feels wrong to me. > > How about I remote the "is not None" in each comparison. This will call the > > __nonzero()__ method on each return value. Makes the code a bit more terse and > > should give us the result we want (as __bool()__ is nonexistent on the return > > value so __len()__ will be called which gives us the result we want). > > Not sure why you're concerned about the local var - the current proposal has 2 > calls to get_value_by_name which is much slower (and more visually noisy). Done.
Sign in to reply to this message.
https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py File plaso/parsers/amcache.py (right): https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:175: def _ProcessAMCacheProgramKey(self, am_entry, parser_mediator): +docstring https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:178: pevent_data = AmcacheProgramEventData() just event_data https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:182: version = am_entry.get_value_by_name(self._AMCACHE_P_VERSION) +white line before "version", to make this more readable, and also for the other attributes. https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:206: msiproductcode = ( Line break list so: msiproductcode = am_entry.get_value_by_name( self._AMCACHE_P_MSIPRODUCTCODE) https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:211: msiproductcode.get_data().decode('utf-16-LE') This fits on one line, but please do the get_data() and unicode decoding on separate lines so this is easier to debug. ie: product_code = msiproductcode.get_data() product_code = product_code.decode('utf-16-LE') event_data.msiproductcode = product_code https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:228: def _ProcessAMCacheFileKey(self, am_entry, parser_mediator): +docstring
Sign in to reply to this message.
Code updated.
Sign in to reply to this message.
https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py File plaso/parsers/amcache.py (right): https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:175: def _ProcessAMCacheProgramKey(self, am_entry, parser_mediator): On 2017/08/24 10:41:31, onager wrote: > +docstring Done. https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:178: pevent_data = AmcacheProgramEventData() On 2017/08/24 10:41:30, onager wrote: > just event_data Done. https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:182: version = am_entry.get_value_by_name(self._AMCACHE_P_VERSION) On 2017/08/24 10:41:30, onager wrote: > +white line before "version", to make this more readable, and also for the other > attributes. Done. https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:206: msiproductcode = ( On 2017/08/24 10:41:31, onager wrote: > Line break list so: > > msiproductcode = am_entry.get_value_by_name( > self._AMCACHE_P_MSIPRODUCTCODE) Done. https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:211: msiproductcode.get_data().decode('utf-16-LE') On 2017/08/24 10:41:31, onager wrote: > This fits on one line, but please do the get_data() and unicode decoding on > separate lines so this is easier to debug. ie: > product_code = msiproductcode.get_data() > product_code = product_code.decode('utf-16-LE') > event_data.msiproductcode = product_code Done. https://codereview.appspot.com/327070043/diff/40001/plaso/parsers/amcache.py#... plaso/parsers/amcache.py:228: def _ProcessAMCacheFileKey(self, am_entry, parser_mediator): On 2017/08/24 10:41:30, onager wrote: > +docstring Done.
Sign in to reply to this message.
LGTM, merging
Sign in to reply to this message.
Changes have been merged with master branch. To close the review and clean up the feature branch you can run: python ./utils/review.py close plugin-amcache
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
