Copilot commented on code in PR #13668:
URL: https://github.com/apache/cloudstack/pull/13668#discussion_r3683383984
##########
plugins/hypervisors/baremetal/src/main/java/com/cloud/baremetal/networkservice/BaremetalPingPxeResource.java:
##########
@@ -241,8 +258,9 @@ private Answer execute(VmDataCommand cmd) {
}
String script = String.format("python
/usr/bin/baremetal_user_data.py '%s'", arg);
- if (!SSHCmdHelper.sshExecuteCmd(sshConnection, script)) {
- return new Answer(cmd, false, "Failed to add user data,
command:" + script);
+ String maskedScript = String.format("python
/usr/bin/baremetal_user_data.py '%s'", maskedArg);
+ if (!SSHCmdHelper.sshExecuteCmd(sshConnection, script,
maskedScript)) {
+ return new Answer(cmd, false, "Failed to add user data,
command:" + maskedScript);
Review Comment:
Wrapping `arg` in single quotes does not safely escape embedded single
quotes or other shell-sensitive characters that can occur in user-data/SSH
keys, which can break the command or enable shell injection depending on
contents. Consider passing the payload via a safer channel (e.g., base64-encode
and decode server-side, write to a temp file via SFTP, or robustly escape
single quotes for POSIX shells) rather than direct interpolation into a shell
command.
##########
utils/src/main/java/com/cloud/utils/ssh/SSHCmdHelper.java:
##########
@@ -227,7 +255,7 @@ public static SSHCmdResult
sshExecuteCmdOneShot(com.trilead.ssh2.Connection sshC
final SSHCmdResult result = new SSHCmdResult(-1,
sbStdoutResult.toString(), sbStdErrResult.toString());
if (!StringUtils.isAllEmpty(result.getStdOut(),
result.getStdErr())) {
- LOGGER.debug("SSH command: " +
cmd.split(KeyStoreUtils.KS_FILENAME)[0] + "\nSSH command output:" +
result.getStdOut().split("-----BEGIN")[0] + "\n" + result.getStdErr());
+ LOGGER.debug("SSH command: " + cmdForLogging + "\nSSH command
output:" + result.getStdOut().split("-----BEGIN")[0] + "\n" +
result.getStdErr());
}
Review Comment:
Even when a `maskedCmd` is provided (indicating the command contains
secrets), the debug log still prints full stderr and (unless it contains a PEM
header) essentially full stdout. That can still leak sensitive values if the
remote script echoes inputs or prints user-data/keys. Consider suppressing
stdout/stderr logging (or heavily redacting it) whenever `maskedCmd != null`,
e.g., log only exit status and/or a fixed 'output omitted (sensitive)' marker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
