Bug 58040

Summary: Log Forging
Product: POI Reporter: Donald <dkwakkel>
Component: POI OverallAssignee: POI Developers List <dev>
Status: RESOLVED FIXED    
Severity: normal    
Priority: P2    
Version: unspecified   
Target Milestone: ---   
Hardware: PC   
OS: Linux   

Description Donald 2015-06-16 12:51:46 UTC
Log files created by poi can be manipulated. For example in MessageSubmissionChunk:

logger.log(POILogger.WARN, "Warning - unable to make sense of date " +dateS);
 
where dataS can contain unvalidated user input. An attacker could take advantage of this behavior to forge log entries or inject malicious content into the log.

Explanation:

Log forging vulnerabilities occur when:

1. Data enters an application from an untrusted source.
2. The data is written to an application or system log file.

Applications typically use log files to store a history of events or transactions for later review, statistics gathering, or debugging. Depending on the nature of the application, the task of reviewing log files may be performed manually on an as-needed basis or automated with a tool that automatically culls logs for important events or trending information.

Interpretation of the log files may be hindered or misdirected if an attacker can supply data to the application that is subsequently logged verbatim. In the most benign case, an attacker may be able to insert false entries into the log file by providing the application with input that includes appropriate characters. If the log file is processed automatically, the attacker can render the file unusable by corrupting the format of the file or injecting unexpected characters. A more subtle attack might involve skewing the log file statistics. Forged or otherwise, corrupted log files can be used to cover an attacker's tracks or even to implicate another party in the commission of a malicious act. In the worst case, an attacker may inject code or other commands into the log file and take advantage of a vulnerability in the log processing utility.
Comment 1 Andreas Beeker 2015-06-16 15:48:05 UTC
Here are two links to this topic from the first google matches ...:
the owasp entry where the entry text was copied from [1]
and how to setup fortify [2] to create custom rules for it

I haven't checked our sonarcube instance for this error and we somehow don't get a slot in the apache sonarcube.

Maybe some can provide a current fortify (or similar) report for this error?

Andi.

[1] https://www.owasp.org/index.php/Log_Forging
[2] http://stackoverflow.com/questions/12784707/log-forging-fortify-fix
Comment 2 Donald 2015-06-16 20:36:59 UTC
Hi Andi,

It is not clear to me if you need INFO from me or someone else.

See this article how to solve log forging (just replace CR/LF): http://www.jtmelton.com/2010/09/21/preventing-log-forging-in-java/

If you need more info from me, please let me know.
Comment 3 Andreas Beeker 2015-06-16 23:55:06 UTC
Actually I'm not sure how to fix this ...:
first thought was, there might be a config option in the underlying logger, but we can't  rely on it as we have different logger interfaces which some (or all?) do not provide such an option.

Next thought was, to simply change the POILogger class and sanitize the CR/LFs, limit the length, but then we also might need XSS filtering. I don't like the idea of forcing html encoding in the logging class, just because the log might be viewed in a browser.

So maybe we just provide another logging facade with the above features, but then should we enable it by default, by system property (which nobody realize to set it) or some heuristic ("we are running in an appserver, so we should activate xss filtering, because appserver logs are often viewed online ...")?

Andi
Comment 4 Dominik Stadler 2015-06-17 05:02:08 UTC
I think something like XSS is thevresponsibility of the web app doing the displaying, not the logger. At least I did not come across projects which do this in such a pro-active way.

I would just do the CR/LF replacing for now always as additional options/properties tend to be ignored by users :)
Comment 5 Andreas Beeker 2015-06-21 19:23:42 UTC
Fixed with r1686748

The fix just replaces CR/LF - the exception logging stays unchanged.
I've changed the log methods to varargs and because this leads to binary incompatibilities, marked the classes as internal, as I think user code shouldn't depend on our internal log propagation mechanism.

Depending on the discussion [1], we might eventually include and move to SLF4J logging.

Andi.

[1] http://apache-poi.1045710.n5.nabble.com/Logging-Binary-compatibility-td5719152.html