Fix security vulnerabilities across codebase by sareluzi · Pull Request #355 · realsenseai/realsense_mipi_platform_driver

sareluzi · 2026-02-10T09:56:59Z

Summary

Add bounds checking for buffer access in ds5_fw_logger.cpp (HIGH severity)
Fix buffer overflow in hwmc.cpp - increased buffer size + validation (HIGH severity)
Replace VLA with heap allocation in StreamView.cpp to prevent stack overflow (HIGH severity)
Quote shell variables and add validation in build_all.sh (HIGH severity)
Add malloc null-check in StreamView.cpp (MEDIUM severity)
Replace unsafe strcpy with strncpy in framesextract.c (MEDIUM severity)
Add input validation for device paths in test_fw_version.py (MEDIUM severity)
Add security warning for NOPASSWD sudoers in install.tegra.artifacts.sh (LOW severity)
Use GitHub actor variable instead of hardcoded git identity in workflows (LOW severity)

Test plan

Compile utilities/streamApp on target platform to verify C++ changes
Run ./build_all.sh --clean 6.2 to verify shell script changes
Run pytest tests to verify Python validation changes
Verify GitHub Actions workflows pass on this PR

🤖 Generated with Claude Code

- Add bounds checking for buffer access in ds5_fw_logger.cpp - Fix buffer overflow in hwmc.cpp (increased buffer size + validation) - Replace VLA with heap allocation in StreamView.cpp to prevent stack overflow - Add malloc null-check in StreamView.cpp - Quote shell variables and add validation in build_all.sh - Replace unsafe strcpy with strncpy in framesextract.c - Add input validation for device paths in test_fw_version.py - Add security warning for NOPASSWD sudoers in install.tegra.artifacts.sh - Use GitHub actor variable instead of hardcoded git identity in workflows Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Copilot

Pull request overview

Addresses multiple security issues across C/C++ utilities, shell scripts, tests, and CI workflows by adding bounds checks, safer allocations/copies, input validation, and safer scripting practices.

Changes:

Hardened C/C++ codepaths against buffer/stack overflows and OOB reads (bounds checks, safer copies, heap allocations).
Improved shell/script safety (quoting, basic validation) and added security warnings in install docs.
Reduced risk in CI by using GitHub actor identity rather than hardcoded git config.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
utilities/streamApp/hwmc.cpp	Adds param bounds checks and output buffer length clamping.
utilities/streamApp/gui/StreamView.cpp	Replaces VLA with heap allocation; adds malloc null-check.
utilities/streamApp/ds5_fw_logger.cpp	Adds bounds checking to prevent out-of-bounds log access.
test/test_metadata/framesextract.c	Replaces unsafe `strcpy` with bounded `strncpy` + terminator.
test/test_fw_version.py	Adds device/path validation and safer subprocess argument usage.
test/install.tegra.artifacts.sh	Adds explicit security warning about NOPASSWD sudoers entry.
build_all.sh	Quotes variables and adds a guard before destructive `rm -rf`.
.github/workflows/build-jp6.yml	Uses `${{ github.actor }}` for git identity in CI.
.github/workflows/build-jp6.2.yml	Uses `${{ github.actor }}` for git identity in CI.
.github/workflows/build-jp6.1.yml	Uses `${{ github.actor }}` for git identity in CI.
.github/workflows/build-jp512.yml	Uses `${{ github.actor }}` for git identity in CI.
.github/workflows/build-jp502.yml	Uses `${{ github.actor }}` for git identity in CI.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-10T09:59:08Z

+		if (inParams.empty()) {
+			cerr << "Error: HWMC requires at least an opcode" << endl;
+			return;
+		}
 		opcode = inParams[0];
 		memset(params, 0, sizeof(params));


The early return on empty inParams skips memset(params, ...), leaving params uninitialized in the constructed object. Initialize params unconditionally (e.g., memset before the empty-check or default-initialize the array) and consider making the “no opcode” case a construction failure (exception/factory/valid-flag) so callers don’t proceed with a partially-initialized HWMC.

Suggested change

if (inParams.empty()) {

cerr << "Error: HWMC requires at least an opcode" << endl;

return;

}

opcode = inParams[0];

memset(params, 0, sizeof(params));

memset(params, 0, sizeof(params));

if (inParams.empty()) {

cerr << "Error: HWMC requires at least an opcode" << endl;

return;

}

opcode = inParams[0];

Copilot · 2026-02-10T09:59:08Z

+            void* ptr = malloc(bufferLength);
+            if (ptr == nullptr) {
+                RS_LOGE("Failed to allocate buffer %d of size %u", i, bufferLength);
+                return -1;
+            }
+            mRsBuffers.emplace_back(ptr, bufferLength, i);


On allocation failure, the function returns after potentially having already allocated earlier buffers in the loop, leaving the object in a partially-initialized state and potentially leaking memory/resources. Before returning, release/clear any previously allocated buffers (or ensure rollback via a scoped guard) so a failed start() doesn’t leave residual allocations.

Copilot · 2026-02-10T09:59:08Z

+        RS_LOGE("Invalid frame size: %u", frameSize);
+        return;
+    }
+    std::vector<char> imageVec(frameSize);


std::vector<char> imageVec(frameSize); allocates and zero-initializes a potentially large buffer on every processCaptureResult() call, which can be expensive on the hot capture path. Consider reusing a member buffer (resize only when needed) or using an uninitialized allocation strategy (where appropriate) to reduce per-frame allocations and memset costs.

Suggested change

std::vector<char> imageVec(frameSize);

static std::vector<char> imageVec;

if (imageVec.size() < frameSize) {

imageVec.resize(frameSize);

}

Copilot · 2026-02-10T09:59:09Z

        dfu_device = subprocess.check_output(["ls", "/sys/class/d4xx-class/"]).decode()
        assert "d4xx-dfu-" in dfu_device, "D4xx DFU device not found"

+        # Validate DFU device name to prevent path traversal
+        dfu_device_name = dfu_device.strip()
+        if not re.match(r'^d4xx-dfu-[0-9]+$', dfu_device_name):
+            raise ValueError(f"Invalid DFU device name: {dfu_device_name}")
+


ls /sys/class/d4xx-class/ can return multiple entries (multi-line output). After .strip(), dfu_device_name may still contain embedded newlines, causing the regex check to fail and the test to error even when valid DFU devices exist. Parse splitlines() and select a single matching entry (or iterate entries and pick the first d4xx-dfu-<n>), then build the /dev/... path from that chosen name.

Suggested change

dfu_device = subprocess.check_output(["ls", "/sys/class/d4xx-class/"]).decode()

assert "d4xx-dfu-" in dfu_device, "D4xx DFU device not found"

# Validate DFU device name to prevent path traversal

dfu_device_name = dfu_device.strip()

if not re.match(r'^d4xx-dfu-[0-9]+$', dfu_device_name):

raise ValueError(f"Invalid DFU device name: {dfu_device_name}")

dfu_device_output = subprocess.check_output(["ls", "/sys/class/d4xx-class/"]).decode()

# Parse ls output, which may contain multiple entries, and select a valid DFU device

dfu_entries = [line.strip() for line in dfu_device_output.splitlines() if line.strip()]

dfu_pattern = re.compile(r'^d4xx-dfu-[0-9]+$')

dfu_device_name = None

for entry in dfu_entries:

if dfu_pattern.match(entry):

dfu_device_name = entry

break

if dfu_device_name is None:

raise AssertionError("D4xx DFU device not found")

Copilot · 2026-02-10T09:59:09Z

+    rm -rf "$SRCS/out"
 fi

 mkdir -p $TEGRA_KERNEL_OUT


$TEGRA_KERNEL_OUT is still unquoted here. For consistency with the new hardening (and to avoid word-splitting/globbing issues), quote it: mkdir -p "$TEGRA_KERNEL_OUT".

Suggested change

mkdir -p $TEGRA_KERNEL_OUT

mkdir -p "$TEGRA_KERNEL_OUT"

- Move memset before empty check in HWMC constructor to ensure params initialization - Add cleanup of allocated buffers on malloc failure in StreamView::start() - Use static buffer in processCaptureResult() to avoid per-frame allocation overhead - Parse ls output with splitlines() to handle multiple DFU device entries - Quote remaining unquoted $TEGRA_KERNEL_OUT in build_all.sh Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

sareluzi · 2026-02-10T10:09:52Z

Thanks for the thorough review! I've addressed all 6 comments in commit 1595c7a:

hwmc.cpp - uninitialized params - Moved memset(params, 0, sizeof(params)) before the empty check so params are always initialized even on early return.
StreamView.cpp - memory leak on malloc failure - Added cleanup loop to free previously allocated buffers before returning -1.
StreamView.cpp - per-frame allocation overhead - Changed to static buffer with resize-on-demand pattern to avoid allocation on every frame.
test_fw_version.py - multi-entry ls output - Now using splitlines() to parse ls output and pattern matching to find valid DFU device entry.
build_all.sh - unquoted variable - Quoted all remaining $TEGRA_KERNEL_OUT occurrences including the mkdir -p calls.

aikido-pr-checks · 2026-02-10T10:10:02Z

+		if (inParams.empty()) {
+			cerr << "Error: HWMC requires at least an opcode" << endl;
+			return;
+		}


Constructor emits ad-hoc stderr output and returns early when inParams is empty; avoid leaving console diagnostic output and surprising early-return behavior in a constructor.

Details

✨ AI Reasoning
A newly added branch in the HWMC constructor writes an error message to stderr and returns early when no opcode is provided. This introduces ad-hoc console output from a constructor and changes control flow during object construction, which can be considered a leftover ad-hoc debug/diagnostic output and surprising behavior in production code. The change also leaves the object constructed with default/zeroed fields after returning from the constructor body, making caller behavior dependent on side-effecting stderr output. Flagging focuses on the newly added console error and early return introduced by this change.

🔧 How do I fix it?
Remove debugging statements like console.log, debugger, dd(), or logic bypasses like || true. Keep legitimate logging for monitoring and error handling.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

Show Fix

Remediation - low confidence
This patch mitigates ad-hoc debug output and early return in a constructor by removing the stderr diagnostic message and empty parameter check that left the object in a partially initialized state.

Suggested change

if (inParams.empty()) {

cerr << "Error: HWMC requires at least an opcode" << endl;

return;

}

Copilot AI review requested due to automatic review settings February 10, 2026 09:57

Copilot AI reviewed Feb 10, 2026

View reviewed changes

aikido-pr-checks Bot reviewed Feb 10, 2026

View reviewed changes

ymodlin closed this May 18, 2026

ymodlin deleted the fix/security-audit branch May 27, 2026 10:37

Navigation Menu

Uh oh!

Fix security vulnerabilities across codebase - #355

Fix security vulnerabilities across codebase#355
sareluzi wants to merge 2 commits into
devfrom
fix/security-audit

sareluzi commented Feb 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

Copilot AI Feb 10, 2026

Uh oh!

sareluzi commented Feb 10, 2026

Uh oh!

aikido-pr-checks Bot Feb 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-    std::vector<char> imageVec(frameSize);
+    static std::vector<char> imageVec;
+    if (imageVec.size() < frameSize) {
+        imageVec.resize(frameSize);
+    }

-        dfu_device = subprocess.check_output(["ls", "/sys/class/d4xx-class/"]).decode()
-        assert "d4xx-dfu-" in dfu_device, "D4xx DFU device not found"
-        # Validate DFU device name to prevent path traversal
-        dfu_device_name = dfu_device.strip()
-        if not re.match(r'^d4xx-dfu-[0-9]+$', dfu_device_name):
-            raise ValueError(f"Invalid DFU device name: {dfu_device_name}")
+        dfu_device_output = subprocess.check_output(["ls", "/sys/class/d4xx-class/"]).decode()
+        # Parse ls output, which may contain multiple entries, and select a valid DFU device
+        dfu_entries = [line.strip() for line in dfu_device_output.splitlines() if line.strip()]
+        dfu_pattern = re.compile(r'^d4xx-dfu-[0-9]+$')
+        dfu_device_name = None
+        for entry in dfu_entries:
+            if dfu_pattern.match(entry):
+                dfu_device_name = entry
+                break
+        if dfu_device_name is None:
+            raise AssertionError("D4xx DFU device not found")

Uh oh!

Conversation

sareluzi commented Feb 10, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

sareluzi commented Feb 10, 2026

Uh oh!

aikido-pr-checks Bot Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants