Fixed issue #310

[HJW8472]

I delegated loading from the class path to SecurityConfiguration because all the directory configurations are there.
The private method loadConfigurationFromClasspath from DefaultSecurityConfiguration could reuse that method, but I didn't dare to change that on short notice.

HJW8472

[kwwall]

@xeno6696 and @jeremiahjstacey - Would like your input as well. Especially you Jeremiah, if you can suggest some ways that @HJW8472 can do more thorough testing via mock testing. Also, do either of you see any negative security implications being able to just select ANY file this way? Maybe this getResourceStreamFromClasspath() method should not be put into the SecurityConfiguration interface, but rather just be a private method within orgowasp.esapi.reference.validation.HTMLValidationRule. I'm just concerned that we haven't considered the broader implications of how this new method might be abused by things like deserialization gadgets, etc.

[jeremiahjstacey]

@kwwall I've had trouble isolating resource/classpath tests in the past. I'll try to take a closer look this weekend and see if there's something we can do to get some happy-path coverage.

I think that in this case we would want to consider (as you said above) some form of security-type testing. Do we need to consider an extension whitelist? (no compile or executable extensions) If I were to be providing this for a project I worked on, I would look at:

• what types of files do I want anyone to be able to load at runtime
• Are there restricted locations that loading should be constrained to
• Am I allowing path traversal in the implementation?
• Am I working around any type of pre-existing filesystem restriction which allows a given user to access a file they would not otherwise be able to read or modify

I don't know that I have answers for all of those point, and it's fair to say that some of my initial concerns may not be valid. Mostly offering an example of the kinds of things I'd be talking through with my team before we considered the testing sufficient for a utility like this.

Should those types of considerations be accounted for in this effort, or are we only looking for functional validation that the general stream handling of the method acts predictably?

I'd certainly appreciate opinions on the aforementioned implications, if they are applicable to this scope.

[kwwall]

@jeremiahjstacey - Indeed security testing is part of my concerns. But if the name of the file that is being searched for will always be "antisamy-esapi.xml" (which is what the static initializer in HTMLValidationRule currently enforces), the easier "fix" (and one that would simplify testing to some degree) would be to just change the name of the new SecurityConfiguration method from getResourceStreamFromClasspath(String filename) to something like getAntiSamyResourceFromClasspath() and hard-code what it is looking for. Less room for mischief that way. That seemed to be the original intent of @HJW8472 anyway.

[HJW8472]

So ... After reading though all the comments ... I guess the method should become a private static method of HTMLValidationRule (or one of its superclasses) and be removed from SecurityConfiguration. And also add a test case ..

I will need help creating a test case for this code ..

[jeremiahjstacey]

I didn't get a chance to dig into the tests this weekend. I will do so as soon as I can.

[HJW8472]

I was having a look at the class path loading test cases of DefaultSecurityConfigurationTest.java, but my knowledge may be lacking or they are bugged: since the security configuration files are all in resources directory, the classpath loading may never be triggered. ... or am i overlooking something?

For my test i would need to remove the antisamy-esapi.xml from the src/test/resources/esapi directory and place it elsewhere (for instance src/test/.esapi) .. and after the test put it back.

[kwwall]

Which is one reason why I suggested being able to pass in the basename of the file at least instead of only using esapi-antisamy.xml...namely it would make testing a whole lot easier. That's still an option. You could make your static method package level rather than private so you can still call it directly. (Otherwise you would have to use reflection, which would only work because JUnit testing doesn't use a Java Security Manager.) I'd be okay with that. If it's a concern to anyone, we could go back to making ESAPI a signed and sealed jar.

…

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

On Mon, Apr 6, 2020, 02:50 HJW8472 ***@***.***> wrote: I was having a look at the class path loading test cases of DefaultSecurityConfigurationTest.java, but my knowledge may be lacking or they are bugged: since the security configuration files are all in resources directory, the class path loading may never be triggered. ... or am i overlooking something? For my test i would need to remove the antisamy-esapi.xml from the src/test/resources/esapi directory and place it elsewhere (for instance src/test/.esapi) .. and after the test put it back. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#541 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAO6PG6BSWUCQKOB3XQWQTTRLF3UBANCNFSM4LTI5NAQ> .

[HJW8472]

That would mean to make the filename configurable in ESAPI properties ... may be a good idea .. but would need some work

[jeremiahjstacey]

Sorry all, I still haven't had time to dig in. My initial thought is to try to use PowerMock to return controlled mocks for the InputStream creation. I think we should be able to inspect the path parameters and control the flow of the test without having to rely on the file sytem state at all. That's the theory, may take some massaging to make it play nice. Still planning on jumping in as soon as I can, but things are volatile right now so I don't know when that will be for sure.

[HJW8472]

I made the esapi-antisamy.xml file configurable in esapi.properties, with a default if not set.
I added a test class .... but I have some issues with it .. the first of the 3 test classes to run loads the configuration file ... since I added the first one to run it loads src\test\resources\esapi-antisamy-CP.xml. The one I added was a copy from the last one to run, that still has to be removed and my test class to be renamed with Throw in it ... and src\test\resources\esapi\antisamy-esapi.xml is kind of useless too.
Run the tests to see that a classpath loading message is coming. I have no idea if an assert is possible there or not.

But have a look at this stage first before i do the final cleanup.

[kwwall]

@HJW8472 - a few other notes in addition to the ones I made in the code review.

1. Please document the new property that you added in both of the ESAPI.properties files. This is important, otherwise, no one will be able to figure out how to use it. In fact, I suggest setting it to:
   Validator.HtmlValidationConfigurationFile=antisamy-esapi.xml
   and then describe that this is the default and when you might want to change it and the steps that are used to search for it.
2. In HTMLValidationRule.java, after line 55, please add a "IMPORTANT NOTE" and state that this private method is very similar to the private method DefaultSecurityConfiguration.loadConfigurationFromClasspath() but that we INTENTIONALLY did not want to make that method public and just make minor tweaks to it because we were concerned about it being abused to obtain private resource files. I think it is important to do that, otherwise some ambitious person will come along and attempt to refactor things by doing exactly that and we could miss that they are doing that or forget our rationale by then. So let's get the rationale recorded in the code that we copied so that it doesn't get undone in the future.
3. Line 150 of HTMLValidationRule.java, change the exception message to say:
   Couldn't parse AntiSamy policy file " + antisamyPolicyFilename
4. Line 154 of HTMLValidationRule.java, change the exception message to say:
   Couldn't find AntiSamy policy file " + antisamyPolicyFilename
   I think I made all my other comments on the code review page (mostly about the current JUnit tests being insufficient). I will review again once you have made these changes.

Thanks for your patience. I know we've all been busy with this COVID-19 pandemic so I appreciate your effort during these times.

[kwwall]

We are getting nowhere fast on this, so here is what I'm going to propose. @HJW8472 can pull down the 'develop' branch of esapi-java-legacy and then add his PR to it. Build it and then use that ESAPI snapshot jar to test the web application that he had in mind. As long as he 1) changes the name of the AntiSamy config file that ESAPI uses to something like My-AntiSamy-ESAPI.xml (anything that is different than the build in default) and 2) places it in his application's .war file as an embedded resource so it will have to be loaded by one of the classpath loaders, they at least we will have tested some of the essential functionality. If it that doesn't work, then even if I merge his PR it would work an official ESAPI release, so I see that as essential. It's an absolute bare minimum. And aside from building a snapshot jar of ESAPI and using it instead of one pulled down from Maven Central, it should not be that much work. It would be much much easier than trying to write any mock tests of the other things we were suggesting.

@HJW8472 - Would you agree to that? You are the one that wanted this feature if I recall so it is in your own best interest to give it a semi-real test. You can do that and then just add a comment attesting to what you did and whether or not it worked and we can then do the merge if everything works out and worry about further testing at a later date. But I just don't want to promise some new feature that is not going to work as anticipated.

Thoughts everyone?

[HJW8472]

Not agreeable, you guys are asking way too much to test this. It feels like overhauling the whole system and fixing all testing bugs at once.
But understandable because it takes too long ... I will think about what to do.

[xeno6696]

@HJW8472 -- I don't think @kwwall is asking for more than this: Compile a local copy of the lib using test properties to correspond with your issue. Kevin can correct me of I'm wrong, but I believe it boils down to:

Test locally on your application to see if it's working properly. You do this by compiling the lib with your new properties file additions compiled into the binary. (add changes to src/main/resources/*.properties IIRC) After validation, revert the prod properties values you compiled into production and report results here. This avoids both mocks and mvn:verify and at least gets us a manual test.

[kwwall]

That is exactly what I was asking for, with the addition that you change the name of your ESAPI AntiSamy config file to be something *different* than "antisamy-esapi.xml". That much is absolutely required and I would think that you would find that essential. Because if you don't do that test, you won't know if it is picking up anything other than the default one. And honestly that doesn't seem like a lot of work to me.

…

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

On Fri, Jun 5, 2020, 11:22 Matt Seil ***@***.***> wrote: @HJW8472 <https://github.com/HJW8472> -- I don't think @kwwall <https://github.com/kwwall> is asking for more than this: Compile a local copy of the lib using test properties to correspond with your issue. Kevin can correct me of I'm wrong, but I believe it boils down to: Test locally on your application to see if it's working properly. You do this by compiling the lib with your new properties file additions compiled into the binary. (add changes to src/main/resources/*.properties IIRC) After validation, revert the prod properties values you compiled into production and report results here. This avoids both mocks and mvn:verify and at least gets us a manual test. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#541 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAO6PG3RQC3EUDYPL2L65FDRVEETRANCNFSM4LTI5NAQ> .

[HJW8472]

To my knowledge I have already tested my changes by adding src/test/java/org/owasp/esapi/reference/validation/HTMLValidationRuleClasspathTest.java In the logs created by "mvn test" you can see that the antisamy.xml is loaded from the classpath and all the remaining test work.
What you don't see is that the old way of loading antisamy.xml is still working. But that can be done by renaming (and so removing from "mvn test") of HTMLValidationRuleClasspathTest.java to HTMLValidationRuleClasspathTest.java.TestToBeRunManualAndNeedsToBeFirstInAlphabeticalOrder in the final pullrequest or not?

And yes I still need to document the new property better. But I am waiting for the above discussion to finish before putting in any extra effort.

[kwwall]

@HJW8472 - I just submitted a PR that will be 2.2.1.0-RC1. I will try to get your PR into RC2, but I must insist on the additional new properties that you added for this to be documented in comments in the configuration/esapi/ESAPI.properties configuration file. If it's not documented then I will reject the PR. If you already did that, just point me to the specific commit as I may have missed it and it is late and I don't want to try to chase that down. I will also add you in the release notes.

[HJW8472]

Have a look at the changes in configuration/esapi/ESAPI.properties

[kwwall]

I'm okay with these changes that were made and while we still could use some additional testing, we've never really tested all the class loader possibilities for finding the ESAPI.properties file in the DefaultSecurityConfiguration class either, so it would be wrong (or at best, inconsistent) to insist on it here in a use case that is likely to be exercised much less often. Therefore I approve the changes and accept the PR.