From 17fb1462c4382aac6f958ff90148c1ee7691e906 Mon Sep 17 00:00:00 2001 From: Grahame Grieve Date: Tue, 9 Jun 2020 17:19:06 +1000 Subject: [PATCH] fix Attachment check, and add Validator Security checks --- .../src/main/resources/Messages.properties | 4 +- .../hl7/fhir/validation/ValidationEngine.java | 12 ++++++ .../fhir/validation/cli/model/CliContext.java | 14 +++++++ .../cli/services/ValidationService.java | 1 + .../fhir/validation/cli/utils/Display.java | 2 + .../hl7/fhir/validation/cli/utils/Params.java | 3 ++ .../instance/InstanceValidator.java | 38 ++++++++++++++++++- .../validation/tests/ValidationTests.java | 3 ++ 8 files changed, 74 insertions(+), 3 deletions(-) diff --git a/org.hl7.fhir.utilities/src/main/resources/Messages.properties b/org.hl7.fhir.utilities/src/main/resources/Messages.properties index 70092ec8e..05b696893 100644 --- a/org.hl7.fhir.utilities/src/main/resources/Messages.properties +++ b/org.hl7.fhir.utilities/src/main/resources/Messages.properties @@ -492,10 +492,12 @@ TYPE_SPECIFIC_CHECKS_DT_ATT_NO_FETCHER = Attachment size cannot be checked becau TYPE_SPECIFIC_CHECKS_DT_ATT_UNKNOWN_URL_SCHEME = Attachment size cannot be checked because the validator doesn't understand how to access {0} TYPE_SPECIFIC_CHECKS_DT_ATT_URL_ERROR = Attachment size cannot be checked because there was an error accesssing {0}: {1} TYPE_SPECIFIC_CHECKS_DT_ATT_TOO_LONG = Attachment size is {0} bytes which exceeds the stated limit of {1} bytes -TYPE_SPECIFIC_CHECKS_DT_ATT_NO_CONTENT = Attachments have data and/or url, or else must have either contentType and/oor language +TYPE_SPECIFIC_CHECKS_DT_ATT_NO_CONTENT = Attachments have data and/or url, or else SHOULD have either contentType and/or language TYPE_SPECIFIC_CHECKS_DT_BASE64_TOO_LONG = Base64 size is {0} bytes which exceeds the stated limit of {1} bytes TYPE_SPECIFIC_CHECKS_DT_DECIMAL_CHARS = Found {0} decimal places which exceeds the stated limit of {1} digits Validation_VAL_Profile_WrongType = Specified profile type was "{0}", but found type "{1}" Validation_VAL_Profile_WrongType2 = Type mismatch processing profile {0} at path {1}: The element type is {4}, but the profile {3} is for a different type {2} VALIDATION_VAL_ILLEGAL_TYPE_CONSTRAINT = Illegal constraint in profile {0} at path {1} - cannot constrain to type {2} from base types {3} EXTENSION_EXT_CONTEXT_WRONG_XVER = The extension {0} from FHIR version {3} is not allowed to be used at this point (allowed = {1}; this element is [{2}; this is a warning since contexts may be renamed between FHIR versions) +SECURITY_STRING_CONTENT_ERROR = The string value contains embedded HTML tags, which are not allowed for security reasons in this context +SECURITY_STRING_CONTENT_WARNING = The string value contains embedded HTML tags. Note that all inputs should be escaped when rendered to HTML as a matter of course diff --git a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/ValidationEngine.java b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/ValidationEngine.java index 20d8b0a98..e4bea56f7 100644 --- a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/ValidationEngine.java +++ b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/ValidationEngine.java @@ -262,6 +262,7 @@ public class ValidationEngine implements IValidatorResourceFetcher { private IValidatorResourceFetcher fetcher; private boolean assumeValidRestReferences; private boolean noExtensibleBindingMessages; + private boolean securityChecks; private Locale locale; private class AsteriskFilter implements FilenameFilter { @@ -1302,6 +1303,7 @@ public class ValidationEngine implements IValidatorResourceFetcher { validator.setValidationLanguage(language); validator.setAssumeValidRestReferences(assumeValidRestReferences); validator.setNoExtensibleWarnings(noExtensibleBindingMessages); + validator.setSecurityChecks(securityChecks); validator.getContext().setLocale(locale); validator.setFetcher(this); return validator; @@ -1738,6 +1740,16 @@ public class ValidationEngine implements IValidatorResourceFetcher { public void setNoExtensibleBindingMessages(boolean noExtensibleBindingMessages) { this.noExtensibleBindingMessages = noExtensibleBindingMessages; } + + + + public boolean isSecurityChecks() { + return securityChecks; + } + + public void setSecurityChecks(boolean securityChecks) { + this.securityChecks = securityChecks; + } public byte[] transformVersion(String source, String targetVer, FhirFormat format, Boolean canDoNative) throws FHIRException, IOException, Exception { Content cnt = loadContent(source, "validate"); diff --git a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/model/CliContext.java b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/model/CliContext.java index 58f2d0757..c5525cb0d 100644 --- a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/model/CliContext.java +++ b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/model/CliContext.java @@ -63,6 +63,9 @@ public class CliContext { @JsonProperty("mode") private Validator.EngineMode mode = Validator.EngineMode.VALIDATION; + @JsonProperty("securityChecks") + private boolean securityChecks = false; + @JsonProperty("locale") private String locale = Locale.ENGLISH.getDisplayLanguage(); @@ -406,6 +409,17 @@ public class CliContext { return this; } + @JsonProperty("securityChecks") + public boolean isSecurityChecks() { + return securityChecks; + } + + @JsonProperty("securityChecks") + public CliContext setSecurityChecks(boolean securityChecks) { + this.securityChecks = securityChecks; + return this; + } + @Override public boolean equals(Object o) { if (this == o) return true; diff --git a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/services/ValidationService.java b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/services/ValidationService.java index ef8a02e17..7b88bfe02 100644 --- a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/services/ValidationService.java +++ b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/services/ValidationService.java @@ -193,6 +193,7 @@ public class ValidationService { validator.setSnomedExtension(cliContext.getSnomedCTCode()); validator.setAssumeValidRestReferences(cliContext.isAssumeValidRestReferences()); validator.setNoExtensibleBindingMessages(cliContext.isNoExtensibleBindingMessages()); + validator.setSecurityChecks(cliContext.isSecurityChecks()); TerminologyCache.setNoCaching(cliContext.isNoInternalCaching()); return validator; } diff --git a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/utils/Display.java b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/utils/Display.java index fb8b08156..a3ad4b07c 100644 --- a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/utils/Display.java +++ b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/utils/Display.java @@ -98,6 +98,8 @@ public class Display { System.out.println(" marked as mustSupport=true. Useful to identify elements included that may be ignored by recipients"); System.out.println("-assumeValidRestReferences: If present, assume that URLs that reference resources follow the RESTful URI pattern"); System.out.println(" and it is safe to infer the type from the URL"); + System.out.println("-security-checks: If present, check that string content doesn't include any html-like tags that might create"); + System.out.println(" problems downstream (though all external input must always be santized by escaping for either html or sql)"); System.out.println(""); System.out.println("The validator also supports the param -proxy=[address]:[port] for if you use a proxy"); System.out.println(""); diff --git a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/utils/Params.java b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/utils/Params.java index 99516a0cd..7a6b3a840 100644 --- a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/utils/Params.java +++ b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/cli/utils/Params.java @@ -49,6 +49,7 @@ public class Params { public static final String RIGHT = "-right"; public static final String NO_INTERNAL_CACHING = "-no-internal-caching"; public static final String NO_EXTENSIBLE_BINDING_WARNINGS = "-no-extensible-binding-warnings"; + public static final String SECURITY_CHECKS = "-security-checks"; /** * Checks the list of passed in params to see if it contains the passed in param. @@ -151,6 +152,8 @@ public class Params { cliContext.setMode(Validator.EngineMode.NARRATIVE); } else if (args[i].equals(SNAPSHOT)) { cliContext.setMode(Validator.EngineMode.SNAPSHOT); + } else if (args[i].equals(SECURITY_CHECKS)) { + cliContext.setSecurityChecks(true); } else if (args[i].equals(SCAN)) { cliContext.setMode(Validator.EngineMode.SCAN); } else if (args[i].equals(TERMINOLOGY)) { diff --git a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/InstanceValidator.java b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/InstanceValidator.java index 174e0a8d0..f5a2c90bc 100644 --- a/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/InstanceValidator.java +++ b/org.hl7.fhir.validation/src/main/java/org/hl7/fhir/validation/instance/InstanceValidator.java @@ -176,6 +176,7 @@ import com.google.gson.JsonObject; public class InstanceValidator extends BaseValidator implements IResourceValidator { private static final String EXECUTED_CONSTRAINT_LIST = "validator.executed.invariant.list"; private static final String EXECUTION_ID = "validator.execution.id"; + private static final String HTML_FRAGMENT_REGEX = "[a-zA-Z]\\w*(((\\s+)(\\S)*)*)"; private class ValidatorHostServices implements IEvaluationContext { @@ -352,6 +353,7 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat private ValidatorHostServices validatorServices; private boolean assumeValidRestReferences; private boolean allowExamples; + private boolean securityChecks; private ProfileUtilities profileUtilities; public InstanceValidator(IWorkerContext theContext, IEvaluationContext hostServices) { @@ -1773,8 +1775,16 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat return; } String regex = context.getExtensionString(ToolingExtensions.EXT_REGEX); - if (regex != null) + if (regex != null) { rule(errors, IssueType.INVALID, e.line(), e.col(), path, e.primitiveValue().matches(regex), I18nConstants.TYPE_SPECIFIC_CHECKS_DT_PRIMITIVE_REGEX, e.primitiveValue(), regex); + } + if (!"xhtml".equals(type)) { + if (securityChecks) { + rule(errors, IssueType.INVALID, e.line(), e.col(), path, !containsHtmlTags(e.primitiveValue()), I18nConstants.SECURITY_STRING_CONTENT_ERROR); + } else { + hint(errors, IssueType.INVALID, e.line(), e.col(), path, !containsHtmlTags(e.primitiveValue()), I18nConstants.SECURITY_STRING_CONTENT_WARNING); + } + } if (type.equals("boolean")) { @@ -1961,6 +1971,22 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat // for nothing to check } + private boolean containsHtmlTags(String cnt) { + int i = cnt.indexOf("<"); + while (i > -1) { + cnt = cnt.substring(i+1); + i = cnt.indexOf("<"); + int e = cnt.indexOf(">"); + if (e > -1 && e < i) { + String s = cnt.substring(0, e); + if (s.matches(HTML_FRAGMENT_REGEX)) { + return true; + } + } + } + return false; + } + /** * Technically this is not bulletproof as some invalid base64 won't be caught, * but I think it's good enough. The original code used Java8 Base64 decoder @@ -2207,7 +2233,7 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat rule(errors, IssueType.STRUCTURE, element.line(), element.col(), path, size <= def, I18nConstants.TYPE_SPECIFIC_CHECKS_DT_ATT_TOO_LONG, size, def); } } - rule(errors, IssueType.STRUCTURE, element.line(), element.col(), path, (element.hasChild("data") || element.hasChild("url")) || (element.hasChild("contentType") || element.hasChild("language")), + warning(errors, IssueType.STRUCTURE, element.line(), element.col(), path, (element.hasChild("data") || element.hasChild("url")) || (element.hasChild("contentType") || element.hasChild("language")), I18nConstants.TYPE_SPECIFIC_CHECKS_DT_ATT_NO_CONTENT); } @@ -4575,4 +4601,12 @@ public class InstanceValidator extends BaseValidator implements IResourceValidat } } + public boolean isSecurityChecks() { + return securityChecks; + } + + public void setSecurityChecks(boolean securityChecks) { + this.securityChecks = securityChecks; + } + } \ No newline at end of file diff --git a/org.hl7.fhir.validation/src/test/java/org/hl7/fhir/validation/tests/ValidationTests.java b/org.hl7.fhir.validation/src/test/java/org/hl7/fhir/validation/tests/ValidationTests.java index 1abc81964..7371a687d 100644 --- a/org.hl7.fhir.validation/src/test/java/org/hl7/fhir/validation/tests/ValidationTests.java +++ b/org.hl7.fhir.validation/src/test/java/org/hl7/fhir/validation/tests/ValidationTests.java @@ -185,6 +185,9 @@ public class ValidationTests implements IEvaluationContext, IValidatorResourceFe } else { val.setAllowExamples(true); } + if (content.has("security-checks")) { + val.setSecurityChecks(content.get("security-checks").getAsBoolean()); + } val.setAssumeValidRestReferences(content.has("assumeValidRestReferences") ? content.get("assumeValidRestReferences").getAsBoolean() : false); System.out.println(String.format("Start Validating (%d to set up)", (System.nanoTime() - setup) / 1000000)); if (name.endsWith(".json"))