Skip to content

Commit 0de29a0

Browse files
abh1sarabh1sar
committed
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.
1 parent 7ea1dca commit 0de29a0

6 files changed

Lines changed: 87 additions & 12 deletions

File tree

‎api/src/main/java/org/apache/cloudstack/api/ApiArgValidator.java‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,15 +49,21 @@ public enum ApiArgValidator {
4949
RFCComplianceDomainName,
5050

5151
/**
52-
* Validates command option strings to avoid unsafe/code-like content.
52+
* Validates mount command option strings to avoid unsafe/code-like content.
5353
*/
54-
SafeCommandOptions((param, annotation) -> {
54+
SafeMountCommandOptions((param, annotation) -> {
5555
if (BaseCmd.CommandType.STRING.equals(annotation.type())) {
56-
validateSafeCommandOptions(param, annotation.name());
56+
validateSafeMountCommandOptions(param, annotation.name());
5757
}
5858
});
5959

60-
private static final Pattern SAFE_COMMAND_OPTIONS_PATTERN = Pattern.compile("^[A-Za-z0-9,._=:/+\\-\\s]*$");
60+
/**
61+
* A mount option list is a comma separated list of "key" or "key=value" entries. Keys stay
62+
* restrictive. Values additionally allow the punctuation that commonly appears in credentials,
63+
* for instance a CIFS username of the form user@domain or a password containing !#%^~.
64+
*/
65+
private static final Pattern SAFE_MOUNT_COMMAND_OPTION_PATTERN =
66+
Pattern.compile("[A-Za-z0-9_.\\-]+(=[A-Za-z0-9_.\\-+:/@!#%^~=]*)?");
6167

6268
private static final String[] UNSAFE_TOKENS = {
6369
"$(", "`", "&&", "||", ";", "|", ">", "<"
@@ -79,14 +85,19 @@ public void validate(final Object paramObj, final Parameter annotation) {
7985
}
8086
}
8187

82-
private static void validateSafeCommandOptions(final Object param, final String argName) {
88+
private static void validateSafeMountCommandOptions(final Object param, final String argName) {
89+
if (param == null) {
90+
return;
91+
}
8392
final String value = String.valueOf(param);
8493
if (StringUtils.isBlank(value)) {
8594
return;
8695
}
8796

88-
if (!SAFE_COMMAND_OPTIONS_PATTERN.matcher(value).matches()) {
89-
throwInvalidParameterValueException(argName, "contains unsupported or unsafe characters");
97+
for (final String option : value.split(",", -1)) {
98+
if (!SAFE_MOUNT_COMMAND_OPTION_PATTERN.matcher(option).matches()) {
99+
throwInvalidParameterValueException(argName, "contains unsupported or unsafe characters");
100+
}
90101
}
91102

92103
final String normalized = value.toLowerCase(Locale.ROOT);

‎api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/AddBackupRepositoryCmd.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public class AddBackupRepositoryCmd extends BaseCmd {
5959
private String provider;
6060

6161
@Parameter(name = ApiConstants.MOUNT_OPTIONS, type = CommandType.STRING, description = "shared storage mount options",
62-
validations = {ApiArgValidator.SafeCommandOptions})
62+
validations = {ApiArgValidator.SafeMountCommandOptions})
6363
private String mountOptions;
6464

6565
@Parameter(name = ApiConstants.ZONE_ID,

‎api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/UpdateBackupRepositoryCmd.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ public class UpdateBackupRepositoryCmd extends BaseCmd {
5555
private String address;
5656

5757
@Parameter(name = ApiConstants.MOUNT_OPTIONS, type = CommandType.STRING, description = "shared storage mount options",
58-
validations = {ApiArgValidator.SafeCommandOptions})
58+
validations = {ApiArgValidator.SafeMountCommandOptions})
5959
private String mountOptions;
6060

6161
@Parameter(name = ApiConstants.CROSS_ZONE_INSTANCE_CREATION, type = CommandType.BOOLEAN, description = "backups in this repository can be used to create Instances on all Zones")

‎scripts/vm/hypervisor/kvm/nasbackup.sh‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,9 @@ mount_operation() {
289289
if [ ${NAS_TYPE} == "cifs" ]; then
290290
MOUNT_OPTS="${MOUNT_OPTS},nobrl"
291291
fi
292-
mount -t ${NAS_TYPE} ${NAS_ADDRESS} ${mount_point} $([[ ! -z "${MOUNT_OPTS}" ]] && echo -o ${MOUNT_OPTS}) 2>&1 | tee -a "$logFile"
292+
mount_args=(-t "${NAS_TYPE}" "${NAS_ADDRESS}" "${mount_point}")
293+
[[ -n "${MOUNT_OPTS}" ]] && mount_args+=(-o "${MOUNT_OPTS}")
294+
mount "${mount_args[@]}" 2>&1 | tee -a "$logFile"
293295
if [ $? -eq 0 ]; then
294296
log -ne "Successfully mounted ${NAS_TYPE} store"
295297
else

‎server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ private void validateField(final Object paramObj, final Parameter annotation) th
174174
break;
175175
}
176176
break;
177-
case SafeCommandOptions:
177+
case SafeMountCommandOptions:
178178
validator.validate(paramObj, annotation);
179179
break;
180180
default:

‎server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java‎

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ public static class TestCmd extends BaseCmd {
9595
@Parameter(name = "vmHostNameParam", type = CommandType.STRING, validations = {ApiArgValidator.RFCComplianceDomainName})
9696
String vmHostNameParam;
9797

98-
@Parameter(name = "mountOptions", type = CommandType.STRING, validations = {ApiArgValidator.SafeCommandOptions})
98+
@Parameter(name = "mountOptions", type = CommandType.STRING, validations = {ApiArgValidator.SafeMountCommandOptions})
9999
String mountOptions;
100100

101101
@Override
@@ -151,6 +151,68 @@ public void processMountOptionsParameter_Valid() {
151151
Assert.assertEquals("vers=4.1,soft,timeo=600,retrans=2", cmd.mountOptions);
152152
}
153153

154+
@Test
155+
public void processMountOptionsParameter_AcceptsCifsCredentials() {
156+
final HashMap<String, String> params = new HashMap<String, String>();
157+
// CIFS credentials routinely contain punctuation that is harmless in a mount option list.
158+
final String options = "username=backup@corp.example.com,password=P@ssw0rd!#%^~,vers=3.0";
159+
params.put("mountOptions", options);
160+
final TestCmd cmd = new TestCmd();
161+
paramProcessWorkerSpy.processParameters(cmd, params);
162+
Assert.assertEquals(options, cmd.mountOptions);
163+
}
164+
165+
@Test
166+
public void processMountOptionsParameter_AcceptsBase64LikePassword() {
167+
final HashMap<String, String> params = new HashMap<String, String>();
168+
final String options = "username=backup,password=YWJjZGVmZ2g=";
169+
params.put("mountOptions", options);
170+
final TestCmd cmd = new TestCmd();
171+
paramProcessWorkerSpy.processParameters(cmd, params);
172+
Assert.assertEquals(options, cmd.mountOptions);
173+
}
174+
175+
@Test(expected = ServerApiException.class)
176+
public void processMountOptionsParameter_RejectWhitespace() {
177+
final HashMap<String, String> params = new HashMap<String, String>();
178+
// Whitespace would turn into additional arguments to mount.
179+
params.put("mountOptions", "vers=4.1,soft -o remount,rw");
180+
final TestCmd cmd = new TestCmd();
181+
paramProcessWorkerSpy.processParameters(cmd, params);
182+
}
183+
184+
@Test(expected = ServerApiException.class)
185+
public void processMountOptionsParameter_RejectCommandSubstitution() {
186+
final HashMap<String, String> params = new HashMap<String, String>();
187+
params.put("mountOptions", "vers=4.1,password=$(id)");
188+
final TestCmd cmd = new TestCmd();
189+
paramProcessWorkerSpy.processParameters(cmd, params);
190+
}
191+
192+
@Test(expected = ServerApiException.class)
193+
public void processMountOptionsParameter_RejectBackticks() {
194+
final HashMap<String, String> params = new HashMap<String, String>();
195+
params.put("mountOptions", "vers=4.1,password=`id`");
196+
final TestCmd cmd = new TestCmd();
197+
paramProcessWorkerSpy.processParameters(cmd, params);
198+
}
199+
200+
@Test(expected = ServerApiException.class)
201+
public void processMountOptionsParameter_RejectGlob() {
202+
final HashMap<String, String> params = new HashMap<String, String>();
203+
params.put("mountOptions", "vers=4.1,credentials=/etc/*");
204+
final TestCmd cmd = new TestCmd();
205+
paramProcessWorkerSpy.processParameters(cmd, params);
206+
}
207+
208+
@Test(expected = ServerApiException.class)
209+
public void processMountOptionsParameter_RejectOptionWithoutKey() {
210+
final HashMap<String, String> params = new HashMap<String, String>();
211+
params.put("mountOptions", "vers=4.1,=value");
212+
final TestCmd cmd = new TestCmd();
213+
paramProcessWorkerSpy.processParameters(cmd, params);
214+
}
215+
154216
@Test(expected = ServerApiException.class)
155217
public void processMountOptionsParameter_RejectCodeLikeContent() {
156218
final HashMap<String, String> params = new HashMap<String, String>();

0 commit comments

Comments
 (0)