[jira] [Commented] (ZOOKEEPER-3504) An information leakage from FileTxnSnapLog to log:

["Jürgen Wagner (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/ZOOKEEPER-3504?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16905774#comment-16905774 ] 

Jürgen Wagner commented on ZOOKEEPER-3504:
------------------------------------------

As I have seen this message on the Zookeeper lists now at least twice, I feel compelled to reply.

The Logger object will only log if debug mode is enabled, anyway. Therefore, wrapping an SLF4J logger call to LOG.XXX in a LOG.isXXXEnabled() is functionally redundant, and may only check for the debug mode a little bit earlier, saving a bit of CPU time to get into the Logger and check, and to compute TxnHeader.getType().

The only criticism one could apply from my point of view is the use of string concatenation instead of string pattern arguments, because this way we would create string garbage even if debug mode is not enabled. However, you can see this being used at the end of processTransaction(), so you seem to be referring to an older version of the code.

https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java

Sensitive information will not be leaked as debug mode is not supposed to be enabled in a production system (apart from the fact that even with the addition of the proposed check, the same amount of information would be logged).

 

> An information leakage from FileTxnSnapLog to log:
> --------------------------------------------------
>
>                 Key: ZOOKEEPER-3504
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-3504
>             Project: ZooKeeper
>          Issue Type: Bug
>          Components: security, server
>    Affects Versions: 3.4.11, 3.4.12, 3.4.13, 3.5.5, 3.4.14
>            Reporter: xiaoqin.fu
>            Priority: Major
>
> In org.apache.zookeeper.server.persistence.FileTxnSnapLog, the statement LOG.debug don't have LOG controls:
>     public void processTransaction(TxnHeader hdr,DataTree dt,
>             Map<Long, Integer> sessions, Record txn)
>         throws KeeperException.NoNodeException {  
> 		......
>         if (rc.err != Code.OK.intValue()) {
>             LOG.debug("Ignoring processTxn failure hdr:" + hdr.getType()
>                     + ", error: " + rc.err + ", path: " + rc.path);
>         }  
> 		......
>     } 
> Sensitive information about hdr type or rc path was leaked. The conditional statement LOG.isDebugEnabled() should be added:
>     public void processTransaction(TxnHeader hdr,DataTree dt,
>             Map<Long, Integer> sessions, Record txn)
>         throws KeeperException.NoNodeException {  
> 		......
>         if (rc.err != Code.OK.intValue()) {
>         	if (LOG.isDebugEnabled())
> 				LOG.debug("Ignoring processTxn failure hdr:" + hdr.getType()
>                     + ", error: " + rc.err + ", path: " + rc.path);
>         }  
> 		......
>     } 



--
This message was sent by Atlassian JIRA
(v7.6.14#76016)