Open
Conversation
Initial commit of code
Add usbparser program to 3rd party library
Added Extra parm for lump list lnk files
Add Running external program and adding artifacts and attributes.
Comment out older USB code from regripper run.
Move position of usb parser
Update bundle messages
Tsk 4.14.0 update
…mber-to-4.22.1 Uprade version number to 4.22.1
Cleanup
Update comments on code
Add linux usbparser executable so Linux can be supported as well
Add check for macos and linux OS;s
Update executable
Update calls for the executable programs.
File file name of executable, remove extra underscore
remove ) in string
Update comment and check for mac instead of macos
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUSB artifact creation was moved out of registry parsing and implemented as a new ExtractUsb ingest step that exports registry hives and other inputs, runs an external USB parser to produce an SQLite DB, and creates USB-related blackboard artifacts; the new step is registered at module startup. Changes
Sequence Diagram(s)sequenceDiagram
participant Ingest as Ingest Module
participant Extract as ExtractUsb
participant FS as Filesystem/DataSource
participant Parser as External USB Parser
participant DB as Parser SQLite DB
participant BB as Blackboard
Ingest->>Extract: start process(dataSource)
Extract->>FS: locate/export SOFTWARE, SYSTEM, NTUSER, EVTX, LNK
FS-->>Extract: copied files to temp dir
Extract->>Parser: execute external usb parser with temp inputs
Parser-->>Extract: produce sqlite DB + stdout/stderr
Extract->>DB: open sqlite and query usb_data / connect_disconnect
DB-->>Extract: rows of device & connection records
Extract->>BB: create/post USB_REMOVABLE_DEVICE and USB_CONN_DISCONN artifacts
BB-->>Extract: posting result
Extract-->>Ingest: finish
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PMD (7.23.0)RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractRegistry.java[ERROR] Cannot load ruleset rulesets/java/basic.xml/SimplifiedTernary: Cannot resolve rule/ruleset reference 'rulesets/java/basic.xml/SimplifiedTernary'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath. RecentActivity/src/org/sleuthkit/autopsy/recentactivity/RAImageIngestModule.java[ERROR] Cannot load ruleset rulesets/java/basic.xml/SimplifiedTernary: Cannot resolve rule/ruleset reference 'rulesets/java/basic.xml/SimplifiedTernary'. Make sure the resource is a valid file or URL and is on the CLASSPATH. Use --debug (or a fine log level) to see the current classpath. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java (4)
365-376: Same early return issue ingetLnkFiles()method.Similar to
getEvtxFiles(), a single file write failure aborts processing all remaining lnk files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java` around lines 365 - 376, The getLnkFiles() loop aborts on the first write failure because of the early return in the catch block; change the logic so that when ContentUtils.writeToFile(lnkFile, new File(lnkFileName)) throws IOException you call this.addErrorMessage(...) and logger.log(...) but do NOT return—continue processing remaining lnkFiles (optionally track a boolean or counter to indicate failures and handle/report once after the loop) so all lnk files are attempted; update references in the method (lnkFiles, lnkFileName, ContentUtils.writeToFile, addErrorMessage, logger) accordingly.
291-302: Early return on single file write failure aborts processing all remaining evtx files.If any single evtx file fails to write, the method returns immediately, skipping all subsequent files. Consider logging the error and continuing with other files.
♻️ Proposed fix: continue on individual file failures
for (AbstractFile evtxFile : evtxFiles) { evtxFileName = tempDirPath + File.separator + evtxFile.getName(); try { ContentUtils.writeToFile(evtxFile, new File(evtxFileName)); } catch (IOException ex) { - this.addErrorMessage(Bundle.ExtractUsb_process_errormsg_find_evtx()); + // Log warning but continue processing other files logger.log(Level.WARNING, String.format("Unable to write %s to temp directory. File name: %s", evtxFile.getName(), evtxFile), ex); //NON-NLS - return; + // Continue to next file } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java` around lines 291 - 302, The loop that writes evtxFiles uses ContentUtils.writeToFile inside a try/catch and currently calls return on IOException, which aborts processing remaining evtx files; change the catch in the loop (around ContentUtils.writeToFile for evtxFile / evtxFileName) to log the error (using this.addErrorMessage and logger.log as you already do) and then continue to the next file instead of returning—i.e., remove the early return and use continue (or otherwise skip the failing file) so the for (AbstractFile evtxFile : evtxFiles) loop processes all files.
162-168: Inconsistent indentation in the try block.The code block for the event log check has incorrect indentation (lines 163-168), which affects readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java` around lines 162 - 168, The code block checking for the event log (call to getEvtFile storing into evtPartitionFile and the subsequent null check that calls this.addErrorMessage(Bundle.EventPartitionLog_Not_Found()) and logger.log(...)) is misindented inside the try block; fix by aligning the if (evtPartitionFile == null) block and its statements with the surrounding try block statements so the null-check, error logging, and return are at the same indentation level as the getEvtFile call and other try-block statements (ensure the braces and indentation for the try block around getEvtFile, evtPartitionFile, addErrorMessage, logger.log, and return are consistent).
155-176: Event log file validation occurs afterextractUsbFiles()is called.The check for
evtPartitionFileon lines 162-168 happens afterextractUsbFiles()has already executed. If the event log is missing, the external tool may have already run unnecessarily. Consider validating all required inputs before invoking the external tool.♻️ Proposed restructure
try { String modOutFile = modOutPath + File.separator + "parseusb.db3"; String usbFileLocation = tempDirPath; - extractUsbFiles(usbDumper, modOutFile, usbFileLocation); - - AbstractFile evtPartitionFile = getEvtFile(dataSource, tempDirPath, "Microsoft-Windows-Partition%4Diagnostic.evtx", "/Windows/System32/winevt/logs/"); if (evtPartitionFile == null) { - this.addErrorMessage(Bundle.EventPartitionLog_Not_Found()); - logger.log(Level.SEVERE, "Error finding Event Log file"); //NON-NLS - return; //If we cannot find the event log we cannot proceed - - } + this.addErrorMessage(Bundle.EventPartitionLog_Not_Found()); + logger.log(Level.SEVERE, "Error finding Event Log file"); //NON-NLS + return; //If we cannot find the event log we cannot proceed + } + extractUsbFiles(usbDumper, modOutFile, usbFileLocation); + createUSBArtifacts(modOutFile, systemHiveFile); createConnectDisconnectArtifacts(modOutFile, evtPartitionFile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java` around lines 155 - 176, The code calls extractUsbFiles(...) before verifying the event log exists; move the getEvtFile(...) check (evtPartitionFile null handling and addErrorMessage/return) to occur before calling extractUsbFiles(...) so we validate required inputs first; specifically, locate the block using getEvtFile(dataSource, tempDirPath, "Microsoft-Windows-Partition%4Diagnostic.evtx", ...) and perform that null-check/early-return prior to invoking extractUsbFiles(usbDumper, modOutFile, usbFileLocation), then proceed to createUSBArtifacts(...) and createConnectDisconnectArtifacts(...) only after the event file is confirmed.RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractRegistry.java (1)
177-177: Remove unusedUSB_MAPPERfield andUSBInfoimport.The
USB_MAPPERstatic field (line 177) and theUsbDeviceIdMapper.USBInfoimport (line 50) are unused in the active code. The USB parsing logic that previously relied on these has been commented out (lines 770–806) and replaced with a new implementation. These can be safely removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractRegistry.java` at line 177, Remove the unused static field USB_MAPPER and the unused import UsbDeviceIdMapper.USBInfo from the ExtractRegistry class: delete the declaration "final private static UsbDeviceIdMapper USB_MAPPER = new UsbDeviceIdMapper();" and remove the corresponding USBInfo import since the USB parsing block using USB_MAPPER is commented out and the new implementation no longer references these symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/src/org/sleuthkit/autopsy/coreutils/PlatformUtil.java`:
- Around line 352-359: Update the Javadoc for the PlatformUtil.isMacOS() method
to correct the typo "Macos OS" to "macOS" (or "Mac OS" if you prefer project
style) so the comment accurately names the platform; leave the implementation
(the call to PlatformUtil.getOSName().toLowerCase().contains("mac")) unchanged
and keep the `@return` description consistent with the corrected name.
In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java`:
- Around line 133-139: The log message in ExtractUsb when checking
systemHiveFile is incorrect: it says "SOFTWARE Hive file" but this block handles
the SYSTEM hive; change the logger.log call in the null-check for systemHiveFile
(the code that calls getHiveFile and
this.addErrorMessage(Bundle.SystemHiveFile_Not_Found())) to log "Error finding
SYSTEM Hive file" (or an equivalent message referencing SYSTEM) so the message
matches the resource and error path.
- Around line 519-524: The log message inside the resultSet processing loop in
ExtractUsb (the block that checks context.dataSourceIngestIsCancelled() and
calls logger.log(Level.INFO, ...)) is incorrect—it's copy-pasted from SRU and
says "Cancelled SRU Net Usage Artifact Creation."; update that logger.log call
to a correct, specific message for this class such as "Cancelled USB
connect/disconnect artifact creation" (or similar) so it accurately reflects USB
Connect/Disconnect processing in ExtractUsb.
- Around line 411-435: getPathForUsbDumper currently can leave local variable
path null for unsupported OSes causing an NPE when calling path.toString();
update getPathForUsbDumper to check if path is null after the PlatformUtil
checks and return null (or otherwise handle the unsupported platform) before
calling InstalledFileLocator.getDefault().locate(...), ensuring the null-guard
prevents the NPE and keeps behavior consistent across getPathForUsbDumper,
InstalledFileLocator usage, and any callers expecting a null return for
unsupported platforms.
- Around line 588-598: The catch blocks in getUSBArtifact() and
getUSBConnectDisconnectArtifact() call String.format with no format specifiers
but pass USB_CONN_DISCONN_ARTIFACT_NAME as an extra argument; fix by either
removing String.format and concatenate/include the artifact name in the
exception message (e.g., "Failed to get USB artifact type: " +
USB_CONN_DISCONN_ARTIFACT_NAME) or add an appropriate format specifier (e.g.,
"Failed to get USB artifact type: %s") so the passed
USB_CONN_DISCONN_ARTIFACT_NAME is actually used when throwing the
TskCoreException from those methods.
- Around line 247-261: The parent-path check in ExtractUsb (inside the for loop
iterating hiveFiles in class ExtractUsb) only tests for "/Users/" and thus
misses legacy Windows profiles; update the condition that checks
hiveFile.getParentPath() to also accept older Windows profile paths (e.g.,
"/Documents and Settings/") — apply this change where
hiveFile.getParentPath().startsWith("/Users/") is used so the same branch
(computing username from the parent Path, building hiveFileName, and calling
ContentUtils.writeToFile) runs for both modern and legacy Windows profile
locations; ensure you preserve existing behavior for macOS and other code paths.
- Around line 172-175: The catch block in ExtractUsb is logging and reporting
the SRUDB message and Bundle key from ExtractSru; update the logger.log call and
the this.addErrorMessage call in the ExtractUsb catch to use a USB-specific
message and Bundle key (replace the "Error processing SRUDB.dat file" string and
Bundle.ExtractSru_process_error_executing_export_srudb_program() with the
appropriate USB message and Bundle.ExtractUsb_* resource), i.e. change the
logger.log(Level.SEVERE, ...) message to reference the USB parser/export and
call the ExtractUsb bundle method (e.g.,
ExtractUsb_process_error_executing_export_usb_program()) so the log and error
message correctly reflect the USB parser failure.
---
Nitpick comments:
In
`@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractRegistry.java`:
- Line 177: Remove the unused static field USB_MAPPER and the unused import
UsbDeviceIdMapper.USBInfo from the ExtractRegistry class: delete the declaration
"final private static UsbDeviceIdMapper USB_MAPPER = new UsbDeviceIdMapper();"
and remove the corresponding USBInfo import since the USB parsing block using
USB_MAPPER is commented out and the new implementation no longer references
these symbols.
In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java`:
- Around line 365-376: The getLnkFiles() loop aborts on the first write failure
because of the early return in the catch block; change the logic so that when
ContentUtils.writeToFile(lnkFile, new File(lnkFileName)) throws IOException you
call this.addErrorMessage(...) and logger.log(...) but do NOT return—continue
processing remaining lnkFiles (optionally track a boolean or counter to indicate
failures and handle/report once after the loop) so all lnk files are attempted;
update references in the method (lnkFiles, lnkFileName,
ContentUtils.writeToFile, addErrorMessage, logger) accordingly.
- Around line 291-302: The loop that writes evtxFiles uses
ContentUtils.writeToFile inside a try/catch and currently calls return on
IOException, which aborts processing remaining evtx files; change the catch in
the loop (around ContentUtils.writeToFile for evtxFile / evtxFileName) to log
the error (using this.addErrorMessage and logger.log as you already do) and then
continue to the next file instead of returning—i.e., remove the early return and
use continue (or otherwise skip the failing file) so the for (AbstractFile
evtxFile : evtxFiles) loop processes all files.
- Around line 162-168: The code block checking for the event log (call to
getEvtFile storing into evtPartitionFile and the subsequent null check that
calls this.addErrorMessage(Bundle.EventPartitionLog_Not_Found()) and
logger.log(...)) is misindented inside the try block; fix by aligning the if
(evtPartitionFile == null) block and its statements with the surrounding try
block statements so the null-check, error logging, and return are at the same
indentation level as the getEvtFile call and other try-block statements (ensure
the braces and indentation for the try block around getEvtFile,
evtPartitionFile, addErrorMessage, logger.log, and return are consistent).
- Around line 155-176: The code calls extractUsbFiles(...) before verifying the
event log exists; move the getEvtFile(...) check (evtPartitionFile null handling
and addErrorMessage/return) to occur before calling extractUsbFiles(...) so we
validate required inputs first; specifically, locate the block using
getEvtFile(dataSource, tempDirPath,
"Microsoft-Windows-Partition%4Diagnostic.evtx", ...) and perform that
null-check/early-return prior to invoking extractUsbFiles(usbDumper, modOutFile,
usbFileLocation), then proceed to createUSBArtifacts(...) and
createConnectDisconnectArtifacts(...) only after the event file is confirmed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6c452fb-0e82-4a78-aafc-c7e9c0131d6d
⛔ Files ignored due to path filters (1)
thirdparty/markmckinnon/mm_artifact_parser_x64_win.exeis excluded by!**/*.exe
📒 Files selected for processing (9)
Core/src/org/sleuthkit/autopsy/coreutils/PlatformUtil.javaRecentActivity/src/org/sleuthkit/autopsy/recentactivity/Bundle.properties-MERGEDRecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractRegistry.javaRecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.javaRecentActivity/src/org/sleuthkit/autopsy/recentactivity/RAImageIngestModule.javathirdparty/markmckinnon/mm_artifact_parser_aarch64_linuxthirdparty/markmckinnon/mm_artifact_parser_aarch64_macosthirdparty/markmckinnon/mm_artifact_parser_x64_linuxthirdparty/markmckinnon/mm_artifact_parser_x64_macos
Comment on lines
+352
to
+359
| /** | ||
| * Check if running on Macos OS | ||
| * | ||
| * @return true if running on Macos OS | ||
| */ | ||
| public static boolean isMacOS() { | ||
| return PlatformUtil.getOSName().toLowerCase().contains("mac"); //NON-NLS | ||
| } |
There was a problem hiding this comment.
Minor typo in Javadoc: "Macos OS" should be "Mac OS" or "macOS".
The method implementation is correct and follows the existing isWindowsOS() pattern.
📝 Proposed fix
/**
- * Check if running on Macos OS
+ * Check if running on Mac OS
*
- * `@return` true if running on Macos OS
+ * `@return` true if running on Mac OS
*/
public static boolean isMacOS() {
return PlatformUtil.getOSName().toLowerCase().contains("mac"); //NON-NLS
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Check if running on Macos OS | |
| * | |
| * @return true if running on Macos OS | |
| */ | |
| public static boolean isMacOS() { | |
| return PlatformUtil.getOSName().toLowerCase().contains("mac"); //NON-NLS | |
| } | |
| /** | |
| * Check if running on Mac OS | |
| * | |
| * `@return` true if running on Mac OS | |
| */ | |
| public static boolean isMacOS() { | |
| return PlatformUtil.getOSName().toLowerCase().contains("mac"); //NON-NLS | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/src/org/sleuthkit/autopsy/coreutils/PlatformUtil.java` around lines 352
- 359, Update the Javadoc for the PlatformUtil.isMacOS() method to correct the
typo "Macos OS" to "macOS" (or "Mac OS" if you prefer project style) so the
comment accurately names the platform; leave the implementation (the call to
PlatformUtil.getOSName().toLowerCase().contains("mac")) unchanged and keep the
`@return` description consistent with the corrected name.
Comment on lines
+133
to
+139
| AbstractFile systemHiveFile = getHiveFile(dataSource, tempDirPath, "system", "/config/"); | ||
| if (systemHiveFile == null) { | ||
| this.addErrorMessage(Bundle.SystemHiveFile_Not_Found()); | ||
| logger.log(Level.SEVERE, "Error finding SOFTWARE Hive file"); //NON-NLS | ||
| return; //If we cannot find the SYSTEM hive we cannot proceed | ||
|
|
||
| } |
There was a problem hiding this comment.
Incorrect log message: references "SOFTWARE" but this is for SYSTEM hive.
The log message on line 136 incorrectly states "SOFTWARE Hive file" when it should say "SYSTEM Hive file".
📝 Proposed fix
AbstractFile systemHiveFile = getHiveFile(dataSource, tempDirPath, "system", "/config/");
if (systemHiveFile == null) {
this.addErrorMessage(Bundle.SystemHiveFile_Not_Found());
- logger.log(Level.SEVERE, "Error finding SOFTWARE Hive file"); //NON-NLS
+ logger.log(Level.SEVERE, "Error finding SYSTEM Hive file"); //NON-NLS
return; //If we cannot find the SYSTEM hive we cannot proceed
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| AbstractFile systemHiveFile = getHiveFile(dataSource, tempDirPath, "system", "/config/"); | |
| if (systemHiveFile == null) { | |
| this.addErrorMessage(Bundle.SystemHiveFile_Not_Found()); | |
| logger.log(Level.SEVERE, "Error finding SOFTWARE Hive file"); //NON-NLS | |
| return; //If we cannot find the SYSTEM hive we cannot proceed | |
| } | |
| AbstractFile systemHiveFile = getHiveFile(dataSource, tempDirPath, "system", "/config/"); | |
| if (systemHiveFile == null) { | |
| this.addErrorMessage(Bundle.SystemHiveFile_Not_Found()); | |
| logger.log(Level.SEVERE, "Error finding SYSTEM Hive file"); //NON-NLS | |
| return; //If we cannot find the SYSTEM hive we cannot proceed | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java`
around lines 133 - 139, The log message in ExtractUsb when checking
systemHiveFile is incorrect: it says "SOFTWARE Hive file" but this block handles
the SYSTEM hive; change the logger.log call in the null-check for systemHiveFile
(the code that calls getHiveFile and
this.addErrorMessage(Bundle.SystemHiveFile_Not_Found())) to log "Error finding
SYSTEM Hive file" (or an equivalent message referencing SYSTEM) so the message
matches the resource and error path.
Comment on lines
+172
to
+175
| } catch (IOException ex) { | ||
| logger.log(Level.SEVERE, "Error processing SRUDB.dat file", ex); //NON-NLS= | ||
| this.addErrorMessage(Bundle.ExtractSru_process_error_executing_export_srudb_program()); | ||
| } |
There was a problem hiding this comment.
Wrong error message: references SRUDB.dat but should reference USB parser.
The exception handler uses the wrong log message and Bundle reference from the ExtractSru module instead of ExtractUsb.
🐛 Proposed fix
} catch (IOException ex) {
- logger.log(Level.SEVERE, "Error processing SRUDB.dat file", ex); //NON-NLS=
- this.addErrorMessage(Bundle.ExtractSru_process_error_executing_export_srudb_program());
+ logger.log(Level.SEVERE, "Error processing USB files", ex); //NON-NLS
+ this.addErrorMessage(Bundle.ExtractUsb_process_error_executing_export_srudb_program());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java`
around lines 172 - 175, The catch block in ExtractUsb is logging and reporting
the SRUDB message and Bundle key from ExtractSru; update the logger.log call and
the this.addErrorMessage call in the ExtractUsb catch to use a USB-specific
message and Bundle key (replace the "Error processing SRUDB.dat file" string and
Bundle.ExtractSru_process_error_executing_export_srudb_program() with the
appropriate USB message and Bundle.ExtractUsb_* resource), i.e. change the
logger.log(Level.SEVERE, ...) message to reference the USB parser/export and
call the ExtractUsb bundle method (e.g.,
ExtractUsb_process_error_executing_export_usb_program()) so the log and error
message correctly reflect the USB parser failure.
Comment on lines
+247
to
+261
| for (AbstractFile hiveFile : hiveFiles) { | ||
|
|
||
| if (hiveFile.getParentPath().startsWith("/Users/")) { | ||
| Path path = Paths.get(hiveFile.getParentPath()); | ||
| String username = path.getFileName().toString(); | ||
| hiveFileName = tempDirPath + File.separator + username + "_" + hiveFile.getName(); | ||
| try { | ||
| ContentUtils.writeToFile(hiveFile, new File(hiveFileName)); | ||
| } catch (IOException ex) { | ||
| this.addErrorMessage(Bundle.ExtractUsb_process_errormsg_find_hive()); | ||
| logger.log(Level.WARNING, String.format("Unable to write %s to temp directory. File name: %s", hiveFile.getName(), hiveFile), ex); //NON-NLS | ||
| return; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Path check uses macOS convention /Users/ but this extracts Windows NTuser.dat files.
Windows NTuser.dat files are typically located under /Users/ on Windows Vista+ or /Documents and Settings/ on older systems. The check on line 249 only matches /Users/, which will miss NTuser.dat files on Windows XP/2003 images.
💡 Consider also checking for legacy Windows paths
for (AbstractFile hiveFile : hiveFiles) {
- if (hiveFile.getParentPath().startsWith("/Users/")) {
+ if (hiveFile.getParentPath().startsWith("/Users/") ||
+ hiveFile.getParentPath().startsWith("/Documents and Settings/")) {
Path path = Paths.get(hiveFile.getParentPath());
String username = path.getFileName().toString();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java`
around lines 247 - 261, The parent-path check in ExtractUsb (inside the for loop
iterating hiveFiles in class ExtractUsb) only tests for "/Users/" and thus
misses legacy Windows profiles; update the condition that checks
hiveFile.getParentPath() to also accept older Windows profile paths (e.g.,
"/Documents and Settings/") — apply this change where
hiveFile.getParentPath().startsWith("/Users/") is used so the same branch
(computing username from the parent Path, building hiveFileName, and calling
ContentUtils.writeToFile) runs for both modern and legacy Windows profile
locations; ensure you preserve existing behavior for macOS and other code paths.
Comment on lines
+411
to
+435
| private String getPathForUsbDumper() { | ||
| Path path = null; | ||
| if (PlatformUtil.isWindowsOS()) { | ||
| path = Paths.get(USB_TOOL_FOLDER, USB_TOOL_NAME_X64_WINDOWS); | ||
| } else if (PlatformUtil.isLinuxOS()) { | ||
| if ("aarch64".equals(PlatformUtil.getOSArch())) { | ||
| path = Paths.get(USB_TOOL_FOLDER, USB_TOOL_NAME_AARCH64_LINUX); | ||
| } else { | ||
| path = Paths.get(USB_TOOL_FOLDER, USB_TOOL_NAME_X64_LINUX); | ||
| } | ||
| } else if (PlatformUtil.isMacOS()) { | ||
| if ("aarch64".equals(PlatformUtil.getOSArch())) { | ||
| path = Paths.get(USB_TOOL_FOLDER, USB_TOOL_NAME_AARCH64_MACOS); | ||
| } else { | ||
| path = Paths.get(USB_TOOL_FOLDER, USB_TOOL_NAME_X64_MACOS); | ||
| } | ||
| } | ||
| File usbToolFile = InstalledFileLocator.getDefault().locate(path.toString(), | ||
| ExtractUsb.class.getPackage().getName(), false); | ||
| if (usbToolFile != null) { | ||
| return usbToolFile.getAbsolutePath(); | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Potential NullPointerException if platform is unsupported.
If PlatformUtil returns an unrecognized OS (not Windows, Linux, or macOS), path remains null, causing an NPE on line 428 when calling path.toString().
🐛 Proposed fix
private String getPathForUsbDumper() {
Path path = null;
if (PlatformUtil.isWindowsOS()) {
path = Paths.get(USB_TOOL_FOLDER, USB_TOOL_NAME_X64_WINDOWS);
} else if (PlatformUtil.isLinuxOS()) {
if ("aarch64".equals(PlatformUtil.getOSArch())) {
path = Paths.get(USB_TOOL_FOLDER, USB_TOOL_NAME_AARCH64_LINUX);
} else {
path = Paths.get(USB_TOOL_FOLDER, USB_TOOL_NAME_X64_LINUX);
}
} else if (PlatformUtil.isMacOS()) {
if ("aarch64".equals(PlatformUtil.getOSArch())) {
path = Paths.get(USB_TOOL_FOLDER, USB_TOOL_NAME_AARCH64_MACOS);
} else {
path = Paths.get(USB_TOOL_FOLDER, USB_TOOL_NAME_X64_MACOS);
}
}
+
+ if (path == null) {
+ return null;
+ }
+
File usbToolFile = InstalledFileLocator.getDefault().locate(path.toString(),
ExtractUsb.class.getPackage().getName(), false);
if (usbToolFile != null) {
return usbToolFile.getAbsolutePath();
}
return null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java`
around lines 411 - 435, getPathForUsbDumper currently can leave local variable
path null for unsupported OSes causing an NPE when calling path.toString();
update getPathForUsbDumper to check if path is null after the PlatformUtil
checks and return null (or otherwise handle the unsupported platform) before
calling InstalledFileLocator.getDefault().locate(...), ensuring the null-guard
prevents the NPE and keeps behavior consistent across getPathForUsbDumper,
InstalledFileLocator usage, and any callers expecting a null return for
unsupported platforms.
Comment on lines
+519
to
+524
| while (resultSet.next()) { | ||
|
|
||
| if (context.dataSourceIngestIsCancelled()) { | ||
| logger.log(Level.INFO, "Cancelled SRU Net Usage Artifact Creation."); //NON-NLS | ||
| return; | ||
| } |
There was a problem hiding this comment.
Incorrect log message: references "SRU Net Usage" but this is for USB Connect/Disconnect.
This appears to be copy-paste from the SRU module.
📝 Proposed fix
if (context.dataSourceIngestIsCancelled()) {
- logger.log(Level.INFO, "Cancelled SRU Net Usage Artifact Creation."); //NON-NLS
+ logger.log(Level.INFO, "Cancelled USB Connect/Disconnect Artifact Creation."); //NON-NLS
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| while (resultSet.next()) { | |
| if (context.dataSourceIngestIsCancelled()) { | |
| logger.log(Level.INFO, "Cancelled SRU Net Usage Artifact Creation."); //NON-NLS | |
| return; | |
| } | |
| while (resultSet.next()) { | |
| if (context.dataSourceIngestIsCancelled()) { | |
| logger.log(Level.INFO, "Cancelled USB Connect/Disconnect Artifact Creation."); //NON-NLS | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java`
around lines 519 - 524, The log message inside the resultSet processing loop in
ExtractUsb (the block that checks context.dataSourceIngestIsCancelled() and
calls logger.log(Level.INFO, ...)) is incorrect—it's copy-pasted from SRU and
says "Cancelled SRU Net Usage Artifact Creation."; update that logger.log call
to a correct, specific message for this class such as "Cancelled USB
connect/disconnect artifact creation" (or similar) so it accurately reflects USB
Connect/Disconnect processing in ExtractUsb.
Comment on lines
+588
to
+598
| private BlackboardArtifact.Type getUSBArtifact() throws TskCoreException { | ||
| if (usbArtifactType == null) { | ||
| try { | ||
| usbArtifactType = tskCase.getBlackboard().getOrAddArtifactType(USB_ARTIFACT_NAME, Bundle.Usb_Artifact_Name()); | ||
| } catch (Blackboard.BlackboardException ex) { | ||
| throw new TskCoreException(String.format("Failed to get USB artifact type", USB_CONN_DISCONN_ARTIFACT_NAME), ex); | ||
| } | ||
| } | ||
|
|
||
| return usbArtifactType; | ||
| } |
There was a problem hiding this comment.
Incorrect String.format: missing format specifier.
Line 593 uses String.format with a message containing no format specifiers, but passes USB_CONN_DISCONN_ARTIFACT_NAME as an argument. The same issue exists in getUSBConnectDisconnectArtifact() on line 613.
📝 Proposed fix for getUSBArtifact()
} catch (Blackboard.BlackboardException ex) {
- throw new TskCoreException(String.format("Failed to get USB artifact type", USB_CONN_DISCONN_ARTIFACT_NAME), ex);
+ throw new TskCoreException(String.format("Failed to get USB artifact type %s", USB_ARTIFACT_NAME), ex);
}📝 Proposed fix for getUSBConnectDisconnectArtifact()
} catch (Blackboard.BlackboardException ex) {
- throw new TskCoreException(String.format("Failed to get connect/disconnect artifact type", USB_ARTIFACT_NAME), ex);
+ throw new TskCoreException(String.format("Failed to get connect/disconnect artifact type %s", USB_CONN_DISCONN_ARTIFACT_NAME), ex);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private BlackboardArtifact.Type getUSBArtifact() throws TskCoreException { | |
| if (usbArtifactType == null) { | |
| try { | |
| usbArtifactType = tskCase.getBlackboard().getOrAddArtifactType(USB_ARTIFACT_NAME, Bundle.Usb_Artifact_Name()); | |
| } catch (Blackboard.BlackboardException ex) { | |
| throw new TskCoreException(String.format("Failed to get USB artifact type", USB_CONN_DISCONN_ARTIFACT_NAME), ex); | |
| } | |
| } | |
| return usbArtifactType; | |
| } | |
| private BlackboardArtifact.Type getUSBArtifact() throws TskCoreException { | |
| if (usbArtifactType == null) { | |
| try { | |
| usbArtifactType = tskCase.getBlackboard().getOrAddArtifactType(USB_ARTIFACT_NAME, Bundle.Usb_Artifact_Name()); | |
| } catch (Blackboard.BlackboardException ex) { | |
| throw new TskCoreException(String.format("Failed to get USB artifact type %s", USB_ARTIFACT_NAME), ex); | |
| } | |
| } | |
| return usbArtifactType; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java`
around lines 588 - 598, The catch blocks in getUSBArtifact() and
getUSBConnectDisconnectArtifact() call String.format with no format specifiers
but pass USB_CONN_DISCONN_ARTIFACT_NAME as an extra argument; fix by either
removing String.format and concatenate/include the artifact name in the
exception message (e.g., "Failed to get USB artifact type: " +
USB_CONN_DISCONN_ARTIFACT_NAME) or add an appropriate format specifier (e.g.,
"Failed to get USB artifact type: %s") so the passed
USB_CONN_DISCONN_ARTIFACT_NAME is actually used when throwing the
TskCoreException from those methods.
Change error message from severe to warning.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java`:
- Around line 119-125: The parser DB filename "parseusb.db3" is case-wide and
can be clobbered across concurrent ingest runs; update the ExtractUsb code that
writes parseusb.db3 so the DB file is created inside the job-scoped temp
directory used for extracted inputs (or else made unique by appending the ingest
job id/UUID), e.g., use the existing job temp dir variable or create a temp file
with Files.createTempFile(...) and store/open that path instead of the fixed
"parseusb.db3"; ensure the same job-scoped/unique path is used where the DB is
opened and later deleted (affects the code around the parseusb.db3 writes/opens
in ExtractUsb).
- Around line 206-219: The extractor currently flattens multiple matches into
the same temp filename/dir and overwrites the event-log handle, causing
"last-match-wins"; update the ExtractUsb copy logic so each matched source gets
a unique path (e.g., per-source subdirectory or unique filename suffix using a
stable identifier such as device+volume+originalPath hash, UUID or timestamp)
instead of writing everything into one flat temp directory, and change any
single event-log handle variable to a collection (list/map) so all matching
event handles are preserved and processed; apply these changes to the
copy-and-store locations in the ExtractUsb class where extracted inputs are
flattened and where the event-log handle is assigned so earlier matches are not
overwritten.
- Around line 161-170: The current code in ExtractUsb.java returns early when
Microsoft-Windows-Partition%4Diagnostic.evtx is missing, preventing
registry-backed usb_data from being processed; instead, remove the unconditional
return and change the logic so the EVTX absence only skips the
partition/EVTX-specific artifact path (the connect/disconnect creation) while
still calling createUSBArtifacts(...) with the registry usb_data. Locate the
EVTX existence check around where usb_data is populated and modify it to set a
flag (e.g., evtxPresent) or conditionally skip only the EVTX-specific
method/branch, but always proceed to call createUSBArtifacts(...) so
registry-derived USB artifacts are created. Ensure references to
createUSBArtifacts(...) and the EVTX filename
Microsoft-Windows-Partition%4Diagnostic.evtx are updated accordingly.
- Around line 454-456: ExtractUsb currently calls ResultSet.getLong() on
nullable columns (first_connect_time, last_connect_time, last_disconnected_time,
TimeCreated_SystemTime), which yields epoch 0 for SQL NULLs; add a private
helper like readNullableLong(ResultSet, String) that calls getLong and returns
null if resultSet.wasNull(), then replace direct getLong(...) usages in
ExtractUsb with readNullableLong(...) and only create/add the DATETIME
attributes (e.g., when building Artifact/BlackboardAttribute entries) if the
returned Long is non-null so you don't emit spurious 1970-01-01 timestamps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3f9a155-1c37-4973-b3d3-3daa173a059c
📒 Files selected for processing (1)
RecentActivity/src/org/sleuthkit/autopsy/recentactivity/ExtractUsb.java
made changes based on coderabbit suggestions
Member
|
@markmckinnon This one too. Sorry. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This replaces the current usb parser artifact with more detail. It uses the registry and event logs to gather USB device information.
Summary by CodeRabbit
New Features
Bug Fixes
Localization