From 6556d08511e0e37f398cc5e82f82de3a7e6e61f8 Mon Sep 17 00:00:00 2001 From: Jeremiah Stacey Date: Sat, 2 Feb 2019 08:04:09 -0600 Subject: [PATCH 1/2] DefaultValidator.*ValidInput uses canonicalize arg Exposing behavior in the StringValidationRule to allow an implementor to opt-out of encoder canonicalization behavior. Tests added to verify intended behavior. DefaultValidator getValidInput has been updated to use the existing canonicalize argument to configure the StringValidationRule delegate for canonicalization behavior before validating the input reference. Tests updated to validate configuration occurs in workflow. Additional test updates in the DefaultValidatorInputStringAPITest to remove unnecessary mock configuration expectations, and to verify that all expected mock interactions have occurred. --- .../esapi/reference/DefaultValidator.java | 1 + .../validation/StringValidationRule.java | 8 ++- .../DefaultValidatorInputStringAPITest.java | 57 +++++++++++++++++-- .../validation/StringValidationRuleTest.java | 22 +++++++ 4 files changed, 82 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/owasp/esapi/reference/DefaultValidator.java b/src/main/java/org/owasp/esapi/reference/DefaultValidator.java index 1a05534c0..abe6c93a5 100644 --- a/src/main/java/org/owasp/esapi/reference/DefaultValidator.java +++ b/src/main/java/org/owasp/esapi/reference/DefaultValidator.java @@ -216,6 +216,7 @@ public String getValidInput(String context, String input, String type, int maxLe } rvr.setMaximumLength(maxLength); rvr.setAllowNull(allowNull); + rvr.setCanonicalize(canonicalize); return rvr.getValid(context, input); } diff --git a/src/main/java/org/owasp/esapi/reference/validation/StringValidationRule.java b/src/main/java/org/owasp/esapi/reference/validation/StringValidationRule.java index 3163c532d..633588e8b 100644 --- a/src/main/java/org/owasp/esapi/reference/validation/StringValidationRule.java +++ b/src/main/java/org/owasp/esapi/reference/validation/StringValidationRule.java @@ -44,6 +44,7 @@ public class StringValidationRule extends BaseValidationRule { protected List blacklistPatterns = new ArrayList(); protected int minLength = 0; protected int maxLength = Integer.MAX_VALUE; + private boolean canonicalizeInput = true; public StringValidationRule( String typeName ) { super( typeName ); @@ -115,6 +116,10 @@ public void setMaximumLength( int length ) { maxLength = length; } + public void setCanonicalize(boolean canonicalize) { + this.canonicalizeInput = canonicalize; + } + /** * checks input against whitelists. * @param context The context to include in exception messages @@ -264,7 +269,7 @@ public String getValid( String context, String input ) throws ValidationExceptio checkLength(context, input); // canonicalize - data = encoder.canonicalize( input ); + data = canonicalizeInput ? encoder.canonicalize( input ) : input; // check whitelist patterns checkWhitelist(context, input); @@ -284,5 +289,6 @@ public String sanitize( String context, String input ) { return whitelist( input, EncoderConstants.CHAR_ALPHANUMERICS ); } + } diff --git a/src/test/java/org/owasp/esapi/reference/DefaultValidatorInputStringAPITest.java b/src/test/java/org/owasp/esapi/reference/DefaultValidatorInputStringAPITest.java index 525584b6a..99e25c493 100644 --- a/src/test/java/org/owasp/esapi/reference/DefaultValidatorInputStringAPITest.java +++ b/src/test/java/org/owasp/esapi/reference/DefaultValidatorInputStringAPITest.java @@ -17,6 +17,7 @@ import java.util.regex.Pattern; import org.hamcrest.core.Is; +import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -66,10 +67,6 @@ public class DefaultValidatorInputStringAPITest { @Before public void setup() throws Exception { - //Is intrusion detection disabled? A: Yes, it is off. - //This logic is confusing: True, the value is False... - when(mockSecConfig.getDisableIntrusionDetection()).thenReturn(true); - contextStr = testName.getMethodName(); testValidatorType = testName.getMethodName() + "_validator_type"; validatorResultString = testName.getMethodName() + "_validator_result"; @@ -101,6 +98,13 @@ public void setup() throws Exception { } + @After + public void verifyDelegateCalls() { + verify(mockSecConfig, times(1)).getValidationPattern(testValidatorType); + + PowerMockito.verifyNoMoreInteractions(spyStringRule, mockSecConfig, mockEncoder); + } + @Test public void getValidInputNullAllowedPassthrough() throws Exception { String safeValue = uit.getValidInput(contextStr, testName.getMethodName(), testValidatorType, testMaximumLength, true); @@ -109,6 +113,7 @@ public void getValidInputNullAllowedPassthrough() throws Exception { verify(spyStringRule, times(1)).setAllowNull(true); verify(spyStringRule, times(0)).setAllowNull(false); verify(spyStringRule, times(1)).setMaximumLength(testMaximumLength); + verify(spyStringRule, times(1)).setCanonicalize(true); verify(spyStringRule, times(1)).getValid(contextStr, testName.getMethodName()); } @@ -120,6 +125,7 @@ public void getValidInputNullNotAllowedPassthrough() throws Exception { verify(spyStringRule, times(0)).setAllowNull(true); verify(spyStringRule, times(1)).setAllowNull(false); verify(spyStringRule, times(1)).setMaximumLength(testMaximumLength); + verify(spyStringRule, times(1)).setCanonicalize(true); verify(spyStringRule, times(1)).getValid(contextStr, testName.getMethodName()); } @@ -137,7 +143,16 @@ public void getValidInputValidationExceptionPropagates() throws Exception { exEx.expect(Is.is(validationEx)); doThrow(validationEx).when(spyStringRule).getValid(ArgumentMatchers.anyString(), ArgumentMatchers.anyString()); - uit.getValidInput(contextStr, testName.getMethodName(), testValidatorType, testMaximumLength, true); + try { + uit.getValidInput(contextStr, testName.getMethodName(), testValidatorType, testMaximumLength, true); + } finally { + verify(spyStringRule, times(1)).addWhitelistPattern(TEST_PATTERN); + verify(spyStringRule, times(1)).setAllowNull(true); + verify(spyStringRule, times(0)).setAllowNull(false); + verify(spyStringRule, times(1)).setMaximumLength(testMaximumLength); + verify(spyStringRule, times(1)).setCanonicalize(true); + verify(spyStringRule, times(1)).getValid(contextStr, testName.getMethodName()); + } } @Test @@ -149,6 +164,12 @@ public void getValidInputValidationExceptionErrorList() throws Exception { assertTrue(result.isEmpty()); assertEquals(1, errorList.size()); assertEquals(validationEx, errorList.getError(contextStr)); + verify(spyStringRule, times(1)).addWhitelistPattern(TEST_PATTERN); + verify(spyStringRule, times(1)).setAllowNull(true); + verify(spyStringRule, times(0)).setAllowNull(false); + verify(spyStringRule, times(1)).setMaximumLength(testMaximumLength); + verify(spyStringRule, times(1)).setCanonicalize(true); + verify(spyStringRule, times(1)).getValid(contextStr, testName.getMethodName()); } @@ -161,6 +182,7 @@ public void isValidInputNullAllowedPassthrough() throws Exception { verify(spyStringRule, times(1)).setAllowNull(true); verify(spyStringRule, times(0)).setAllowNull(false); verify(spyStringRule, times(1)).setMaximumLength(testMaximumLength); + verify(spyStringRule, times(1)).setCanonicalize(true); verify(spyStringRule, times(1)).getValid(contextStr, testName.getMethodName()); } @@ -169,6 +191,12 @@ public void isValidInputValidationExceptionReturnsFalse() throws Exception { doThrow(validationEx).when(spyStringRule).getValid(ArgumentMatchers.anyString(), ArgumentMatchers.anyString()); boolean result = uit.isValidInput(contextStr, testName.getMethodName(), testValidatorType, testMaximumLength, true); assertFalse(result); + verify(spyStringRule, times(1)).addWhitelistPattern(TEST_PATTERN); + verify(spyStringRule, times(1)).setAllowNull(true); + verify(spyStringRule, times(0)).setAllowNull(false); + verify(spyStringRule, times(1)).setMaximumLength(testMaximumLength); + verify(spyStringRule, times(1)).setCanonicalize(true); + verify(spyStringRule, times(1)).getValid(contextStr, testName.getMethodName()); } @Test @@ -180,5 +208,24 @@ public void isValidInputValidationExceptionErrorListReturnsFalse() throws Except assertFalse(result); assertEquals(1, errorList.size()); assertEquals(validationEx, errorList.getError(contextStr)); + verify(errors, times(1)).addError(contextStr, validationEx); + verify(spyStringRule, times(1)).addWhitelistPattern(TEST_PATTERN); + verify(spyStringRule, times(1)).setAllowNull(true); + verify(spyStringRule, times(0)).setAllowNull(false); + verify(spyStringRule, times(1)).setMaximumLength(testMaximumLength); + verify(spyStringRule, times(1)).setCanonicalize(true); + verify(spyStringRule, times(1)).getValid(contextStr, testName.getMethodName()); + } + + @Test + public void canonicalizeSettingPassedThrough() throws Exception { + String safeValue = uit.getValidInput(contextStr, testName.getMethodName(), testValidatorType, testMaximumLength, false,false); + assertEquals(validatorResultString, safeValue); + verify(spyStringRule, times(1)).addWhitelistPattern(TEST_PATTERN); + verify(spyStringRule, times(0)).setAllowNull(true); + verify(spyStringRule, times(1)).setAllowNull(false); + verify(spyStringRule, times(1)).setMaximumLength(testMaximumLength); + verify(spyStringRule, times(1)).setCanonicalize(false); + verify(spyStringRule, times(1)).getValid(contextStr, testName.getMethodName()); } } diff --git a/src/test/java/org/owasp/esapi/reference/validation/StringValidationRuleTest.java b/src/test/java/org/owasp/esapi/reference/validation/StringValidationRuleTest.java index d580d4cc7..e172e2ebb 100644 --- a/src/test/java/org/owasp/esapi/reference/validation/StringValidationRuleTest.java +++ b/src/test/java/org/owasp/esapi/reference/validation/StringValidationRuleTest.java @@ -3,6 +3,9 @@ import junit.framework.Assert; import org.junit.Test; +import org.mockito.ArgumentMatchers; +import org.mockito.Mockito; +import org.owasp.esapi.Encoder; import org.owasp.esapi.ValidationErrorList; import org.owasp.esapi.errors.ValidationException; @@ -155,4 +158,23 @@ public void testAllowNull() throws ValidationException { Assert.assertTrue(validationRule.isValid("", null)); } + @Test + public void testSetCanonicalize() throws ValidationException { + String context = "test-scope"; + String inputString = "SomeInputValue"; + String encoderReturn = "MockReturnValue"; + Encoder mockEncoder = Mockito.mock(Encoder.class); + Mockito.when(mockEncoder.canonicalize(inputString)).thenReturn(encoderReturn); + StringValidationRule testRule = new StringValidationRule(context, mockEncoder); + String valid = testRule.getValid(context, inputString); + Assert.assertEquals(encoderReturn, valid); + Mockito.verify(mockEncoder, Mockito.times(1)).canonicalize(inputString); + Mockito.reset(mockEncoder); + + testRule.setCanonicalize(false); + valid = testRule.getValid(context, inputString); + Assert.assertEquals(inputString, valid); + Mockito.verify(mockEncoder, Mockito.times(0)).canonicalize(inputString); + } + } From 6361b9e7a20202af0b41c8f157f339dc72fb1b5c Mon Sep 17 00:00:00 2001 From: Jeremiah Stacey Date: Sat, 2 Feb 2019 08:50:33 -0600 Subject: [PATCH 2/2] Logging in StringValidationRule Adding recommended logging if canonicalization is set to false. --- .../reference/validation/StringValidationRule.java | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/owasp/esapi/reference/validation/StringValidationRule.java b/src/main/java/org/owasp/esapi/reference/validation/StringValidationRule.java index 633588e8b..8d0c88cf0 100644 --- a/src/main/java/org/owasp/esapi/reference/validation/StringValidationRule.java +++ b/src/main/java/org/owasp/esapi/reference/validation/StringValidationRule.java @@ -20,8 +20,10 @@ import java.util.regex.Pattern; import java.util.regex.PatternSyntaxException; +import org.owasp.esapi.ESAPI; import org.owasp.esapi.Encoder; import org.owasp.esapi.EncoderConstants; +import org.owasp.esapi.Logger; import org.owasp.esapi.StringUtilities; import org.owasp.esapi.errors.ValidationException; import org.owasp.esapi.util.NullSafe; @@ -39,7 +41,7 @@ * http://en.wikipedia.org/wiki/Whitelist */ public class StringValidationRule extends BaseValidationRule { - + private static final Logger LOGGER = ESAPI.getLogger(StringValidationRule.class); protected List whitelistPatterns = new ArrayList(); protected List blacklistPatterns = new ArrayList(); protected int minLength = 0; @@ -267,9 +269,15 @@ public String getValid( String context, String input ) throws ValidationExceptio // check length checkLength(context, input); - + // canonicalize - data = canonicalizeInput ? encoder.canonicalize( input ) : input; + if (canonicalizeInput) { + data = encoder.canonicalize(input); + } else { + String message = String.format("Input validaiton excludes canonicalization. Context: %s Input: %s", context, input); + LOGGER.warning(Logger.SECURITY_AUDIT, message); + data = input; + } // check whitelist patterns checkWhitelist(context, input);