Missed a legal input case in DefaultSecurityConfiguration.java

[xeno6696]

When was ESAPI-2.2.0.0-RC2 released? Our applications fails to load ESAPI.properties. The change seems to be with xeno6696@2837538

in = loaders[i].getResourceAsStream(DefaultSearchPath.ROOT.toString());

This change attempts to load "ROOT" and not fileName (aka., "ESAPI.properties"). We are picking up this RC version via the transitive dependency in encoder-esapi:

        <dependency>
            <groupId>org.owasp.esapi</groupId>
            <artifactId>esapi</artifactId>
            <version>[2.0,3)</version>
        </dependency>

Originally posted by @fransonsr in #487 (comment)

[lawrencexu]

The workaround I can find is to put the properties file in a sub-folder esapi which is documented way for this class.
Actually it never mentions the "ROOT" usage.

[fransonsr]

I have created a ticket to move our ESAPI.properties file to a proper documented location. However, I think I will continue to use a pinned version of esapi until it is released, just to maintain the principle of "least astonishment." I have brought this up with the OWASP encoder. (RE: OWASP/owasp-java-encoder#30).

[xeno6696]

@lawrencexu , the "ROOT" case is buried in some long history. Back when this lib started, the recommended usage was to compile your own version of the library with your own properties files embedded in the source, at that time at the root of the directory. Then at some point they converted to Maven and it seems over time the possible locations grew, from "/" to ".esapi/" and so on. So it doesn't shock me that the documentation never mentioned the ROOT case. Thanks for pointing this out!

[kwwall]

If "ROOT" is the top level or a .war / .ear file, that sounds like a bad default place for it, as that would be potentially susceptible to "direct browsing" attacks. Why not have it look under "WEB-INF/" instead, so that the JavaEE application container will protect it by default? (Or is that what "ROOT" evaluates to?)

…

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

On Thu, May 2, 2019, 11:28 Matt Seil ***@***.***> wrote: @lawrencexu <https://github.com/lawrencexu> , the "ROOT" case is buried in some long history. Back when this lib started, the recommended usage was to compile your own version of the library with your own properties files embedded in the source, at that time at the root of the directory. Then at some point they converted to Maven and it seems over time the possible locations grew, from "/" to ".esapi/" and so on. So it doesn't shock me that the documentation never mentioned the ROOT case. Thanks for pointing this out! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#488 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAO6PG6YNIKIJ5JZRVVRAL3PTMCAFANCNFSM4HJGN2VA> .

[fransonsr]

I believe that "ROOT" in this context is the root of the classpath.

It evaluates to "ROOT" because of these lines:

esapi-java-legacy/src/main/java/org/owasp/esapi/reference/DefaultSecurityConfiguration.java

Lines 647 to 649 in 9a42290

	// try root
	String currentClasspathSearchLocation = "/ (root)";
	in = loaders[i].getResourceAsStream(DefaultSearchPath.ROOT.toString());

[kwwall]

@xeno6696 Where do we stand with this? This sounds related to the one PR #489. Is it? Asking because the proposed fix here sounds different than for the PR.

[xeno6696]

Yes, it's the same.

…

On Wed, May 29, 2019 at 9:33 PM Kevin W. Wall ***@***.***> wrote: @xeno6696 <https://github.com/xeno6696> Where do we stand with this? This sounds related to the one PR #489 <#489>. Is it? Asking because the proposed fix here sounds different than for the PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#488?email_source=notifications&email_token=ACIQAQKYED2DJZS4JVYZNRTPX5KILA5CNFSM4HJGN2VKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWRKMHI#issuecomment-497198621>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACIQAQMW2ZDOZSAFFZG2QU3PX5KILANCNFSM4HJGN2VA> .

-- Matt Seil Cyber Security Software Engineer Member ACM/OWASP

[kwwall]

Note in #489 (comment), @JoergAdler noted that:
"There are ClassLoader.getResource() and Class.getResource() and they work differently [...] The name should not have a leading “/”."

This was originally fixed in his PR, PR#489, but I rejected that PR because of excessive white-space diffs and that the fact that the properties file really only needed a single property.