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..c6832c86065b 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,22 @@ 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)) { + // 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; } - 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..bf2990e4ddd7 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" + 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 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..47fe2d59f45c 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,126 @@ 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 + 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(); + // 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();