search

ESAPI / esapi-java-legacy

ESAPI (The OWASP Enterprise Security API) is a free, open source, web application security control library that makes it easier for programmers to write lower-risk applications.
https://owasp.org/www-project-enterprise-security-api/
Other
614 stars 367 forks source link

JUnit test ValidatorTest.testIsValidSafeHTML() now failing #521

Closed kwwall closed 4 years ago

kwwall commented 5 years ago

Reproduce by using JDK 8 and run the following commands:

$ rm -fr ./esapi-java-legacy
$ git clone https://github.com/ESAPI/esapi-java-legacy.git
$ cd ./esapi-java-legacy
$ mvn compile
$ mvn -Dtest=org.owasp.esapi.reference.ValidatorTest#testIsValidSafeHTML test

which will result in a failure at line 890 in ValidatorTest.java, which is this line: assertTrue(instance.isValidSafeHTML("test", "Test. <script>alert(document.cookie)</script>", 100, false)); The details of the failure, including what commit broke this test may be found here.

The "fix" for this MUST NOT simply consist of changing this test. As part of commit [d869303](https://github.com/ESAPI/esapi-java-legacy/commit d869303/d86930362d703c790a18453882db1541c23f5c16), we have broken previously existing behavior, and even if that behavior was incorrect (and that could be argued since the behavior did not previously follow the documented Javadoc).

So we have two options: 1) roll back to the previous (and presumably incorrect?) behavior and "fix" by changing the Javadoc to agree with that previous behavior, 2) keep the current new (correct?) behavior as the default, but add some property to the ESAPI.properties that will allow ESAPI users to revert back to the prior behavior, 3) leave everything as-is and just announce this in the next release notes. I personally am adamantly against the third approach because few, if any read the release notes.

Before any "fixes" I would like a debate on what the correct / best approach should be.

Note: @davewichers apparently has not seen this under CentOS 7 or 8 (or whatever version he is running). I'm hoping he can confirm that.

-kevin

davewichers commented 5 years ago

I'll be back at work on Weds and I can retest then to verify behavior for this test on CentOS 8.

On Sun, Oct 6, 2019 at 11:36 PM Kevin W. Wall notifications@github.com wrote:

Reproduce by using JDK 8 and run the following commands:

$ rm -fr ./esapi-java-legacy $ git clone https://github.com/ESAPI/esapi-java-legacy.git $ cd ./esapi-java-legacy $ mvn compile $ mvn -Dtest=org.owasp.esapi.reference.ValidatorTest#testIsValidSafeHTML test

which will result in a failure at line 890 in ValidatorTest.java, which is this line: assertTrue(instance.isValidSafeHTML("test", "Test.

", 100, false)); The details of the failure, including what commit broke this test may be found here . The "fix" for this *MUST NOT* *simply* consist of changing this test. As part of commit [d869303 ](https://github.com/ESAPI/esapi-java-legacy/commit d869303 /d86930362d703c790a18453882db1541c23f5c16), we have broken previously existing behavior, and even if that behavior was incorrect (and that could be argued since the behavior did not previously follow the documented Javadoc). So we have two options: 1) roll back to the previous (and presumably incorrect?) behavior and "fix" by changing the Javadoc to agree with that previous behavior, 2) keep the current new (correct?) behavior as the default, but add some property to the ESAPI.properties that will allow ESAPI users to revert back to the prior behavior, 3) leave everything as-is and just announce this in the next release notes. I personally am adamantly against the third approach because few, if any read the release notes. Before any "fixes" I would like a debate on what the correct / best approach should be. Note: @davewichers apparently has not seen this under CentOS 7 or 8 (or whatever version he is running). I'm hoping he can confirm that. -kevin — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub , or mute the thread .
jeremiahjstacey commented 5 years ago

I'm seeing that there are 2 test in the ValidatorTest related to changes in the HtmlValidationRule that are now failing.

testIsValidSafeHTML
testGetValidSafeHTML

Between these tests there are 5 Strings that now cause validation failures. They are repeated multiple times in the two tests.

"Test. <script>alert(document.cookie)</script>"
"Test. <div style={xss:expression(xss)}>"
"Test. <s%00cript>alert(document.cookie)</script>"
"Test. <s\tcript>alert(document.cookie)</script>"
"Test. <s\r\n\0cript>alert(document.cookie)</script>"
davewichers commented 5 years ago

I'm not seeing this test case fail on CentOS 7.

On Sun, Oct 6, 2019 at 11:36 PM Kevin W. Wall notifications@github.com wrote:

Reproduce by using JDK 8 and run the following commands:

$ rm -fr ./esapi-java-legacy $ git clone https://github.com/ESAPI/esapi-java-legacy.git $ cd ./esapi-java-legacy $ mvn compile $ mvn -Dtest=org.owasp.esapi.reference.ValidatorTest#testIsValidSafeHTML test

which will result in a failure at line 890 in ValidatorTest.java, which is this line: assertTrue(instance.isValidSafeHTML("test", "Test.

", 100, false)); The details of the failure, including what commit broke this test may be found here . The "fix" for this *MUST NOT* *simply* consist of changing this test. As part of commit [d869303 ](https://github.com/ESAPI/esapi-java-legacy/commit d869303 /d86930362d703c790a18453882db1541c23f5c16), we have broken previously existing behavior, and even if that behavior was incorrect (and that could be argued since the behavior did not previously follow the documented Javadoc). So we have two options: 1) roll back to the previous (and presumably incorrect?) behavior and "fix" by changing the Javadoc to agree with that previous behavior, 2) keep the current new (correct?) behavior as the default, but add some property to the ESAPI.properties that will allow ESAPI users to revert back to the prior behavior, 3) leave everything as-is and just announce this in the next release notes. I personally am adamantly against the third approach because few, if any read the release notes. Before any "fixes" I would like a debate on what the correct / best approach should be. Note: @davewichers apparently has not seen this under CentOS 7 or 8 (or whatever version he is running). I'm hoping he can confirm that. -kevin — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub , or mute the thread .
jeremiahjstacey commented 5 years ago

Just to confirm @davewichers , are you running the test from a clean clone of the root esapi-legacy project or from your fork? Trying to track down the reason you would not be seeing the failure and I happened to notice that your fork does not yet contain the offending changeset.

davewichers commented 5 years ago

I'm using Kevin's apparently:

$ git config --get remote.origin.url https://github.com/kwwall/esapi-java-legacy.git

-Dave

On Wed, Oct 9, 2019 at 4:29 PM jeremiahjstacey notifications@github.com wrote:

Just to confirm @davewichers https://github.com/davewichers , are you running the test from a clean clone of the root esapi-legacy project or from your fork? Trying to track down the reason you would not be seeing the failure and I happened to notice that your fork does not yet contain the offending changeset.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/issues/521?email_source=notifications&email_token=ABGFWBKFYD6DJC2NQMFIPIDQNY5I7A5CNFSM4I57ISY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAZHSWA#issuecomment-540178776, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGFWBJAMIWXFMKWQFO7J4LQNY5I7ANCNFSM4I57ISYQ .

davewichers commented 5 years ago

Wow, weird.

So I had an existing dir pulled from: https://github.com/ESAPI/esapi-java-legacy.git https://github.com/kwwall/esapi-java-legacy.git

So I went into that directory, did a git pull, set my branch to master, and did a pull again (just in case). Then ran mvn clean, then mvn compile, then: mvn -Dtest=org.owasp.esapi.reference.ValidatorTest#testIsValidSafeHTML test

And it passed no problem. But to just be absolutely sure, I then also did rm -rf ./esapi-java-legacy first, then recloned it, then mvn compile, then the same test command, and it FAILED this time.

No clue why. But I'm now seeing the same failure (I believe). I'm using Java 1.8.0_201-b09 by the way.

I then did the same thing on Windows, and get the same failure as well using a slightly older version of Java 8.

-Dave

On Wed, Oct 9, 2019 at 4:35 PM Dave Wichers dwichers@gmail.com wrote:

I'm using Kevin's apparently:

$ git config --get remote.origin.url https://github.com/kwwall/esapi-java-legacy.git

-Dave

On Wed, Oct 9, 2019 at 4:29 PM jeremiahjstacey notifications@github.com wrote:

Just to confirm @davewichers https://github.com/davewichers , are you running the test from a clean clone of the root esapi-legacy project or from your fork? Trying to track down the reason you would not be seeing the failure and I happened to notice that your fork does not yet contain the offending changeset.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/issues/521?email_source=notifications&email_token=ABGFWBKFYD6DJC2NQMFIPIDQNY5I7A5CNFSM4I57ISY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAZHSWA#issuecomment-540178776, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGFWBJAMIWXFMKWQFO7J4LQNY5I7ANCNFSM4I57ISYQ .

kwwall commented 5 years ago

@davewichers Can you try on CentOS 7 using https://github.com/ESAPI/esapi-java-legacy.git (from the 'develop' branch). @jeremiahjstacey - If you got this to fail from the 'master' branch, that would include the PR that I thought broke this, in which case I have no idea of what's going on. It worked back in June or so when we released 2.2.0.0. I have no test failures then, but have had it fail consistently ever since we merged @ZiluckMichael's PR #501. As far as I can tell, we just were not re-throwing an exception that AntiSamy was originally throwing. Before PR #501, we were just catching it and logging, but now we are throwing an exception in those cases (as per our original Javadoc).

Thus my concern--if throwing an exception is indeed the correct behavior--is many people likely are relying on the old behavior of ESAPI not throwing in that case. I am reluctant to break production code because we "missed this" early (i.e., Issue #509) and now we decide to fix it. I remember when David Korn did that with the KornShell in an early release and broke backward compatibility with the Bourne Shell even though the latter's behavior was clearly broken / buggy. Lots of developers pressured him to restore the Bourne Shell behavior. So if we decide that the current behavior is correct after merging PR #510, then we need to provide some fallback mechanism to allow for the previous behavior (even though we may think it is incorrect). OTOH, if we decide that it was just the documentation was wrong, then we should rollback the commits associated with PR #510 and fix issue #510 by correcting the Javadoc.

At this point, I think the mos critical thing is to make sure everyone is seeing the test failure I described in this issue regardless of what platform, OS, or specific JDK version that you are using. Because if we are getting inconsistent test results across different environments, I think there is something much deeper going on here that we will first need to figure out.

jeremiahjstacey commented 5 years ago

Master is still logging. If that branch is failing it is a different failure.

(refer to line 117)

davewichers commented 5 years ago

Hey. Sorry I wasn't clear yesterday. On CentOS7 I get these results:

And same results on Windows.

-Dave

On Wed, Oct 9, 2019 at 9:55 PM Kevin W. Wall notifications@github.com wrote:

@davewichers https://github.com/davewichers Can you try on CentOS 7 using https://github.com/ESAPI/esapi-java-legacy.git (from the 'develop' branch). @jeremiahjstacey https://github.com/jeremiahjstacey - If you got this to fail from the 'master' branch, that would include the PR that I thought broke this, in which case I have no idea of what's going on. It worked back in June or so when we released 2.2.0.0. I have no test failures then, but have had it fail consistently ever since we merged @ZiluckMichael https://github.com/ZiluckMichael's PR #501 https://github.com/ESAPI/esapi-java-legacy/pull/501. As far as I can tell, we just were not re-throwing an exception that AntiSamy was originally throwing. Before PR #501 https://github.com/ESAPI/esapi-java-legacy/pull/501, we were just catching it and logging, but now we are throwing an exception in those cases (as per our original Javadoc).

Thus my concern--if throwing an exception is indeed the correct behavior--is many people likely are relying on the old behavior of ESAPI not throwing in that case. I am reluctant to break production code because we "missed this" early (i.e., Issue #509 https://github.com/ESAPI/esapi-java-legacy/issues/509) and now we decide to fix it. I remember when David Korn did that with the KornShell in an early release and broke backward compatibility with the Bourne Shell even though the latter's behavior was clearly broken / buggy. Lots of developers pressured him to restore the Bourne Shell behavior. So if we decide that the current behavior is correct after merging PR #510 https://github.com/ESAPI/esapi-java-legacy/pull/510, then we need to provide some fallback mechanism to allow for the previous behavior (even though we may think it is incorrect). OTOH, if we decide that it was just the documentation was wrong, then we should rollback the commits associated with PR #510 https://github.com/ESAPI/esapi-java-legacy/pull/510 and fix issue #510 https://github.com/ESAPI/esapi-java-legacy/pull/510 by correcting the Javadoc.

At this point, I think the mos critical thing is to make sure everyone is seeing the test failure I described in this issue regardless of what platform, OS, or specific JDK version that you are using. Because if we are getting inconsistent test results across different environments, I think there is something much deeper going on here that we will first need to figure out.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/issues/521?email_source=notifications&email_token=ABGFWBNFBGB7KCRAZRCR5NTQN2DPTA5CNFSM4I57ISY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA2C6EA#issuecomment-540290832, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGFWBLFZLIHINYW7DLDB5TQN2DPTANCNFSM4I57ISYQ .

jeremiahjstacey commented 5 years ago

I feel there are 2 primary questions to answer (I may be over-simplifying)

  1. Are the inputs that are causing the failures ACTUALLY safe input? 
    "Test. <script>alert(document.cookie)</script>"
"Test. <div style={xss:expression(xss)}>"
"Test. <s%00cript>alert(document.cookie)</script>"
"Test. <s\tcript>alert(document.cookie)</script>"
"Test. <s\r\n\0cript>alert(document.cookie)</script>"
  2. Is the antisamy configuration used for the test correct?
    • I think this is src/test/resources/esapi/antisamy-esapi.xml (for test scope)
    • runtime appears to resolve to target/test-classes/esapi/antisamy-esapi.xml

On Antisamy

I see 2 antisamy-esapi.xml files in the baseline: They all appear to match from what I can tell

  1. configuration/esapi/antisamy-esapi.xml
  2. src/test/resources/esapi/antisamy-esapi.xml

Failure Information Breakdown

I went through and extracted the error messages that Antisamy is returning for each of the five lines. "SAFE_HTML" is the return value of CleanResults.getCleanHTML() from within the HTMLValidationRule. The errors are from the CleanResults.getErrorMessages()

  1. INPUT: Test. <script>alert(document.cookie)</script> SAFE_HTML: Test. ERROR_MSG: The script tag is not allowed for security reasons. This tag should not affect the display of the input.

  2. INPUT: Test. <div style={xss:expression(xss)}> SAFE_HTML: Test. ERROR_MSG: The div tag was empty, and therefore we could not process it. The rest of the message is intact, and its removal should not have any side effects.

  3. INPUT: Test. <s%00cript>alert(document.cookie)</script> SAFE_HTML: Test. alert(document.cookie) ERROR_MSG: The s tag has been filtered for security reasons. The contents of the tag will remain in place.

  4. INPUT: Test. <s\tcript>alert(document.cookie)</script> SAFE_HTML: Test. alert(document.cookie) ERROR_MSG: The s tag has been filtered for security reasons. The contents of the tag will remain in place.

  5. INPUT": Test. <s\r\n\0cript>alert(document.cookie)</script> SAFE_HTML: Test. alert(document.cookie)` ERROR_MSG: The s tag has been filtered for security reasons. The contents of the tag will remain in place.

It is currently my opinion

The HTMLValidationRule implementation should adhere to the ValidationRule API. If the exception is being thrown and it should not be, then we should change antisamy to not reject the valid html.

On the other hand, if the test inputs should actually be rejected and they have not been, then this is was a bug in the implementation. The implementation may have been allowing unsafe html into applications (if I'm understanding correctly). I believe the correct response would be to ensure the baseline supports the most secure solution out of the box, and make all updates accordingly to represent that intention. Since antisamy is the tool driving the safe/unsafe html comparison, we may be able to provide a different/new antisamy configuration file to any who would like to retain the previous behavior.

kwwall commented 5 years ago

I just did a 'git pull' from origin 'https://github.com/ESAPI/esapi-java-legacy.git to get the latest updates. I then switched to the 'master' branch and confirmed that there are no failing tests there. So at least that is good. But I am getting different failure results for the 'develop' branch than Jeremiah is getting.

@jeremiahjstacey wrote:

I'm seeing that there are 2 test in the ValidatorTest related to changes in the HtmlValidationRule that are now failing.

testIsValidSafeHTML
testGetValidSafeHTML

Between these tests there are 5 Strings that now cause validation failures. They are repeated multiple times in the two tests.

"Test. " "Test.

" "Test. <s%00cript>alert(document.cookie)" "Test. <s\tcript>alert(document.cookie)" "Test. <s\r\n\0cript>alert(document.cookie)"


So what OS are you running this on? Windows? Running on Linux Mint 19.2 with OpenJDK 8u222-b10-1ubuntu1~18.04.1 (which is 1.8.0_222) and **_only_** get a failure on `testIsValidSafeHTML`.
jeremiahjstacey commented 5 years ago

Windows 7, JDK 8 You're right one test is failing, the other is an error. $ mvn -Dtest=ValidatorTest test Tests run: 46, Failures: 1, Errors: 1, Skipped: 0, Time elapsed: 0.618 s <<< FAILURE! - in org.owasp.esapi.reference.ValidatorTest

testGetValidSafeHTML (ERROR)

 testGetValidSafeHTML(org.owasp.esapi.reference.ValidatorTest)  Time elapsed: 0.002 s  <<< ERROR!
org.owasp.esapi.errors.ValidationException: test: Invalid HTML input
    at org.owasp.esapi.reference.ValidatorTest.testGetValidSafeHTML(ValidatorTest.java:250)

250 (testGetValidSafeHTML) assertEquals("Test.", ESAPI.validator().getRule("test").getValid("test", "Test. <script>alert(document.cookie)</script>"));


org.owasp.esapi.errors.ValidationException: test: Invalid HTML input
at org.owasp.esapi.reference.validation.HTMLValidationRule.invokeAntiSamy(HTMLValidationRule.java:121)
at org.owasp.esapi.reference.validation.HTMLValidationRule.getValid(HTMLValidationRule.java:83)
at org.owasp.esapi.reference.validation.HTMLValidationRule.getValid(HTMLValidationRule.java:1)
at org.owasp.esapi.reference.ValidatorTest.testGetValidSafeHTML(ValidatorTest.java:250)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at junit.framework.TestCase.runTest(TestCase.java:176)
at junit.framework.TestCase.runBare(TestCase.java:141)
at junit.framework.TestResult$1.protect(TestResult.java:122)
at junit.framework.TestResult.runProtected(TestResult.java:142)
at junit.framework.TestResult.run(TestResult.java:125)
at junit.framework.TestCase.run(TestCase.java:129)
at junit.framework.TestSuite.runTest(TestSuite.java:252)
at junit.framework.TestSuite.run(TestSuite.java:247)
at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:86)
at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:89)
at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:41)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:541)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:763)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:463)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:209)

### testIsValidSafeHTML (FAILURE)

testIsValidSafeHTML(org.owasp.esapi.reference.ValidatorTest) Time elapsed: 0.084 s <<< FAILURE! junit.framework.AssertionFailedError at org.owasp.esapi.reference.ValidatorTest.testIsValidSafeHTML(ValidatorTest.java:890)


> 890 (testIsValidSafeHTML)
> `  assertTrue(instance.isValidSafeHTML("test", "Test. <script>alert(document.cookie)</script>", 100, false));`

Both tests use the same 5 input strings, both tests delegate to the HTMLValidationRule through the DefaultValidator.

If I remove the throws clause from the HTMLValidationRule they both pass.
jeremiahjstacey commented 5 years ago

@kwwall do you also see the erroring test?

Michael-Ziluck commented 5 years ago

As a side note, I was the first one to notice and open an issue with this functionality in the many years, and it wasn't even because it was a bug in my code. I just happened to notice it when reading through the documentation. If we wanted to just revert the functionality change and then update the documentation to reflect the actual behavior, that would be a safe route to take as well. I know that idea has already been discussed, but I thought my input would also be relevant since I opened the issue and made the changes in the first place. @jeremiahjstacey @kwwall

kwwall commented 5 years ago

@jeremiahjstacey Yes, I see the same failures / errors are you, but only on the 'develop' branch, which is what I expect.

@ZiluckMichael I'm really not sure what to do about this. I think maybe reaching out to some of the original contributors to see about their intent for this method was. But I think not making it break behavior is more important than making it adhere to the Javadoc (which very few people apparently read anyway since you seem to have been the first to notice the discrepancy). On the other hand, simply fixing the Javadoc is going to try to enumerate those cases of where we still throw ValidationException and when it will fail without throwing that. So I'm not sure what the best course of action is here and would like to get Jeff Williams, Jim Manico, and/or maybe Mike Fauzy to add their $.02 here before we decide what the most appropriate course of action is here. I'll probably try to compose an email, send it to the ESAPI Users Google group and then link to it here.

jeremiahjstacey commented 5 years ago

@kwwall We are all on the same page then. On the 'develop' branch of the baseline the ValidatorTest has 2 tests being scrutinized. First, testIsSafeValidHTML shows an explicit validation failure. Second testGetSafeValidHTML is now throwing an error in execution.

Regardless of what we want the method to do, I would very much like to revisit the question of whether or not the input being tested should, by default, be considered safe or unsafe for a production application?

I have re-listed the inputs below.

  1. "Test. <script>alert(document.cookie)</script>"
  2. "Test. <div style={xss:expression(xss)}>"
  3. "Test. <s%00cript>alert(document.cookie)</script>"
  4. "Test. <s\tcript>alert(document.cookie)</script>"
  5. "Test. <s\r\n\0cript>alert(document.cookie)</script>"

It would help my understanding if these were clarified in a simple yes or no format.
For Example:

  1. Yes
  2. No

Please note that I am not trying to understand what we want to do with the method at this point. I'm simply trying to clarify whether these Strings should be considered 'safe' out of the box.

kwwall commented 5 years ago

(Sorry for this quick reply via my phone. I know that the Gmail app top-posting messes up the formatting a bit in GitHub.)

I do not believe any of these inputs you have listed should be considered "safe". In the past, HTMLValidationRule.getValid(String,String) simply logged these. Now it throws a ValidationException. It probably should have been doing that all along, so if we were starting fresh, IMO, that would be the correct behavior. But changing it to this correct behavior now could break someone's production code. (Could, but IMO, not likely, as I don't see this as legitimate input ever except as edge test cases or maybe websites to teach hacking about XSS.)

I personally am leaning towards keeping the new behavior but adding a new ESAPI property (that we eventually can deprecate) that allows one to revert to the old behavior of logging only instead of throwing a ValidationException in such cases. That's messy and would have to be well documented in the Javadoc and in the release notes.

What are all your thoughts?

-kevin

Blog: http://off-the-wall-security.blogspot.com/ | Twitter: @KevinWWall NSA: All your crypto bit are belong to us.

On Wed, Oct 23, 2019, 08:04 jeremiahjstacey notifications@github.com wrote:

@kwwall https://github.com/kwwall We are all on the same page then. On the 'develop' branch of the baseline the ValidatorTest has 2 tests being scrutinized. First, testIsSafeValidHTML shows an explicit validation failure. Second testGetSafeValidHTML is now throwing an error in execution.

Regardless of what we want the method to do, I would very much like to revisit the question of whether or not the input being tested should, by default, be considered safe or unsafe for a production application?

I have re-listed the inputs below.

  1. "Test. "
  2. "Test.
    "
  3. "Test. <s%00cript>alert(document.cookie)"
  4. "Test. <s\tcript>alert(document.cookie)"
  5. "Test. <s\r\n\0cript>alert(document.cookie)"

It would help my understanding if these were clarified in a simple yes or no format. For Example:

  1. Yes
  2. No

Please note that I am not trying to understand what we want to do with the method at this point. I'm simply trying to clarify whether these Strings should be considered 'safe' out of the box.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ESAPI/esapi-java-legacy/issues/521?email_source=notifications&email_token=AAO6PGZZXASWLIT6FJ36DC3QQA4T7A5CNFSM4I57ISY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECBE4YQ#issuecomment-545410658, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO6PG6DC2SIWGQ6VZV24GDQQA4T7ANCNFSM4I57ISYQ .

jeremiahjstacey commented 5 years ago

I dont' know how easy it would be, but rather than providing an ESAPI property can we offer an alternate antisamy_allow_script.xml that a company can opt-into by explicitly configuring their environment by putting that file in the appropriate path location?

HTMLValidationRule uses org.owasp.validator.html.AntiSamy, which uses the antisamy.xml for the ruleset applied. It makes the most sense to me to update the antisamy configuration and adjust how ESAPI responds to the input.

kwwall commented 5 years ago

@jeremiahjstacey - On the surface, that sounds like a logical approach on the service, but the only way to do that will make ESAPI less secure, because I think we would have to provide an alternate antisamy-esapi.xml file that is simply NOT SAFE. While the previous behavior did not throw a ValidationException when something such as a " Githubissues.

  • Githubissues is a development platform for aggregating issues.