You are viewing a plain text version of this content. The canonical link for it is here.
Posted to user@zookeeper.apache.org by Xiaoqin Fu <xi...@gmail.com> on 2019/08/01 02:11:24 UTC
Apache Zookeeper Bugs
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.
Re: Apache Zookeeper Bugs
Posted by Alexander Shraer <sh...@gmail.com>.
Thanks Xiaoqin! Would you be able to open a Jira for this and perhaps
submit a PR ?
https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute
On Thu, Aug 1, 2019 at 8:23 AM Xiaoqin Fu <xi...@gmail.com> wrote:
> 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.
>
Re: Apache Zookeeper Bugs
Posted by Alexander Shraer <sh...@gmail.com>.
Thanks Xiaoqin! Would you be able to open a Jira for this and perhaps
submit a PR ?
https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute
On Thu, Aug 1, 2019 at 8:23 AM Xiaoqin Fu <xi...@gmail.com> wrote:
> 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.
>