ESAPI is not returning right logger level

[SalmanMohammedTR]

Description :
org.owasp.esapi.Logger class methods always return true irrespective of root logger level.

• isDebugEnabled()
• isErrorEnabled()
• isInfoEnabled()
• isFatalEnabled()
• isTraceEnabled()
• isWarningEnabled()

Version : esapi-2.4.0.0

Analysis:
Since i am using SL4J noticed that while creating following object new Slf4JLogger(slf4JLogger, LOG_BRIDGE, Logger.ALL); the 3rd parameter always passed as ALL rather it should be taken from logback.xml file.

[jeremiahjstacey]

This is true for all of the *LogFactory implementations. The ESAPI logger is enabled to support all ESAPI Logging levels by default; however, it will filter the log event to the delegate logger in the bridge implementations.

https://github.com/ESAPI/esapi-java-legacy/blob/develop/src/main/java/org/owasp/esapi/logging/slf4j/Slf4JLogBridgeImpl.java

   public void log(Logger logger, int esapiLevel, EventType type, String message) {
        Slf4JLogLevelHandler handler = esapiSlfLevelMap.get(esapiLevel);
        if (handler == null) {
            throw new IllegalArgumentException("Unable to lookup SLF4J level mapping for esapi value of " + esapiLevel);
        }
HERE -->        if (handler.isEnabled(logger)) {

To that end, the implementation IS ACTUALLY returning "the right logger level". The Logger class is comparing against the configured ESAPI logging level from the constructor: ALL. It is working as designed and intended.

---

That being said, I will concede that there is room for an enhancement :

Enable Configuration for the default ESAPI Logging Level

I would suggest:

1. Add a new property to ESAPI.properties to capture the desired default logging level. (It should default to ALL)
2. Update the SLF4JLogFactory to read that property value and resolve the associated org.owasp.esapi.Logger level value
3. Update the SLF4JLogFactory to pass the new configuration to the SLF4JLogger constructor.
4. Apply the same changes to the JavaLogFactory

If the property value cannot be resolved to a Logger level value, then the behavior should be well defined and intentional. These seem like reasonable options:

• Throw a ConfigurationException?
• Default to ALL?

I might even be tempted to try converting the Level references into an enumeration for easier reference and resolution, but that may not be necessary.

At that point you'll be able to set the value to match your logback.xml configuration (if you would choose to do that). It should not be implemented to refer to the underlying logger implementation, as that makes too many assumptions about logging behavior across client environments.

[kwwall]

@jeremiahjstacey wrote:

I would suggest:

Add a new property to ESAPI.properties to capture the desired default logging level. (It should default to ALL)

The only potential problem I see with this is what if the default log level of the underlying logger (JUL, or whatever is used through SLF4J) is different than what might be in the ESAPI.properties file. That could lead to inconsistent logging in the following sense: Much of the code I see using ESAPI only indirectly uses ESAPI logging via the other components (e.g., through the ESAPI Encoder, ESAPI Validator, ESAPI Encryptor, etc.) and for all the rest of their application they are explicitly using SLF4J or JUL and have a default log value set elsewhere. So that could cause unexpected logging in an application if those log levels were different. So if you add a new property to ESAPI.properties, then I think we need to figure out how to address that edge case with the principle of least surprise and I'm not sure exactly what that would be.

[jeremiahjstacey]

You're right, @kwwall. There is a chance for inconsistency, which is why I left it as ALL as a hard-coded attribute until now.

To your point

figure out how to address that edge case with the principle of least surprise

My interpretation is that there is a misunderstanding as to how the ESAPI Logger is behaving. To aide in level-setting the expectations of current behavior I would like to submit the following points:

• The ESAPI Log level is a configuration at the ESAPI Logging implementation Level.
• It is a value which determines whether a logging event from the ESAPI Logging API should be processed by the ESAPI Logging implementation.
  • It is not (currently) meant to reflect or indicate any reference to the underlying logging implementation selected by a client environment.
• ESAPI is not an SLF4J implementation, but a Decorator around a delegate logger, and has its own API and configuration that it adheres to.

---

If we want to consider other options, I've identified some additional possibilities below.

1. Set up the Logger to use the bridge to determine if the underlying logger level is enabled.

This seems appealing. The ESAPI Logging level would be used as ESAPI-internal references only to bridge between an ESAPI log event and a delegate logger level. Eventually, this will get us back to the same problem though; someone will want the map of the ESAPI log level to delegate log level exposed.

This option accounts for more of the edge cases of the request, but will introduce more questions later.

2. Deprecate the esapi Logger level enabled check API

Not my favorite option, but still valid to consider. If we leave all other implementation alone and deprecate the isEnabled methods (with intent to delete in insert timeline here), then there is no additional confusion of the API. The ESAPI Log wrapper forwards everything and relies on the underlying config.

This option is the most simple, requiring the least code changes and insulates from future work around ESAPI Logging configuration exposure

3. Allow the User to configure the ESAPI Logging Level Independent of the underlying Logger

This is the original option. The implementations have been using Level.ALL since the logging updates went in. This conversation is the first discussion we've had on the subject since that integration. The lack of variation in the configuration makes me feel that most users are satisfied with the current behavior and will not be leveraging the configuration. If someone is going to configure the system to act differently than the default installation then the onus of understanding the change they make is on them.

This option is aligned with the current approach of the baseline to expose the minimal piece required to configure the application in the current baseline state. It does come with a Buyer Beware tag, but I do not believe that it will be a frequent issue since it took this long to be brought up.

4. Deprecate the ESAPI Logger API entirely and convert the project to use the SLF4J Logger

Long-term this solves all of the client confusion problems. Most of the more commonly used SLF4J implementations now support log forging protection as well as output encoding. Examples would be required as a part of this effort.

This pushes the full responsibility of Logging configuration on the client environments, so users will be able to introduce issues if they do not understand how to configure their chosen logging frameworks.

Least feasible short-term option. Requires identifying the most likely SLF4J delegate logger types and researching the log forging and encoding implementation strategies, along with configuration documentation.

[SalmanMohammedTR]

Hi Team,
Guys any conclusion out of this discussion?
Thanks.,

[SalmanMohammedTR]

Hi @kwwall any conclusion out of this discussion?

[kwwall]

@SalmanMohammedTR - My inclination is to close this as "wontfix" unless @jeremiahjstacey can think of a solution that will not have unexpected impact. It doesn't sound like there are any good solutions to this though and I think it is relatively low impact, so at this point I think that the best we can offer is to try to thoroughly document this in the Javadoc and perhaps on a ESAPI GitHub wiki page. The only path forward that I see is to deprecate the direct use of JUL and then force those who still wish to use JUL to do so through SLF4J. However, our deprecation process takes 2 years and in this case, we don't really have the advantage of getting rid of any dependent jars. Furthermore, the fact that very few people read the release notes gives me pause for deprecating JUL and changing the default logger to SLF4J as the last time we made a change to ESAPI.Logger it caused all sorts of confusion because ESAPI clients migrating from earlier versions completely ignoring the release notes.