FunctionID: Add AddSingleFunction.java Ghidra script #1
Conversation
WalkthroughA new Ghidra script, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AddSingleFunction Script
participant FidService
participant FID Database
User->>AddSingleFunction Script: Run script
AddSingleFunction Script->>AddSingleFunction Script: Validate program, address, function
AddSingleFunction Script->>FidService: Compute function hash
AddSingleFunction Script->>User: Prompt to select FID database
User->>AddSingleFunction Script: Select FID database
AddSingleFunction Script->>FID Database: Open database
AddSingleFunction Script->>FID Database: Fetch libraries
alt No libraries exist
AddSingleFunction Script->>User: Prompt for new library details
User->>AddSingleFunction Script: Provide details
AddSingleFunction Script->>FID Database: Create new library
else Libraries exist
AddSingleFunction Script->>User: Select existing library
User->>AddSingleFunction Script: Select library
end
AddSingleFunction Script->>User: Prompt for namespace stripping
User->>AddSingleFunction Script: Respond
AddSingleFunction Script->>FidServiceLibraryIngest: Check for terminator
AddSingleFunction Script->>FID Database: Add function record
AddSingleFunction Script->>FID Database: Save and close
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Ghidra/Features/FunctionID/ghidra_scripts/AddSingleFunction.java (1)
34-115: Consider adding more documentation comments to improve maintainability.While the script is well-structured, adding more documentation comments explaining the purpose and logic of different sections would improve maintainability and help future developers understand the code more easily.
public class AddSingleFunction extends GhidraScript { + /** + * FID database instance that will be used to store the function + */ private FidDB fidDb = null; @Override protected void run() throws Exception { + // Validate prerequisites: program, address and function if (currentProgram == null) { printerr("No current program"); return; } // ... existing code ... + // Calculate function hash FidService service = new FidService(); FidHashQuad hashFunction = service.hashFunction(function); // ... existing code ... + // Retrieve and select FID database FidFileManager fidFileManager = FidFileManager.getInstance(); // ... existing code ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Ghidra/Features/FunctionID/ghidra_scripts/AddSingleFunction.java(1 hunks)Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/service/FidServiceLibraryIngest.java(2 hunks)
🔇 Additional comments (11)
Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/service/FidServiceLibraryIngest.java (2)
40-40: LGTM: Class visibility changed to public to support external access.The change from package-private to public visibility allows the new
AddSingleFunctionscript to access this class.
526-527: LGTM: Method visibility changed to public to enable terminator detection.Making the
findTerminatormethod public and static enables the new script to check if a function contains a terminator instruction, which is necessary for the FID database entry creation process.Ghidra/Features/FunctionID/ghidra_scripts/AddSingleFunction.java (9)
17-19: LGTM: Clear and concise script description.The script description correctly states its purpose and is properly categorized under FunctionID.
41-54: LGTM: Proper validation of prerequisites.The script correctly validates that the current program, address, and function exist before proceeding, providing appropriate error messages when conditions aren't met.
56-61: LGTM: Function hash verification.The script properly verifies that the function can be hashed and terminates with an appropriate message if the function is too small.
63-70: LGTM: FID database availability check and selection.The script validates that FID databases are available and provides a user selection prompt.
71-94: LGTM: Library handling logic.The script correctly handles both cases: creating a new library when none exist or selecting an existing one from the FID database.
95-98: LGTM: Optional namespace stripping configuration.Providing the option to disable namespace stripping gives users flexibility in how function names are stored in the FID database.
101-101: LGTM: Usage of the now-public findTerminator method.The script correctly uses the newly exposed
FidServiceLibraryIngest.findTerminatormethod to determine if the function has a terminator.
105-107: LGTM: Function registration with appropriate attributes.The script creates a new function entry in the FID database with all required attributes: library, hash, name (with namespace handling option), offset, source file name, and terminator status.
109-113: LGTM: Proper resource management.The script properly saves changes to the database and ensures it's closed in a finally block, following good resource management practices.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Ghidra/Features/FunctionID/ghidra_scripts/AddSingleFunction.java (2)
63-70: Consider improving error message for no available FID databases.The error message "No available FID DB" is clear but doesn't provide guidance on how to resolve the issue.
Consider enhancing the error message to guide users on how to add a FID database:
- printerr("No available FID DB"); + printerr("No available FID databases. Please add a FID database to your project first.");
34-38: Add success message.The script currently doesn't inform the user when a function is successfully added to the database.
Consider adding a success message after the function is added:
fidDb.createNewFunction(library, hashFunction, function.getName(disableNamespaceStripping), offset, domainFile.getName(), hasTerminator); + + println("Successfully added function " + function.getName(disableNamespaceStripping) + + " to library " + library.getName()); fidDb.saveDatabase("Saving", monitor);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
Ghidra/Features/FunctionID/ghidra_scripts/AddSingleFunction.java(1 hunks)Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/service/FidServiceLibraryIngest.java(2 hunks)
🔇 Additional comments (9)
Ghidra/Features/FunctionID/src/main/java/ghidra/feature/fid/service/FidServiceLibraryIngest.java (2)
40-40: Changed class visibility to support new script.The
FidServiceLibraryIngestclass visibility has been changed from package-private to public, making it accessible for use in the newAddSingleFunction.javascript. This approach allows the script to leverage existing functionality while maintaining separation of concerns.
526-526: Changed method visibility to support functionality in new script.The
findTerminatormethod visibility has been changed from private to public static, allowing it to be used by the newAddSingleFunction.javascript to determine if a function contains a terminator instruction. This is necessary for the function creation process within the script.Ghidra/Features/FunctionID/ghidra_scripts/AddSingleFunction.java (7)
17-19: Good script documentation.The script is well-documented with a concise description and appropriate category annotation, making it easy for users to understand its purpose and find it in Ghidra's script manager.
41-54: Proper prerequisite validation.The script appropriately checks for the existence of a current program, address, and function before proceeding. This prevents potential null pointer exceptions and provides clear error messages to the user.
56-61: Appropriate early termination for unsuitable functions.The script correctly checks if the function is too small to be hashed and provides a clear error message in this case. This prevents adding problematic functions to the FID database that wouldn't be useful for matching.
76-89: Good user prompting for library creation.The script effectively guides users through creating a new library when none exists, collecting all necessary metadata and using current program information where appropriate.
95-98: Clear namespace configuration.The script provides a clear prompt for configuring namespace stripping, which is an important option when adding functions to a FID database as it affects function name matching.
101-101: Effective use of public API.The script makes good use of the newly public
FidServiceLibraryIngest.findTerminatormethod to determine if the function contains a terminator instruction, which is necessary for creating accurate function records.
109-113: Proper resource management.The script correctly uses a try-finally block to ensure the database is closed even if an exception occurs during processing. This prevents resource leaks and database corruption.
| try { | ||
| fidDb = fidFile.getFidDB(true); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing exception handling.
The code opens the FID database but doesn't handle specific exceptions that might occur during this operation, such as file not found, permission issues, or database corruption.
Consider adding specific exception handling:
try {
fidDb = fidFile.getFidDB(true);
+ } catch (IOException e) {
+ printerr("Failed to open FID database: " + e.getMessage());
+ return;
Committable suggestion skipped: line range outside the PR's diff.
|
/refacto-test |
1 similar comment
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
1 similar comment
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Security Review: FunctionID Script👍 Well Done
📝 Additional Comments
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| function.getName(disableNamespaceStripping), offset, domainFile.getName(), | ||
| hasTerminator); |
There was a problem hiding this comment.
Missing Input Validation
Function name from user input is used without validation. Malicious function names could potentially cause injection vulnerabilities when stored in the database.
| function.getName(disableNamespaceStripping), offset, domainFile.getName(), | |
| hasTerminator); | |
| String functionName = function.getName(disableNamespaceStripping); | |
| // Validate function name before database insertion | |
| if (functionName == null || functionName.isEmpty()) { | |
| printerr("Invalid function name"); | |
| return; | |
| } | |
| fidDb.createNewFunction(library, hashFunction, | |
| functionName, offset, domainFile.getName(), |
Standards
- CWE-20
- OWASP-A03
| try { | ||
| fidDb = fidFile.getFidDB(true); |
There was a problem hiding this comment.
Resource Leak Risk
Database resource opened without try-with-resources pattern. If an exception occurs between opening and the finally block, resource might not be properly closed.
| try { | |
| fidDb = fidFile.getFidDB(true); | |
| try (FidDB db = fidFile.getFidDB(true)) { | |
| fidDb = db; |
Standards
- CWE-772
- OWASP-A06
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: FunctionID Script👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| finally { | ||
| fidDb.close(); | ||
| } |
There was a problem hiding this comment.
Missing Null Check
fidDb.close() is called without checking if fidDb is null. If an exception occurs before fidDb is initialized (line 72), a NullPointerException will be thrown in the finally block, masking the original exception.
| finally { | |
| fidDb.close(); | |
| } | |
| finally { | |
| if (fidDb != null) { | |
| fidDb.close(); | |
| } | |
| } |
Standards
- Logic-Verification-Null-Safety
- Algorithm-Correctness-Exception-Handling
- Business-Rule-Resource-Management
| FidFile fidFile = | ||
| askChoice("FID database", "Choose FID database", userFid, userFid.get(0)); | ||
| try { | ||
| fidDb = fidFile.getFidDB(true); |
There was a problem hiding this comment.
Unchecked Database Operation
The getFidDB call may fail but lacks error handling. If database access fails, the script will throw an exception without proper user feedback about the specific database issue.
Standards
- Business-Rule-Validation
- Logic-Verification-Error-Handling
- Algorithm-Correctness-Exception-Management
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: FunctionID Script👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| String libraryFamilyName = | ||
| askString("Library Family Name", "Choose Library Family Name"); | ||
| String libraryVersion = askString("Library Version", "Choose Library Version"); | ||
| String libraryVariant = askString("Library Variant", "Choose Library Variant"); |
There was a problem hiding this comment.
Missing Input Validation
User input for library metadata is accepted without validation before being used to create database entries. This could allow injection of malicious data into the FID database, potentially enabling database corruption or injection attacks when these values are later used in queries.
| String libraryFamilyName = | |
| askString("Library Family Name", "Choose Library Family Name"); | |
| String libraryVersion = askString("Library Version", "Choose Library Version"); | |
| String libraryVariant = askString("Library Variant", "Choose Library Variant"); | |
| String libraryFamilyName = askString("Library Family Name", "Choose Library Family Name"); | |
| if (libraryFamilyName == null || libraryFamilyName.trim().isEmpty()) { | |
| printerr("Library Family Name cannot be empty"); | |
| return; | |
| } | |
| String libraryVersion = askString("Library Version", "Choose Library Version"); | |
| if (libraryVersion == null || libraryVersion.trim().isEmpty()) { | |
| printerr("Library Version cannot be empty"); | |
| return; | |
| } | |
| String libraryVariant = askString("Library Variant", "Choose Library Variant"); | |
| if (libraryVariant == null || libraryVariant.trim().isEmpty()) { | |
| printerr("Library Variant cannot be empty"); | |
| return; | |
| } |
Standards
- CWE-20
- OWASP-A03
- NIST-SSDF-PW.1
| fidDb.close(); | ||
| } |
There was a problem hiding this comment.
Missing Error Handling
The fidDb.close() call doesn't check if fidDb is null before closing, which could cause a NullPointerException if database initialization fails. This would prevent proper error handling and resource cleanup under failure conditions.
Standards
- ISO-IEC-25010-Performance-Efficiency-Resource-Utilization
- Optimization-Pattern-Resource-Management
- Error-Handling-Robustness
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: FunctionID Script👍 Well Done
📌 Files Processed
📝 Additional Comments
|
|
No review comments were generated during this review iteration of the PR. |
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: FunctionID Component👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| import ghidra.util.task.TaskMonitor; | ||
|
|
||
| class FidServiceLibraryIngest { | ||
| public class FidServiceLibraryIngest { |
There was a problem hiding this comment.
Visibility Modifier Change
Changing class visibility from package-private to public without apparent need exposes implementation details. This increases API surface area and reduces encapsulation, making future refactoring more difficult as external code may depend on this class.
Standards
- SOLID-OCP
- Maintainability-Quality-Encapsulation
- Clean-Code-Class-Organization
| * @throws CancelledException if the user cancels | ||
| */ | ||
| private static boolean findTerminator(Function function, TaskMonitor monitor) | ||
| public static boolean findTerminator(Function function, TaskMonitor monitor) |
There was a problem hiding this comment.
Method Visibility Change
Method visibility changed from private to public without clear architectural justification. This exposes implementation details and creates potential coupling points. Consider if a more targeted interface approach would better preserve encapsulation.
Standards
- SOLID-OCP
- Maintainability-Quality-Encapsulation
- Clean-Code-Method-Access
|
/refacto-test-arvi |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: FunctionID Script Enhancements👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| DomainFile domainFile = getCurrentProgram().getDomainFile(); | ||
|
|
||
| fidDb.createNewFunction(library, hashFunction, | ||
| function.getName(disableNamespaceStripping), offset, domainFile.getName(), |
There was a problem hiding this comment.
User Input Validation
Function names from user input are stored in the FID database without validation. Malicious function names could potentially be used for injection attacks when the database is later queried or data is displayed, potentially leading to XSS or SQL injection depending on how the database is implemented.
Standards
- CWE-20
- OWASP-A03
- NIST-SSDF-PW.1
| @Override | ||
| protected void run() throws Exception { |
There was a problem hiding this comment.
Excessive Method Responsibility
The run() method handles multiple responsibilities: validation, user interaction, database operations, and function creation. This violates Single Responsibility Principle and makes maintenance more difficult as changes to one aspect affect the entire method.
Standards
- SOLID-SRP
- Clean-Code-Functions
- Refactoring-Extract-Method
3 participants
Fixes NationalSecurityAgency#5212
Introduction
As described in the linked issue there is currently no way to add a single function to a FID DB which is something I have wanted for a long time as well. The Ghidra script
AddSingleFunction.javanow implements this functionality.Overview of the script
Prerequisites:
The script checks if there is any available library in the FID DB. If a library exists the user can select it. If not the user will be prompted to create one. The user can then decide whether to preserve the namespaces of the selected function or save only the basename. Finally the function entry is saved.
Testing
Empty FID DB (create new library)
Select a function (
rayon::main):Run
AddSingleFunction.java:List the FID DB content with
ListFunctions.java:Non-empty FID DB (already contains at least one library)
Select a function (
crossbeam_deque::deque::Stealer<T>::steal):Run
AddSingleFunction.java:List the FID DB content with
ListFunctions.java:Additional notes
I had to make
FidServiceLibraryIngestandfindTerminator()public because they are required to calculatehasTerminatorwhich is passed tocreateNewFunction(). If this is not allowed let me know and I will duplicatefindTerminator()instead.Summary by CodeRabbit
New Features
Improvements