From 17b20fd103aa9afd6e45e4bb8fde0ec4221bf895 Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Sat, 29 Aug 2026 18:19:12 +0530 Subject: [PATCH 1/2] api,kvm: allow credentials in backup repository mount options The SafeCommandOptions whitelist added for command injection hardening only accepted [A-Za-z0-9,._=:/+-] and whitespace. Mount options are how credentials reach a CIFS backup repository, and any realistic value is rejected: an Active Directory username such as user@domain, or a password containing @ ! # % ^ ~. Adding or updating such a repository fails with "contains unsupported or unsafe characters". The list is now parsed for what it is, a comma separated list of "key" or "key=value" entries, with the punctuation that appears in credentials allowed in values only. It is not a loosening across the board. Keys keep the old restrictive character set, and everything the shell treats specially or expands is still rejected in both: whitespace, quotes, $ ` ; | & < > ( ) { } [ ] \ and the glob characters * and ?. Whitespace was previously accepted and is not any more, since that is what would let a value turn into extra mount arguments. nasbackup.sh interpolated the options into the mount command unquoted, so a value containing whitespace or a glob was split or expanded by the shell before mount saw it. The command is now built as an array and the options passed as a single quoted argument, so the option list cannot influence anything but the -o argument regardless of what validation allows through. --- .../cloudstack/api/ApiArgValidator.java | 25 +++-- .../repository/AddBackupRepositoryCmd.java | 2 +- .../repository/UpdateBackupRepositoryCmd.java | 2 +- scripts/vm/hypervisor/kvm/nasbackup.sh | 4 +- .../api/dispatch/ParamProcessWorker.java | 2 +- .../api/dispatch/ParamProcessWorkerTest.java | 104 +++++++++++++++++- 6 files changed, 127 insertions(+), 12 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/ApiArgValidator.java b/api/src/main/java/org/apache/cloudstack/api/ApiArgValidator.java index b60549346bfc..a98a665ef417 100644 --- a/api/src/main/java/org/apache/cloudstack/api/ApiArgValidator.java +++ b/api/src/main/java/org/apache/cloudstack/api/ApiArgValidator.java @@ -49,15 +49,21 @@ public enum ApiArgValidator { RFCComplianceDomainName, /** - * Validates command option strings to avoid unsafe/code-like content. + * Validates mount command option strings to avoid unsafe/code-like content. */ - SafeCommandOptions((param, annotation) -> { + SafeMountCommandOptions((param, annotation) -> { if (BaseCmd.CommandType.STRING.equals(annotation.type())) { - validateSafeCommandOptions(param, annotation.name()); + validateSafeMountCommandOptions(param, annotation.name()); } }); - private static final Pattern SAFE_COMMAND_OPTIONS_PATTERN = Pattern.compile("^[A-Za-z0-9,._=:/+\\-\\s]*$"); + /** + * A mount option list is a comma separated list of "key" or "key=value" entries. Keys stay + * restrictive. Values additionally allow the punctuation that commonly appears in credentials, + * for instance a CIFS username of the form user@domain or a password containing !#%^~. + */ + private static final Pattern SAFE_MOUNT_COMMAND_OPTION_PATTERN = + Pattern.compile("[A-Za-z0-9_.\\-]+(=[A-Za-z0-9_.\\-+:/@!#%^~=]*)?"); private static final String[] UNSAFE_TOKENS = { "$(", "`", "&&", "||", ";", "|", ">", "<" @@ -79,14 +85,19 @@ public void validate(final Object paramObj, final Parameter annotation) { } } - private static void validateSafeCommandOptions(final Object param, final String argName) { + private static void validateSafeMountCommandOptions(final Object param, final String argName) { + if (param == null) { + return; + } final String value = String.valueOf(param); if (StringUtils.isBlank(value)) { return; } - if (!SAFE_COMMAND_OPTIONS_PATTERN.matcher(value).matches()) { - throwInvalidParameterValueException(argName, "contains unsupported or unsafe characters"); + for (final String option : value.split(",", -1)) { + if (!SAFE_MOUNT_COMMAND_OPTION_PATTERN.matcher(option).matches()) { + throwInvalidParameterValueException(argName, "contains unsupported or unsafe characters"); + } } final String normalized = value.toLowerCase(Locale.ROOT); diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/AddBackupRepositoryCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/AddBackupRepositoryCmd.java index 630bca4f26a4..3f47554b9f1f 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/AddBackupRepositoryCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/AddBackupRepositoryCmd.java @@ -59,7 +59,7 @@ public class AddBackupRepositoryCmd extends BaseCmd { private String provider; @Parameter(name = ApiConstants.MOUNT_OPTIONS, type = CommandType.STRING, description = "shared storage mount options", - validations = {ApiArgValidator.SafeCommandOptions}) + validations = {ApiArgValidator.SafeMountCommandOptions}) private String mountOptions; @Parameter(name = ApiConstants.ZONE_ID, diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/UpdateBackupRepositoryCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/UpdateBackupRepositoryCmd.java index 740936221b5d..5d3f876a95f6 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/UpdateBackupRepositoryCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/UpdateBackupRepositoryCmd.java @@ -55,7 +55,7 @@ public class UpdateBackupRepositoryCmd extends BaseCmd { private String address; @Parameter(name = ApiConstants.MOUNT_OPTIONS, type = CommandType.STRING, description = "shared storage mount options", - validations = {ApiArgValidator.SafeCommandOptions}) + validations = {ApiArgValidator.SafeMountCommandOptions}) private String mountOptions; @Parameter(name = ApiConstants.CROSS_ZONE_INSTANCE_CREATION, type = CommandType.BOOLEAN, description = "backups in this repository can be used to create Instances on all Zones") diff --git a/scripts/vm/hypervisor/kvm/nasbackup.sh b/scripts/vm/hypervisor/kvm/nasbackup.sh index 656ff5ac28d6..a4161a71ead7 100755 --- a/scripts/vm/hypervisor/kvm/nasbackup.sh +++ b/scripts/vm/hypervisor/kvm/nasbackup.sh @@ -289,7 +289,9 @@ mount_operation() { if [ ${NAS_TYPE} == "cifs" ]; then MOUNT_OPTS="${MOUNT_OPTS},nobrl" fi - mount -t ${NAS_TYPE} ${NAS_ADDRESS} ${mount_point} $([[ ! -z "${MOUNT_OPTS}" ]] && echo -o ${MOUNT_OPTS}) 2>&1 | tee -a "$logFile" + mount_args=(-t "${NAS_TYPE}" "${NAS_ADDRESS}" "${mount_point}") + [[ -n "${MOUNT_OPTS}" ]] && mount_args+=(-o "${MOUNT_OPTS}") + mount "${mount_args[@]}" 2>&1 | tee -a "$logFile" if [ $? -eq 0 ]; then log -ne "Successfully mounted ${NAS_TYPE} store" else diff --git a/server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java b/server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java index 8592d1a9fef8..ce0c30883aef 100644 --- a/server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java +++ b/server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java @@ -174,7 +174,7 @@ private void validateField(final Object paramObj, final Parameter annotation) th break; } break; - case SafeCommandOptions: + case SafeMountCommandOptions: validator.validate(paramObj, annotation); break; default: diff --git a/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java b/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java index 81e14bcfe66f..528fb53f83e3 100644 --- a/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java +++ b/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java @@ -95,7 +95,7 @@ public static class TestCmd extends BaseCmd { @Parameter(name = "vmHostNameParam", type = CommandType.STRING, validations = {ApiArgValidator.RFCComplianceDomainName}) String vmHostNameParam; - @Parameter(name = "mountOptions", type = CommandType.STRING, validations = {ApiArgValidator.SafeCommandOptions}) + @Parameter(name = "mountOptions", type = CommandType.STRING, validations = {ApiArgValidator.SafeMountCommandOptions}) String mountOptions; @Override @@ -151,6 +151,108 @@ public void processMountOptionsParameter_Valid() { Assert.assertEquals("vers=4.1,soft,timeo=600,retrans=2", cmd.mountOptions); } + @Test + public void processMountOptionsParameter_AcceptsCifsCredentials() { + final HashMap params = new HashMap(); + // CIFS credentials routinely contain punctuation that is harmless in a mount option list. + final String options = "username=backup@corp.example.com,password=P@ssw0rd!#%^~,vers=3.0"; + params.put("mountOptions", options); + final TestCmd cmd = new TestCmd(); + paramProcessWorkerSpy.processParameters(cmd, params); + Assert.assertEquals(options, cmd.mountOptions); + } + + @Test + public void processMountOptionsParameter_AcceptsBase64LikePassword() { + final HashMap params = new HashMap(); + final String options = "username=backup,password=YWJjZGVmZ2g="; + params.put("mountOptions", options); + final TestCmd cmd = new TestCmd(); + paramProcessWorkerSpy.processParameters(cmd, params); + Assert.assertEquals(options, cmd.mountOptions); + } + + @Test + public void processMountOptionsParameter_AcceptsCephFsOptions() { + final HashMap params = new HashMap(); + // A CephFS repository is mounted with the cephx user and a bare option such as defaults. + final String options = "name=user,secret=xyz,defaults"; + params.put("mountOptions", options); + final TestCmd cmd = new TestCmd(); + paramProcessWorkerSpy.processParameters(cmd, params); + Assert.assertEquals(options, cmd.mountOptions); + } + + @Test + public void processMountOptionsParameter_AcceptsCephFsBase64Secret() { + final HashMap params = new HashMap(); + // A cephx key is base64, so it can contain + / and trailing =. + final String options = "name=cloudstack,secret=AQBvE2VmS0J8FxAA9F1c2Wq+8kZ3Xn5Yz7Lw==,defaults"; + params.put("mountOptions", options); + final TestCmd cmd = new TestCmd(); + paramProcessWorkerSpy.processParameters(cmd, params); + Assert.assertEquals(options, cmd.mountOptions); + } + + @Test + public void processMountOptionsParameter_AcceptsCephFsSecretFile() { + final HashMap params = new HashMap(); + final String options = "name=user,secretfile=/etc/ceph/secret.key,_netdev"; + params.put("mountOptions", options); + final TestCmd cmd = new TestCmd(); + paramProcessWorkerSpy.processParameters(cmd, params); + Assert.assertEquals(options, cmd.mountOptions); + } + + @Test(expected = ServerApiException.class) + public void processMountOptionsParameter_RejectCephFsSecretWithCommandSubstitution() { + final HashMap params = new HashMap(); + params.put("mountOptions", "name=user,secret=$(cat /etc/ceph/keyring)"); + final TestCmd cmd = new TestCmd(); + paramProcessWorkerSpy.processParameters(cmd, params); + } + + @Test(expected = ServerApiException.class) + public void processMountOptionsParameter_RejectWhitespace() { + final HashMap params = new HashMap(); + // Whitespace would turn into additional arguments to mount. + params.put("mountOptions", "vers=4.1,soft -o remount,rw"); + final TestCmd cmd = new TestCmd(); + paramProcessWorkerSpy.processParameters(cmd, params); + } + + @Test(expected = ServerApiException.class) + public void processMountOptionsParameter_RejectCommandSubstitution() { + final HashMap params = new HashMap(); + params.put("mountOptions", "vers=4.1,password=$(id)"); + final TestCmd cmd = new TestCmd(); + paramProcessWorkerSpy.processParameters(cmd, params); + } + + @Test(expected = ServerApiException.class) + public void processMountOptionsParameter_RejectBackticks() { + final HashMap params = new HashMap(); + params.put("mountOptions", "vers=4.1,password=`id`"); + final TestCmd cmd = new TestCmd(); + paramProcessWorkerSpy.processParameters(cmd, params); + } + + @Test(expected = ServerApiException.class) + public void processMountOptionsParameter_RejectGlob() { + final HashMap params = new HashMap(); + params.put("mountOptions", "vers=4.1,credentials=/etc/*"); + final TestCmd cmd = new TestCmd(); + paramProcessWorkerSpy.processParameters(cmd, params); + } + + @Test(expected = ServerApiException.class) + public void processMountOptionsParameter_RejectOptionWithoutKey() { + final HashMap params = new HashMap(); + params.put("mountOptions", "vers=4.1,=value"); + final TestCmd cmd = new TestCmd(); + paramProcessWorkerSpy.processParameters(cmd, params); + } + @Test(expected = ServerApiException.class) public void processMountOptionsParameter_RejectCodeLikeContent() { final HashMap params = new HashMap(); From 5af611f78f43de3c0bd1ac145e99eadc8a35b661 Mon Sep 17 00:00:00 2001 From: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com> Date: Sun, 30 Aug 2026 20:53:56 +0530 Subject: [PATCH 2/2] api,kvm: reject a mount option list that only looks empty Validation returned early for a blank value, so a list of nothing but whitespace was accepted although whitespace is rejected everywhere else in the list. The two sides then disagreed about it: the restore wrapper treats it as no options at all, while the backup script sees a non empty string and hands it to mount as an option of its own. An empty value still clears the options, anything else is now validated. The array holding the mount command is also declared local, as arrays assigned in a bash function are otherwise global. mount_point and dest stay as they are, the callers of mount_operation read them. --- .../apache/cloudstack/api/ApiArgValidator.java | 5 ++++- scripts/vm/hypervisor/kvm/nasbackup.sh | 2 +- .../api/dispatch/ParamProcessWorkerTest.java | 18 ++++++++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/ApiArgValidator.java b/api/src/main/java/org/apache/cloudstack/api/ApiArgValidator.java index a98a665ef417..c6832c86065b 100644 --- a/api/src/main/java/org/apache/cloudstack/api/ApiArgValidator.java +++ b/api/src/main/java/org/apache/cloudstack/api/ApiArgValidator.java @@ -90,7 +90,10 @@ private static void validateSafeMountCommandOptions(final Object param, final St return; } final String value = String.valueOf(param); - if (StringUtils.isBlank(value)) { + // An empty value clears the mount options and is allowed. A value that only looks empty is + // not: whitespace is rejected everywhere else in the list, and the backup script would pass + // it on to mount as an option of its own. + if (value.isEmpty()) { return; } diff --git a/scripts/vm/hypervisor/kvm/nasbackup.sh b/scripts/vm/hypervisor/kvm/nasbackup.sh index a4161a71ead7..bf2990e4ddd7 100755 --- a/scripts/vm/hypervisor/kvm/nasbackup.sh +++ b/scripts/vm/hypervisor/kvm/nasbackup.sh @@ -289,7 +289,7 @@ mount_operation() { if [ ${NAS_TYPE} == "cifs" ]; then MOUNT_OPTS="${MOUNT_OPTS},nobrl" fi - mount_args=(-t "${NAS_TYPE}" "${NAS_ADDRESS}" "${mount_point}") + local mount_args=(-t "${NAS_TYPE}" "${NAS_ADDRESS}" "${mount_point}") [[ -n "${MOUNT_OPTS}" ]] && mount_args+=(-o "${MOUNT_OPTS}") mount "${mount_args[@]}" 2>&1 | tee -a "$logFile" if [ $? -eq 0 ]; then diff --git a/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java b/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java index 528fb53f83e3..47fe2d59f45c 100644 --- a/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java +++ b/server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java @@ -212,6 +212,24 @@ public void processMountOptionsParameter_RejectCephFsSecretWithCommandSubstituti paramProcessWorkerSpy.processParameters(cmd, params); } + @Test + public void processMountOptionsParameter_AcceptsEmptyValueToClearTheOptions() { + final HashMap params = new HashMap(); + params.put("mountOptions", ""); + final TestCmd cmd = new TestCmd(); + paramProcessWorkerSpy.processParameters(cmd, params); + Assert.assertEquals("", cmd.mountOptions); + } + + @Test(expected = ServerApiException.class) + public void processMountOptionsParameter_RejectWhitespaceOnly() { + final HashMap params = new HashMap(); + // Only looks empty: the backup script would hand this to mount as an option. + params.put("mountOptions", " "); + final TestCmd cmd = new TestCmd(); + paramProcessWorkerSpy.processParameters(cmd, params); + } + @Test(expected = ServerApiException.class) public void processMountOptionsParameter_RejectWhitespace() { final HashMap params = new HashMap();