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

Solution 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.

Solution 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));

Solution 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));

Solution 4

In order to avoid Veracode CWE 117 vulnerability I have used a custom logger class which uses HtmlUtils.htmlEscape() function to mitigate the vulnerablity. Recommended solution to this problem by Veracode is to use ESAPI loggers but if you dont want to add an extra dependency to your project this should work fine. https://github.com/divyashree11/VeracodeFixesJava/blob/master/spring-annotation-logs-demo/src/main/java/com/spring/demo/util/CustomLogger.java

Solution 5

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.

View more solutions

Comments

[Jimson James]

Please add the solution worked for you as well.

[Vitaliy Borisok]

@Aczire as I mentioned in UPD: I simply used ESAPI logger without any additional configuration. It used my default slf4j logger. private static final Logger logger = ESAPI.getLogger(MyClass.class); ... logger.error(null, "Simple error message", ex);

[Charu Jain]

Hi @VitaliyBorisok , I am also facing the same issue. Can you please help me with what Slf4j configuration you used with ESAPI logger. I used your above suggested solution. But I am getting: Caused by: java.lang.IllegalArgumentException: Failed to load ESAPI.properties as a classloader resource.

[Vitaliy Borisok]

Hi @CharuJain, Do you have ESAPI.properties file in your classpath? ESAPI library requires this file. Check github.com/OWASP/EJSF/blob/master/esapi_master_FULL/WebConte‌​nt/… as an example of ESAPI configuration file.

[scott.korin]

I would argue that logging exceptions don't need to be sanitized for CRLF, because the exception data IS trusted (can't be manipulated by a user)

[Vitaliy Borisok]

Thanks for clarifications and suggestions, but I believe in my case Veracode doesn't like Throwable parameter, because message is just a String without any concatenation with dynamic params, so simple encodeForHTML doesn't work here and ESAPI doesn't provide methods to get valid Throwable for logging.

[Vitaliy Borisok]

In the end I used ESAPI logger. I thought it wouldn't use my default logging service, but I was wrong.

[avgvstvs]

If we're literally talking about passing around a literal Throwable that makes sense since in general it's not a great idea to play with exceptions at that level.

[Vitaliy Borisok]

I agree with you.

[Kevin M]

So why has the ESAPI library not been actively maintained since 2011? There was a .1 release and a .1.1 release since then, that's it. No activity. Why should we use a library that's not keeping up with the latest in security since 2011?

[avgvstvs]

@KevinM Since 2011 it was just one guy, until I rolled up my sleeves and made it 2. 2.1.0.1 fixed a CWE and a few long-standing bugs, and we're on a point release (immanently) and a major release (2.2) coming later this year. If guys like you help out, we could release more often!

[Kevin M]

Good point. I think I just expected there to be a bigger "team" of people contributing to this library since it's so widely recommended as the go to library for solving lots of security issues. Perhaps I will take a look :)

[avgvstvs]

@KevinM It was bigger at inception, but reality is a lot of guys dropped off after (2013, not 2011).

[Deian]

Arent you missing the actual error message like that?

[Urosh T.]

Yes, this solves the issue with a Spring Utils class. Good stuff, you don't need to import additional dependencies or change the current logger implementation.

[Vitaliy Borisok]

Interesting. But I have a question/doubt about performance issues of this approach. For example brand object is very 'heavy' and you log it only at debug level, in prod you have info level. In your code you still processing brand object each time without respecting log level that is turned on. Is there a way to use your escape util without additional if statement? It seems ugly to put if statement for each log and it breaks some benefits of slf4j

[Vishwas Upadhyay]

I guess you are trying to ask what should be performant way of logging. well developer should use their best judgment to use proper log levels to print heavy objects. The idea here is to use htmlEscape method to surpass CWE117 problem.

[user2957009]

This is nice, but unfortunately Veracode does not actually know this (or any other solution while writing the log) has taken place, and you still have to mitigate.

[mzzzzb]

@user2957009 If you believe this fixes the security vulnerability in question, you can close the violation as 'Mitigated by design'. Afterall we should strive to write secure and maintainable code, not just to make Veracode happy