cloudstack-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CLOUDSTACK-10153) Sanitize string arguments in API requests for XSS scripts
Date Tue, 19 Dec 2017 07:31:00 GMT

    [ https://issues.apache.org/jira/browse/CLOUDSTACK-10153?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16296377#comment-16296377
] 

ASF GitHub Bot commented on CLOUDSTACK-10153:
---------------------------------------------

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("email@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:
users@infra.apache.org


> Sanitize string arguments in API requests for XSS scripts
> ---------------------------------------------------------
>
>                 Key: CLOUDSTACK-10153
>                 URL: https://issues.apache.org/jira/browse/CLOUDSTACK-10153
>             Project: CloudStack
>          Issue Type: Bug
>      Security Level: Public(Anyone can view this level - this is the default.) 
>            Reporter: Rohit Yadav
>            Assignee: Rohit Yadav
>             Fix For: Future
>
>
> API requests made using the CloudStack UI does not allow use of special characters "<"
and ">", however direct API requests made using clis such as cloudmonkey may pass unsanitized
string API arguments to backend. A backend sanitizer can be put in to filter string API argument
and remove possible XSS scripts using libraries such as https://github.com/OWASP/java-html-sanitizer



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Mime
View raw message