Skip to content

Fix security vulnerabilities across codebase #355

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

Closed
sareluzi wants to merge 2 commits into from
Closed

Fix security vulnerabilities across codebase #355

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3567ffa
Fix security vulnerabilities across codebase
sareluzi Feb 10, 2026
1595c7a
Address PR review comments
sareluzi Feb 10, 2026
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
5 changes: 4 additions & 1 deletion .github/workflows/build-jp502.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ jobs:
- name: setup workspace
run: yes | ./setup_workspace.sh 5.0.2
- name: apply patches
run: git config --global user.email "builder@example.com" && git config --global user.name "builder" && ./apply_patches.sh 5.0.2
run: |
git config --global user.email "${{ github.actor }}@users.noreply.github.com"
git config --global user.name "${{ github.actor }}"
./apply_patches.sh 5.0.2
- name: build
run: ./build_all.sh 5.0.2
5 changes: 4 additions & 1 deletion .github/workflows/build-jp512.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ jobs:
- name: setup workspace
run: yes | ./setup_workspace.sh 5.1.2
- name: apply patches
run: git config --global user.email "builder@example.com" && git config --global user.name "builder" && ./apply_patches.sh 5.1.2
run: |
git config --global user.email "${{ github.actor }}@users.noreply.github.com"
git config --global user.name "${{ github.actor }}"
./apply_patches.sh 5.1.2
- name: build
run: ./build_all.sh 5.1.2
5 changes: 4 additions & 1 deletion .github/workflows/build-jp6.1.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ jobs:
- name: setup workspace
run: yes | ./setup_workspace.sh 6.1
- name: apply patches
run: git config --global user.email "builder@example.com" && git config --global user.name "builder" && ./apply_patches.sh 6.1
run: |
git config --global user.email "${{ github.actor }}@users.noreply.github.com"
git config --global user.name "${{ github.actor }}"
./apply_patches.sh 6.1
- name: build
run: ./build_all.sh 6.1
5 changes: 4 additions & 1 deletion .github/workflows/build-jp6.2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ jobs:
- name: setup workspace
run: yes | ./setup_workspace.sh 6.2
- name: apply patches
run: git config --global user.email "builder@example.com" && git config --global user.name "builder" && ./apply_patches.sh 6.2
run: |
git config --global user.email "${{ github.actor }}@users.noreply.github.com"
git config --global user.name "${{ github.actor }}"
./apply_patches.sh 6.2
- name: build
run: ./build_all.sh 6.2
5 changes: 4 additions & 1 deletion .github/workflows/build-jp6.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ jobs:
- name: setup workspace
run: yes | ./setup_workspace.sh 6.0
- name: apply patches
run: git config --global user.email "builder@example.com" && git config --global user.name "builder" && ./apply_patches.sh 6.0
run: |
git config --global user.email "${{ github.actor }}@users.noreply.github.com"
git config --global user.name "${{ github.actor }}"
./apply_patches.sh 6.0
- name: build
run: ./build_all.sh 6.0
10 changes: 7 additions & 3 deletions build_all.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,12 @@ export TEGRA_KERNEL_OUT="$DEVDIR/images/$1"
# Clean if requested
if [[ $CLEAN == 1 ]]; then
echo "Cleaning build artifacts for $1..."
rm -rf $TEGRA_KERNEL_OUT
rm -rf $SRCS/out
if [[ -z "$TEGRA_KERNEL_OUT" ]]; then
echo "Error: TEGRA_KERNEL_OUT is not set"
exit 1
fi
rm -rf "$TEGRA_KERNEL_OUT"
rm -rf "$SRCS/out"
fi

mkdir -p $TEGRA_KERNEL_OUT

Copilot AI Feb 10, 2026

Copy link

Choose a reason for hiding this comment

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

$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"

Copilot uses AI. Check for mistakes.
Expand All @@ -75,7 +79,7 @@ export KERNEL_MODULES_OUT=$TEGRA_KERNEL_OUT/modules
if [[ "$JETPACK_VERSION" == "6.x" ]]; then
cd $SRCS
export KERNEL_HEADERS=$SRCS/kernel/kernel-jammy-src
ln -sf $TEGRA_KERNEL_OUT $SRCS/out
ln -sf "$TEGRA_KERNEL_OUT" "$SRCS/out"
if [[ "$DEVDBG" == "1" ]]; then
cd $KERNEL_HEADERS
# Generate .config file from default defconfig
Expand Down
8 changes: 6 additions & 2 deletions test/install.tegra.artifacts.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
#!/bin/bash
set -e

# add jenkins user to sudo group
# add the following line to /etc/sudoers for jenkins user, here nvidia
# SECURITY WARNING: The sudoers configuration below grants passwordless root access.
# Only use in isolated CI/CD environments with restricted network access.
# Never enable on production systems or systems with sensitive data.
# Consider using dedicated CI service accounts with audit logging.
#
# To enable for jenkins/CI user (NOT RECOMMENDED for production):
# nvidia ALL=(root) NOPASSWD: /sbin/reboot, /sbin/install.tegra.artifacts.sh

RELEASE=$(ls lib/modules)
Expand Down
21 changes: 18 additions & 3 deletions test/test_fw_version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,24 @@
import subprocess
import pytest
import re

def validate_video_device(device):
"""Validate device is a safe video device path."""
# Accept only video device numbers (0-99)
if not re.match(r'^[0-9]{1,2}$', str(device)):
raise ValueError(f"Invalid device: {device}. Expected video device number (0-99)")
return str(device)

@pytest.mark.d457
@pytest.mark.parametrize("device", {'0'})
def test_fw_version(device):
try:
device = validate_video_device(device)
key = "fw_version"
result = subprocess.check_call(["v4l2-ctl", "-d"+device, "-C", key])
result = subprocess.check_call(["v4l2-ctl", "-d", device, "-C", key])
assert result == 0

std_output = subprocess.check_output(["v4l2-ctl", "-d"+device, "-C", key])
std_output = subprocess.check_output(["v4l2-ctl", "-d", device, "-C", key])
key += ": "
assert key in std_output.decode(), "Couldn't fetch FW version"

Expand All @@ -26,8 +35,14 @@ def test_fw_version(device):
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}")

Copilot AI Feb 10, 2026

Copy link

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
# Get FW version from DFU device info
dfu_device_info = subprocess.check_output(["cat", "/dev/"+dfu_device.strip()]).decode()
dfu_device_path = f"/dev/{dfu_device_name}"
dfu_device_info = subprocess.check_output(["cat", dfu_device_path]).decode()

# Check whether the DFU info also has same FW version
assert fw_version_str in dfu_device_info, "FW versions read through v4l2-ctl utility and DFU device info doesn't match"
Expand Down
6 changes: 4 additions & 2 deletions test/test_metadata/framesextract.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ void process_frame_data(void){
char *token;
unsigned int col = 0;
char tmp[1024];
strcpy(tmp, line);
strncpy(tmp, line, sizeof(tmp) - 1);
tmp[sizeof(tmp) - 1] = '\0';
token = strtok(tmp, ",");
unsigned long int curr_val = 0;
while (token && col < 3) {
Expand Down Expand Up @@ -128,7 +129,8 @@ void process_frame_data(void){
while (fgets(line, sizeof(line), csv)) {
lines[count] = strdup(line);
char tmp[1024];
strcpy(tmp, line);
strncpy(tmp, line, sizeof(tmp) - 1);
tmp[sizeof(tmp) - 1] = '\0';
char *token = strtok(tmp, ",");
int col = 0;
while (token && col < 3) {
Expand Down
5 changes: 4 additions & 1 deletion utilities/streamApp/ds5_fw_logger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,10 @@ int main(int argc, char *argv[])
cout << "FW_Log_Data:";
for (i = 4; i < sizeof(ds5_fw_log_msg) + HEADER_SIZE; i++)
{
cout << uppercase << std::setfill ('0') << setw(2) << std::hex << (int)log[j*sizeof(ds5_fw_log_msg)+i] << " ";
size_t idx = j * sizeof(ds5_fw_log_msg) + i;
if (idx >= sizeof(log))
break;
cout << uppercase << std::setfill ('0') << setw(2) << std::hex << (int)log[idx] << " ";
}
cout << endl;
last_seq = msg->seq_id;
Expand Down
17 changes: 15 additions & 2 deletions utilities/streamApp/gui/StreamView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,12 @@ int StreamView::start(uint32_t memoryType) {
break;
case V4L2_MEMORY_USERPTR:
for (int i = 0; i < mBuffersCount; i++) {
mRsBuffers.emplace_back(malloc(bufferLength), bufferLength, i);
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);
Comment on lines +235 to +247

Copilot AI Feb 10, 2026

Copy link

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}
break;
default:
Expand Down Expand Up @@ -281,7 +286,15 @@ void StreamView::processCaptureResult(uint32_t index)
uint32_t cnt = 0;
char* left;
char* right;
char image[mFormat.calcBytesPerFrame()];
// Use heap allocation instead of VLA to prevent stack overflow
uint32_t frameSize = mFormat.calcBytesPerFrame();
constexpr uint32_t MAX_FRAME_SIZE = 64 * 1024 * 1024; // 64MB max
if (frameSize == 0 || frameSize > MAX_FRAME_SIZE) {
RS_LOGE("Invalid frame size: %u", frameSize);
return;
}
std::vector<char> imageVec(frameSize);

Copilot AI Feb 10, 2026

Copy link

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
char* image = imageVec.data();
//lock_guard<mutex> lock(mMutex);
switch(mStreamType) {
case V4L2Utils::StreamUtils::StreamType::RS_DEPTH_STREAM:
Expand Down
20 changes: 15 additions & 5 deletions utilities/streamApp/hwmc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <vector>
#include <cstring>
#include <iomanip>
#include <algorithm>

#include <fcntl.h>
#include <sys/ioctl.h>
Expand All @@ -22,11 +23,16 @@ using namespace realsense::utils;

#pragma pack(push, 1)
struct HWMC {
HWMC(const vector<int32_t> &inParams):header(0x14), magic_word(0xCDAB) {
HWMC(const vector<int32_t> &inParams):header(0x14), magic_word(0xCDAB), opcode(0) {
if (inParams.empty()) {
cerr << "Error: HWMC requires at least an opcode" << endl;
return;
}
Comment on lines +28 to +31

@aikido-pr-checks aikido-pr-checks Bot Feb 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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;
}

opcode = inParams[0];
memset(params, 0, sizeof(params));

Copilot AI Feb 10, 2026

Copy link

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
for (size_t i = 1; i < inParams.size(); i++)
params[i-1] = inParams[i];
size_t paramCount = std::min(inParams.size() - 1, sizeof(params)/sizeof(params[0]));
for (size_t i = 0; i < paramCount; i++)
params[i] = inParams[i + 1];
}
uint16_t header = 0x14;
uint16_t magic_word = 0xCDAB;
Expand Down Expand Up @@ -64,7 +70,7 @@ int main(int argc, char *argv[]) {
if (!fd)
return -1;

uint8_t hwmcBuff[1028] {0};
uint8_t hwmcBuff[1024 + sizeof(struct HWMC)] {0};
memset(hwmcBuff, 0, sizeof(hwmcBuff));

struct v4l2_ext_control ctrl {0};
Expand All @@ -84,9 +90,13 @@ int main(int argc, char *argv[]) {
return -1;
}
if (hmc.opcode == *(hwmcBuff + sizeof(struct HWMC))) {
uint16_t outLen = hwmcBuff[1001 + sizeof(struct HWMC)] << 8;
uint16_t outLen = hwmcBuff[1001 + sizeof(struct HWMC)] << 8;
outLen |= hwmcBuff[1000 + sizeof(struct HWMC)];
cout << "output length: "<< outLen << endl;;
// Bounds check: ensure we don't read past buffer
uint16_t maxLen = sizeof(hwmcBuff) - sizeof(struct HWMC) - 4;
if (outLen > maxLen)
outLen = maxLen;
for (int i = 0; i < outLen; ++i) {
if (i != 0 && 0 == (i % 16))
cout << endl;
Expand Down