fix: Secure OpenWrt change_password against OS command injection

[kartikbhartiya]

Security Fix for OS Command Injection

We have identified and resolved a critical OS Command Injection vulnerability in the OpenWrt connection connector (change_password method).

Changes Made

1. Hardened command building in ssh.py

• File: ssh.py
• Problem: The original change_password method accepted unescaped user inputs password, confirm_password, and user, interpolating them directly into a double-quoted shell string inside echo -e. This allowed authenticated users to inject arbitrary shell commands.
• Solution:
  • Imported shlex and sanitized all inputs (password, confirm_password, user) using shlex.quote().
  • Refactored the command execution to use printf '%s\n%s\n' instead of echo -e. This avoids backslash interpretation issues for passwords that might contain backslashes, ensuring standard POSIX-compliant piping to the passwd binary.

2. Updated corresponding test case

• File: test_models.py
• Solution: Updated the test assertion in test_execute_change_password to match the newly generated, securely escaped command format (printf '%s\n%s\n' 'Newpasswd@123' 'Newpasswd@123' | passwd 'root').

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR hardens the remote password-change functionality in the OpenWrt SSH connector. The change_password method now escapes user-provided values (password, confirm password, and username) using shlex.quote and constructs the command via printf instead of interpolating them directly into an echo -e command. A corresponding test update verifies the new command format is generated correctly.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
Bug Fixes	❌ Error	The test assertion expects quoted values but shlex.quote() doesn't quote safe strings like "Newpasswd@123". The test would fail on execution.	Update test to match shlex.quote() output: "printf '%s\\n%s\\n' Newpasswd@123 Newpasswd@123 | passwd root" OR manually quote all values in implementation.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title follows the required [type] format with 'fix' type and clearly describes the main security improvement: preventing OS command injection in the change_password method.
Description check	✅ Passed	The description covers the security vulnerability, detailed changes to both files, and includes test updates, but is missing the standard template sections like checklist, issue reference, and screenshots.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

This PR correctly addresses a critical OS command injection vulnerability in the change_password method.

What was fixed:

• Original issue: Direct string interpolation of user-controlled password, confirm_password, and user values into a shell command allowed arbitrary command execution
• Solution applied:
  • shlex.quote() properly sanitizes all user inputs before shell execution
  • printf '%s\n%s\n' replaces echo -e for more reliable handling of passwords containing backslashes

Review notes:

• The test assertion is correctly updated to match the new command format
• The fix follows Python security best practices for shell command construction
• No regressions or edge cases identified

Files Reviewed (2 files)

• openwisp_controller/connection/connectors/openwrt/ssh.py - Security fix using shlex.quote() and printf
• openwisp_controller/connection/tests/test_models.py - Updated test assertion for new command format

---

_{Reviewed by kimi-k2.5-0127 · 88,845 tokens}

[Copilot]

Pull request overview

Addresses an OS command injection vulnerability in OpenWrt.change_password by sanitizing all string inputs via shlex.quote and replacing echo -e with printf '%s\n%s\n' to avoid backslash interpretation.

Changes:

• Sanitize password, confirm_password, and user arguments with shlex.quote before string interpolation.
• Switch from echo -e to printf '%s\n%s\n' for safer/POSIX-compliant piping into passwd.
• Update test_execute_change_password assertion to the new command format.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
openwisp_controller/connection/connectors/openwrt/ssh.py	Quote shell inputs and use printf instead of echo -e in change_password.
openwisp_controller/connection/tests/test_models.py	Update the expected exec_command argument in the change-password test.

Notes:

• The updated test's expected string ('Newpasswd@123', 'root') does not match what shlex.quote actually returns for those safe-character inputs, so the test will fail as written. There is also no new test asserting the quoting actually protects against shell metacharacters (e.g. a password containing spaces, ;, $, "), which is the core hardening claim of this PR.

---

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

[openwisp-companion]

Test Failures in openwisp_controller.connection.tests.test_models

Hello @kartikbhartiya,
(Analysis for commit 9fc2cad)

There is one test failure:

• test_execute_change_password failed with AssertionError: The exec_command call in the test is missing single quotes around the password arguments.

Fix:
Modify the assert_called_with line in openwisp_controller/connection/tests/test_models.py to include single quotes around the password:

mocked_exec_command.assert_called_with(
"printf '%s\\n%s\\n' 'Newpasswd@123' 'Newpasswd@123' | passwd 'root'", timeout=30
)