cloudstack-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] rhtyd closed pull request #2336: CLOUDSTACK-10153: Introduce string API arg trust validation
Date Thu, 01 Jan 1970 00:00:00 GMT
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


With regards,
Apache Git Services

Mime
View raw message