Skip to content

FunctionID: Add AddSingleFunction.java Ghidra script #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+117 −2
Open

FunctionID: Add AddSingleFunction.java Ghidra script #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 115 additions & 0 deletions Ghidra/Features/FunctionID/ghidra_scripts/AddSingleFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@

/* ###
* IP: GHIDRA
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
//Adds the function at the current address to the chosen FID library.
//@category FunctionID

import java.util.List;

import ghidra.app.script.GhidraScript;
import ghidra.feature.fid.db.*;
import ghidra.feature.fid.hash.FidHashQuad;
import ghidra.feature.fid.service.FidService;
import ghidra.feature.fid.service.FidServiceLibraryIngest;
import ghidra.framework.model.DomainFile;
import ghidra.program.model.lang.CompilerSpec;
import ghidra.program.model.lang.Language;
import ghidra.program.model.lang.LanguageID;
import ghidra.program.model.listing.Function;
import ghidra.program.model.listing.FunctionManager;

public class AddSingleFunction extends GhidraScript {

private FidDB fidDb = null;

@Override
protected void run() throws Exception {
Comment on lines +38 to +39
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (currentProgram == null) {
printerr("No current program");
return;
}
if (currentAddress == null) {
printerr("No current address (?)");
return;
}
FunctionManager functionManager = currentProgram.getFunctionManager();
Function function = functionManager.getFunctionContaining(currentAddress);
if (function == null) {
printerr("No current function");
return;
}

FidService service = new FidService();
FidHashQuad hashFunction = service.hashFunction(function);
if (hashFunction == null) {
printerr("Function too small");
return;
}

FidFileManager fidFileManager = FidFileManager.getInstance();
List<FidFile> userFid = fidFileManager.getUserAddedFiles();
if (userFid.isEmpty()) {
printerr("No available FID DB");
return;
}
FidFile fidFile =
askChoice("FID database", "Choose FID database", userFid, userFid.get(0));
try {
fidDb = fidFile.getFidDB(true);
Comment on lines +71 to +72
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
try {
fidDb = fidFile.getFidDB(true);
try (FidDB db = fidFile.getFidDB(true)) {
fidDb = db;
Standards
  • CWE-772
  • OWASP-A06

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Comment on lines +71 to +73
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

List<LibraryRecord> libraries = fidDb.getAllLibraries();
LibraryRecord library;
if (libraries == null || libraries.isEmpty()) {
println("No libraries found. Creating one...");

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");
Comment on lines +79 to +82
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

LanguageID languageId = currentProgram.getLanguageID();
Language language = currentProgram.getLanguage();
CompilerSpec compilerSpec = currentProgram.getCompilerSpec();
library = fidDb.createNewLibrary(libraryFamilyName, libraryVersion, libraryVariant,
getGhidraVersion(), languageId, language.getVersion(),
language.getMinorVersion(), compilerSpec.getCompilerSpecID());
}
else {
library =
askChoice("FID libraries", "Choose FID library", libraries, libraries.get(0));
}

boolean disableNamespaceStripping =
askYesNo("Namespace stripping",
"Do you want to disable namespace stripping?");

long offset = function.getEntryPoint().getOffset();

boolean hasTerminator = FidServiceLibraryIngest.findTerminator(function, monitor);

DomainFile domainFile = getCurrentProgram().getDomainFile();

fidDb.createNewFunction(library, hashFunction,
function.getName(disableNamespaceStripping), offset, domainFile.getName(),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

hasTerminator);
Comment on lines +106 to +107
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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


fidDb.saveDatabase("Saving", monitor);
}
finally {
fidDb.close();
}
Comment on lines +111 to +113
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
finally {
fidDb.close();
}
finally {
if (fidDb != null) {
fidDb.close();
}
}
Standards
  • Logic-Verification-Null-Safety
  • Algorithm-Correctness-Exception-Handling
  • Business-Rule-Resource-Management

Comment on lines +112 to +113
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
4 changes: 2 additions & 2 deletions ...Features/FunctionID/src/main/java/ghidra/feature/fid/service/FidServiceLibraryIngest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
import ghidra.util.exception.VersionException;
import ghidra.util.task.TaskMonitor;

class FidServiceLibraryIngest {
public class FidServiceLibraryIngest {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private static final int MAXIMUM_NUMBER_OF_NAME_RESOLUTION_RELATIONS = 12;

private FidDB fidDb; // The database being populated
Expand Down Expand Up @@ -523,7 +523,7 @@ private void resolveNamedRelations() throws CancelledException {
* @return if a terminating flow was found in the function body
* @throws CancelledException if the user cancels
*/
private static boolean findTerminator(Function function, TaskMonitor monitor)
public static boolean findTerminator(Function function, TaskMonitor monitor)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throws CancelledException {
boolean retFound = false;
AddressSetView body = function.getBody();
Expand Down