Dear developers:
I am a Ph.D. student at Washington State University. I applied dynamic
taint analyzer (distTaint) to Apache Zookeeper (version 3.4.11). And then I
find several bugs, that exist from 3.4.11-3.4.14 and 3.5.5, from tainted
paths:
1. In org.apache.zookeeper.server.ZooKeeperServer:
public ZooKeeperServer(FileTxnSnapLog txnLogFactory, int tickTime,
int minSessionTimeout, int maxSessionTimeout, ZKDatabase zkDb)
{
......
LOG.info("Created server with tickTime " + tickTime
+ " minSessionTimeout " + getMinSessionTimeout()
+ " maxSessionTimeout " + getMaxSessionTimeout()
+ " datadir " + txnLogFactory.getDataDir()
+ " snapdir " + txnLogFactory.getSnapDir());
......
}
public void finishSessionInit(ServerCnxn cnxn, boolean valid)
......
if (!valid) {
LOG.info("Invalid session 0x"
+ Long.toHexString(cnxn.getSessionId())
+ " for client "
+ cnxn.getRemoteSocketAddress()
+ ", probably expired");
cnxn.sendBuffer(ServerCnxnFactory.closeConn);
} else {
LOG.info("Established session 0x"
+ Long.toHexString(cnxn.getSessionId())
+ " with negotiated timeout " +
cnxn.getSessionTimeout()
+ " for client "
+ cnxn.getRemoteSocketAddress());
cnxn.enableRecv();
}
......
}
Sensitive information about DataDir, SnapDir, SessionId and
RemoteSocketAddress may be leaked. I think that it is better to add
LOG.isInfoEnabled() conditional statements:
public ZooKeeperServer(FileTxnSnapLog txnLogFactory, int tickTime,
int minSessionTimeout, int maxSessionTimeout, ZKDatabase zkDb)
{
......
if (LOG.isInfoEnabled())
LOG.info("Created server with tickTime " + tickTime
+ " minSessionTimeout " + getMinSessionTimeout()
+ " maxSessionTimeout " + getMaxSessionTimeout()
+ " datadir " + txnLogFactory.getDataDir()
+ " snapdir " + txnLogFactory.getSnapDir());
......
}
public void finishSessionInit(ServerCnxn cnxn, boolean valid) {
......
if (!valid) {
if (LOG.isInfoEnabled())
LOG.info("Invalid session 0x"
+ Long.toHexString(cnxn.getSessionId())
+ " for client "
+ cnxn.getRemoteSocketAddress()
+ ", probably expired");
cnxn.sendBuffer(ServerCnxnFactory.closeConn);
} else {
if (LOG.isInfoEnabled())
LOG.info("Established session 0x"
+ Long.toHexString(cnxn.getSessionId())
+ " with negotiated timeout " +
cnxn.getSessionTimeout()
+ " for client "
+ cnxn.getRemoteSocketAddress());
cnxn.enableRecv();
}
......
}
The LOG.isInfoEnabled() conditional statement already exists in
org.apache.zookeeper.server.persistence.FileTxnLog:
public synchronized boolean append(TxnHeader hdr, Record txn) throws
IOException {
{ ......
if(LOG.isInfoEnabled()){
LOG.info("Creating new log file: " + Util.makeLogName(hdr.getZxid()));
}
......
}
2. In org.apache.zookeeper.ClientCnxn$SendThread,
void readResponse(ByteBuffer incomingBuffer) throws IOException {
......
LOG.warn("Got server path " + event.getPath()
+ " which is too short for chroot path "
+ chrootPath);
......
}
Sensitive information about event path and chroot path may be leaked. The
LOG.isWarnEnabled() conditional statement should be added:
void readResponse(ByteBuffer incomingBuffer) throws IOException {
......
if (LOG.isWarnEnabled())
LOG.warn("Got server path " + event.getPath()
+ " which is too short for chroot path "
+ chrootPath);
......
}
3. In org.apache.zookeeper.server.ZooTrace, there are two relevant methods
which all use the same conditional statements:
public static void logTraceMessage(Logger log, long mask, String msg) {
if (isTraceEnabled(log, mask)) {
log.trace(msg);
}
}
static public void logQuorumPacket(Logger log, long mask,
char direction, QuorumPacket qp)
{
if (isTraceEnabled(log, mask)) {
logTraceMessage(log, mask, direction +
" " + LearnerHandler.packetToString(qp));
}
}
If other methods call logQuorumPacket, they execute "if
(isTraceEnabled(log, mask))" conditional statement twice.
We should remove one of two "if (isTraceEnabled(log, mask))" conditional
statements.
|