Skip to content

Conversation

tulik
Copy link
Contributor

@tulik tulik commented Jul 8, 2025

Harden Shell Execution Against Command Injection

Overview

This patch series eliminates command-injection vectors in networking and configuration modules by escaping every shell argument with escapeshellarg().

Files Updated

Area Files
Networking scripts ajax/networking/do_sys_reset.php
ajax/networking/get_ip_summary.php
ajax/networking/get_netcfg.php
Configuration & client management includes/configure_client.php
includes/dhcp.php
includes/get_clients.php
includes/provider.php
Host AP control includes/hostapd.php

Key Fixes

  • Replaced all string-concatenated command lines with safely escaped argument lists.
  • Removed chained commands to prevent multi-stage injection.
  • Normalised helper functions to enforce escaping on future additions.

Impact

  • Closes all known command-injection paths.
  • No breaking changes for existing configs or CLI usage.
  • Improves overall security posture.

Validation

  • Fuzz-tested each entry point with malicious payloads; no execution occurred.
  • Regression suite passed for networking, DHCP, and client-management workflows.

tulik added 2 commits July 8, 2025 21:36
- Add proper escapeshellarg() escaping for all user inputs passed to shell commands
- Fixed vulnerabilities in networking, client management, and provider modules
- Ensures all network interface names, file paths, and command parameters are properly escaped
- Addresses security issues found in code review of shell script execution patterns
@tulik tulik force-pushed the fix/command-injection-vulnerabilities branch from e861284 to 5988d29 Compare July 9, 2025 05:35
@billz
Copy link
Member

billz commented Jul 9, 2025

@tulik thanks for this PR. one item to note:

  • includes/get_clients.php is deprecated and will be removed in the next release

I'll carve out time to test the other changes.

@billz
Copy link
Member

billz commented Jul 20, 2025

@tulik just a heads up: there's a big refactor underway in #1907. you may want to rebase this branch against it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants