rhtyd closed pull request #2336: CLOUDSTACK-10153: Introduce string API arg
trust validation
URL: https://github.com/apache/cloudstack/pull/2336
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/api/src/org/apache/cloudstack/api/ApiArgValidator.java
b/api/src/org/apache/cloudstack/api/ApiArgValidator.java
index bd2294c6018..9454a196f13 100644
--- a/api/src/org/apache/cloudstack/api/ApiArgValidator.java
+++ b/api/src/org/apache/cloudstack/api/ApiArgValidator.java
@@ -20,4 +20,5 @@
public enum ApiArgValidator {
NotNullOrEmpty, // does Strings.isNullOrEmpty check
PositiveNumber, // does != null and > 0 check
+ SkipSanitization, // does no HTML sanitization checks on string fields
}
diff --git
a/api/src/org/apache/cloudstack/api/command/admin/ca/IssueCertificateCmd.java
b/api/src/org/apache/cloudstack/api/command/admin/ca/IssueCertificateCmd.java
index 8926829205f..1c8af4e19e6 100644
---
a/api/src/org/apache/cloudstack/api/command/admin/ca/IssueCertificateCmd.java
+++
b/api/src/org/apache/cloudstack/api/command/admin/ca/IssueCertificateCmd.java
@@ -25,6 +25,7 @@
import org.apache.cloudstack.acl.RoleType;
import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiArgValidator;
import org.apache.cloudstack.api.ApiConstants;
import org.apache.cloudstack.api.ApiErrorCode;
import org.apache.cloudstack.api.BaseAsyncCmd;
@@ -60,7 +61,7 @@
//////////////// API parameters /////////////////////
/////////////////////////////////////////////////////
- @Parameter(name = ApiConstants.CSR, type = BaseCmd.CommandType.STRING,
description = "The certificate signing request (in pem format), if CSR is not
provided then configured/provided options are considered", length = 65535)
+ @Parameter(name = ApiConstants.CSR, type = BaseCmd.CommandType.STRING,
description = "The certificate signing request (in pem format), if CSR is not
provided then configured/provided options are considered", validations =
{ApiArgValidator.SkipSanitization}, length = 65535)
private String csr;
@Parameter(name = ApiConstants.DOMAIN, type = BaseCmd.CommandType.STRING,
description = "Comma separated list of domains, the certificate should be
issued for. When csr is not provided, the first domain is used as a subject/CN")
diff --git
a/api/src/org/apache/cloudstack/api/command/admin/resource/UploadCustomCertificateCmd.java
b/api/src/org/apache/cloudstack/api/command/admin/resource/UploadCustomCertificateCmd.java
index e8d6cc55d6d..814e3ab2d9e 100644
---
a/api/src/org/apache/cloudstack/api/command/admin/resource/UploadCustomCertificateCmd.java
+++
b/api/src/org/apache/cloudstack/api/command/admin/resource/UploadCustomCertificateCmd.java
@@ -16,6 +16,7 @@
// under the License.
package org.apache.cloudstack.api.command.admin.resource;
+import org.apache.cloudstack.api.ApiArgValidator;
import org.apache.log4j.Logger;
import org.apache.cloudstack.api.APICommand;
@@ -38,7 +39,7 @@
private static final String s_name = "uploadcustomcertificateresponse";
- @Parameter(name = ApiConstants.CERTIFICATE, type = CommandType.STRING,
required = true, description = "The certificate to be uploaded.", length =
65535)
+ @Parameter(name = ApiConstants.CERTIFICATE, type = CommandType.STRING,
required = true, description = "The certificate to be uploaded.", validations =
{ApiArgValidator.SkipSanitization}, length = 65535)
private String certificate;
@Parameter(name = ApiConstants.ID,
diff --git
a/api/src/org/apache/cloudstack/api/command/user/loadbalancer/UploadSslCertCmd.java
b/api/src/org/apache/cloudstack/api/command/user/loadbalancer/UploadSslCertCmd.java
index 309e43fcbbe..6914252da0b 100644
---
a/api/src/org/apache/cloudstack/api/command/user/loadbalancer/UploadSslCertCmd.java
+++
b/api/src/org/apache/cloudstack/api/command/user/loadbalancer/UploadSslCertCmd.java
@@ -18,9 +18,8 @@
import javax.inject.Inject;
-import org.apache.log4j.Logger;
-
import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiArgValidator;
import org.apache.cloudstack.api.ApiConstants;
import org.apache.cloudstack.api.ApiErrorCode;
import org.apache.cloudstack.api.BaseCmd;
@@ -30,13 +29,14 @@
import org.apache.cloudstack.api.response.ProjectResponse;
import org.apache.cloudstack.api.response.SslCertResponse;
import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.network.tls.CertService;
+import org.apache.log4j.Logger;
import com.cloud.exception.ConcurrentOperationException;
import com.cloud.exception.InsufficientCapacityException;
import com.cloud.exception.NetworkRuleConflictException;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.exception.ResourceUnavailableException;
-import org.apache.cloudstack.network.tls.CertService;
@APICommand(name = "uploadSslCert", description = "Upload a certificate to
CloudStack", responseObject = SslCertResponse.class,
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
@@ -52,13 +52,13 @@
//////////////// API parameters /////////////////////
/////////////////////////////////////////////////////
- @Parameter(name = ApiConstants.CERTIFICATE, type = CommandType.STRING,
required = true, description = "SSL certificate", length = 16384)
+ @Parameter(name = ApiConstants.CERTIFICATE, type = CommandType.STRING,
required = true, description = "SSL certificate", validations =
{ApiArgValidator.SkipSanitization}, length = 16384)
private String cert;
- @Parameter(name = ApiConstants.PRIVATE_KEY, type = CommandType.STRING,
required = true, description = "Private key", length = 16384)
+ @Parameter(name = ApiConstants.PRIVATE_KEY, type = CommandType.STRING,
required = true, description = "Private key", validations =
{ApiArgValidator.SkipSanitization}, length = 16384)
private String key;
- @Parameter(name = ApiConstants.CERTIFICATE_CHAIN, type =
CommandType.STRING, description = "Certificate chain of trust", length =
2097152)
+ @Parameter(name = ApiConstants.CERTIFICATE_CHAIN, type =
CommandType.STRING, description = "Certificate chain of trust", validations =
{ApiArgValidator.SkipSanitization}, length = 2097152)
private String chain;
@Parameter(name = ApiConstants.PASSWORD, type = CommandType.STRING,
description = "Password for the private key")
diff --git
a/api/src/org/apache/cloudstack/api/command/user/ssh/RegisterSSHKeyPairCmd.java
b/api/src/org/apache/cloudstack/api/command/user/ssh/RegisterSSHKeyPairCmd.java
index ed9c4cd304e..a82cb0ea829 100644
---
a/api/src/org/apache/cloudstack/api/command/user/ssh/RegisterSSHKeyPairCmd.java
+++
b/api/src/org/apache/cloudstack/api/command/user/ssh/RegisterSSHKeyPairCmd.java
@@ -16,6 +16,7 @@
// under the License.
package org.apache.cloudstack.api.command.user.ssh;
+import org.apache.cloudstack.api.ApiArgValidator;
import org.apache.log4j.Logger;
import org.apache.cloudstack.api.APICommand;
@@ -42,7 +43,7 @@
@Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required =
true, description = "Name of the keypair")
private String name;
- @Parameter(name = "publickey", type = CommandType.STRING, required = true,
description = "Public key material of the keypair", length = 5120)
+ @Parameter(name = "publickey", type = CommandType.STRING, required = true,
description = "Public key material of the keypair", validations =
{ApiArgValidator.SkipSanitization}, length = 5120)
private String publicKey;
//Owner information
diff --git
a/plugins/database/quota/src/org/apache/cloudstack/api/command/QuotaEmailTemplateUpdateCmd.java
b/plugins/database/quota/src/org/apache/cloudstack/api/command/QuotaEmailTemplateUpdateCmd.java
index a47a7832e8c..0dfddeaf101 100644
---
a/plugins/database/quota/src/org/apache/cloudstack/api/command/QuotaEmailTemplateUpdateCmd.java
+++
b/plugins/database/quota/src/org/apache/cloudstack/api/command/QuotaEmailTemplateUpdateCmd.java
@@ -18,6 +18,7 @@
import com.cloud.user.Account;
import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiArgValidator;
import org.apache.cloudstack.api.ApiErrorCode;
import org.apache.cloudstack.api.BaseCmd;
import org.apache.cloudstack.api.Parameter;
@@ -41,10 +42,10 @@
@Parameter(name = "templatetype", type = CommandType.STRING,
required=true, description = "Type of the quota email template, allowed types:
QUOTA_LOW, QUOTA_EMPTY")
private String templateName;
- @Parameter(name = "templatesubject", type = CommandType.STRING,
required=true, description = "The quota email template subject, max: 77
characters", length = 77)
+ @Parameter(name = "templatesubject", type = CommandType.STRING,
required=true, description = "The quota email template subject, max: 77
characters", validations = {ApiArgValidator.SkipSanitization}, length = 77)
private String templateSubject;
- @Parameter(name = "templatebody", type = CommandType.STRING,
required=true, description = "The quota email template body, max: 500k
characters", length = 512000)
+ @Parameter(name = "templatebody", type = CommandType.STRING,
required=true, description = "The quota email template body, max: 500k
characters", validations = {ApiArgValidator.SkipSanitization}, length = 512000)
private String templateBody;
@Parameter(name = "locale", type = CommandType.STRING, description = "The
locale of the email text")
diff --git a/pom.xml b/pom.xml
index caa26cdff70..b938ca2e33f 100644
--- a/pom.xml
+++ b/pom.xml
@@ -113,6 +113,7 @@
<cs.findbugs.version>3.0.3</cs.findbugs.version>
<cs.javadoc.version>2.10.3</cs.javadoc.version>
<cs.opensaml.version>2.6.4</cs.opensaml.version>
+ <cs.jsoup.version>1.11.2</cs.jsoup.version>
<cs.xml-apis.version>1.4.01</cs.xml-apis.version>
<cs.joda-time.version>2.8.1</cs.joda-time.version>
<cs.batik.version>1.9.1</cs.batik.version>
diff --git a/server/pom.xml b/server/pom.xml
index 4067b8590a0..cced38c8fac 100644
--- a/server/pom.xml
+++ b/server/pom.xml
@@ -141,6 +141,11 @@
<artifactId>opensaml</artifactId>
<version>${cs.opensaml.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.jsoup</groupId>
+ <artifactId>jsoup</artifactId>
+ <version>${cs.jsoup.version}</version>
+ </dependency>
</dependencies>
<build>
<testResources>
diff --git a/server/src/com/cloud/api/dispatch/ParamProcessWorker.java
b/server/src/com/cloud/api/dispatch/ParamProcessWorker.java
index feefaabc551..e5ef7986bee 100644
--- a/server/src/com/cloud/api/dispatch/ParamProcessWorker.java
+++ b/server/src/com/cloud/api/dispatch/ParamProcessWorker.java
@@ -22,6 +22,7 @@
import java.lang.reflect.Field;
import java.text.DateFormat;
import java.text.ParseException;
+import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.Date;
@@ -30,18 +31,15 @@
import java.util.Map;
import java.util.StringTokenizer;
import java.util.regex.Matcher;
-import java.text.SimpleDateFormat;
import javax.inject.Inject;
-import com.google.common.base.Strings;
-import org.apache.log4j.Logger;
-
import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.acl.InfrastructureEntity;
import org.apache.cloudstack.acl.SecurityChecker;
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
import org.apache.cloudstack.api.ACL;
+import org.apache.cloudstack.api.ApiArgValidator;
import org.apache.cloudstack.api.ApiErrorCode;
import org.apache.cloudstack.api.BaseAsyncCreateCmd;
import org.apache.cloudstack.api.BaseCmd;
@@ -50,7 +48,6 @@
import org.apache.cloudstack.api.InternalIdentity;
import org.apache.cloudstack.api.Parameter;
import org.apache.cloudstack.api.ServerApiException;
-import org.apache.cloudstack.api.ApiArgValidator;
import org.apache.cloudstack.api.command.admin.resource.ArchiveAlertsCmd;
import org.apache.cloudstack.api.command.admin.resource.DeleteAlertsCmd;
import org.apache.cloudstack.api.command.admin.usage.GetUsageRecordsCmd;
@@ -58,6 +55,9 @@
import org.apache.cloudstack.api.command.user.event.DeleteEventsCmd;
import org.apache.cloudstack.api.command.user.event.ListEventsCmd;
import org.apache.cloudstack.context.CallContext;
+import org.apache.log4j.Logger;
+import org.jsoup.Jsoup;
+import org.jsoup.safety.Whitelist;
import com.cloud.exception.InvalidParameterValueException;
import com.cloud.user.Account;
@@ -65,6 +65,7 @@
import com.cloud.utils.DateUtil;
import com.cloud.utils.db.EntityManager;
import com.cloud.utils.exception.CloudRuntimeException;
+import com.google.common.base.Strings;
public class ParamProcessWorker implements DispatchWorker {
@@ -112,10 +113,22 @@ private void validateNaturalNumber(final Object param,
final String argName) {
}
}
+ protected void validateUntrustedString(final String unsafe, final String
argName) {
+ if (Strings.isNullOrEmpty(unsafe)) {
+ return;
+ }
+
+ final String safe = Jsoup.clean(unsafe, Whitelist.basic());
+ if (!unsafe.trim().equals(safe)) {
+ throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
String.format("The requested API argument '%s' contains unsafe (html) string
that is not allowed", argName));
+ }
+ }
+
private void validateField(final Object paramObj, final Parameter
annotation) throws ServerApiException {
if (annotation == null) {
return;
}
+ boolean needSanitization = true;
final String argName = annotation.name();
for (final ApiArgValidator validator : annotation.validations()) {
if (validator == null) {
@@ -139,8 +152,14 @@ private void validateField(final Object paramObj, final
Parameter annotation) th
break;
}
break;
+ case SkipSanitization:
+ needSanitization = false;
+ break;
}
}
+ if (needSanitization && annotation.type() == CommandType.STRING) {
+ validateUntrustedString(paramObj.toString(), argName);
+ }
}
@SuppressWarnings({"unchecked", "rawtypes"})
diff --git a/server/test/com/cloud/api/dispatch/ParamProcessWorkerTest.java
b/server/test/com/cloud/api/dispatch/ParamProcessWorkerTest.java
index 7ac982db623..2a9b5cfd8cb 100644
--- a/server/test/com/cloud/api/dispatch/ParamProcessWorkerTest.java
+++ b/server/test/com/cloud/api/dispatch/ParamProcessWorkerTest.java
@@ -109,4 +109,32 @@ public void processParameters() {
Assert.assertTrue(Double.compare(cmd.doubleparam1, 11.89) == 0);
}
+ @Test
+ public void validateUntrustedHtmlValid() {
+
paramProcessWorker.validateUntrustedString("cd360613-e1cf-3a32-b8ef-d07bad8dd92b",
"uuid");
+ paramProcessWorker.validateUntrustedString("1000", "number");
+ paramProcessWorker.validateUntrustedString("123.456", "double");
+ paramProcessWorker.validateUntrustedString("somestringwithnospaces",
"string");
+ paramProcessWorker.validateUntrustedString("some argument with
spaces", "arg");
+ paramProcessWorker.validateUntrustedString("some argument with space
at end ", "arg");
+ paramProcessWorker.validateUntrustedString(" some argument with spaces
at beginning", "arg");
+ paramProcessWorker.validateUntrustedString(" some argument with
spaces on both ends ", "arg");
+ paramProcessWorker.validateUntrustedString("em...@server.com",
"email");
+ paramProcessWorker.validateUntrustedString("????? ????", "unicode");
+ paramProcessWorker.validateUntrustedString("map[0].value=123", "map");
+ paramProcessWorker.validateUntrustedString("one,two,three,four",
"list");
+ paramProcessWorker.validateUntrustedString(null, "null");
+ paramProcessWorker.validateUntrustedString("", "empty");
+ }
+
+ @Test(expected = ServerApiException.class)
+ public void validateUntrustedHtmlFailTemplate() {
+ paramProcessWorker.validateUntrustedString("<div><h1>Title</h1><p>Some
paragraph <a href='https://cloudstack.apache.org'>click here!</a></p></div>",
"template");
+ }
+
+ @Test(expected = ServerApiException.class)
+ public void validateUntrustedHtmlFailXSS() {
+
paramProcessWorker.validateUntrustedString("<script>alert(123);</script>",
"xss-alert");
+ }
+
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
