Dear Fangmin and other developers:
I added two comments:
For https://issues.apache.org/jira/browse/ZOOKEEPER-3488:
However, in org.apache.zookeeper.server.quorum.FastLeaderElection, lines
319 - 321:
if(LOG.isInfoEnabled()){
LOG.info("Backward compatibility mode, server id=" + n.sid);
}
LOG.isInfoEnabled() also exists in
org.apache.zookeeper.server.persistence.FileTxnLog and
org.apache.zookeeper.server.quorum.FLECompatibilityTest.
The message is very short.
I think LOG.isInfoEnabled() should be deleted in here.
For https://issues.apache.org/jira/browse/ZOOKEEPER-3504:
However, in org.apache.zookeeper.ClientCnxn, lines 112 - 121:
static {
......
disableAutoWatchReset =
Boolean.getBoolean("zookeeper.disableAutoWatchReset");
if (LOG.isDebugEnabled()) {
LOG.debug("zookeeper.disableAutoWatchReset is "
+ disableAutoWatchReset);
}
}
disableAutoWatchReset is a boolean variant, and should not be critical. The
message is also very short and doesn't have string concatenation.
I think LOG.isDebugEnabled() should be deleted in here.
Another thing is that zookeeper is used in many production systems so that
we should protect sensitive information.
Thank you very much!
Yours sincerely
Xiaoqin Fu
On Mon, Aug 12, 2019 at 11:45 AM Fangmin Lv <[email protected]> wrote:
> Agreed with Enrico and Patrick, those informations in log seems not
> critical to me, doesn't sound like a "Security issue".
>
> Regards,
> Fangmin
>
> On Mon, Aug 5, 2019 at 10:53 PM Patrick Hunt <[email protected]> wrote:
>
>> On Mon, Aug 5, 2019 at 10:17 PM Enrico Olivelli <[email protected]>
>> wrote:
>>
>> > Xiaoqin
>> >
>> > Il giorno mar 6 ago 2019 alle ore 03:25 Xiaoqin Fu <
>> [email protected]>
>> > ha scritto:
>> >
>> > > Dear Patrick Hunt:
>> > > I opened two issues in the JIRA:
>> > > https://issues.apache.org/jira/browse/ZOOKEEPER-3488 and
>> > > https://issues.apache.org/jira/browse/ZOOKEEPER-3489
>> > > I am very sorry that I cannot reply
>> > >
>> > >
>> >
>> https://lists.apache.org/thread.html/ea0f515bcb994bae54e62584c2c659b83efa4d22e741d4b688c4479b
>> > > <
>> > >
>> >
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.apache.org_thread.html_ea0f515bcb994bae54e62584c2c659b83efa4d22e741d4b688c4479b-40-253Cuser.zookeeper.apache.org-253E&d=DwMFaQ&c=C3yme8gMkxg_ihJNXS06ZyWk4EJm8LdrrvxQb-Je7sw&r=I7kF79XmGGZ9RLTiorMoewvoyduMXtLcRb1B2epe9cI&m=C_FrvfYh4GtqQesDyKDl4kau6xsDwvLGHAA0IZB9etE&s=mZcIhnGSnLca5JxJbOoKzoTiSuQQZmtyDo_eadoJHcw&e=
>> > > >
>> > > Please confirm them and give them CVE IDs.
>> > >
>> >
>> > Honestly they don't sound to me as "Security issues",
>> > we are talking about not using LOG.isXXXEnabled that is only an
>> > optimization
>> >
>> > if (Log.isWarnEnabled()) {
>> > Log.warn(xxxxxx)
>> > }
>> >
>> > this construct is used only to save resources and do no evaluate "xxx",
>> it
>> > is expected
>> > that the log subsystem won't output the result of "xxx" if configured
>> > propertly.
>> >
>> > Maybe I am missing something
>> >
>> >
>> That was essentially my read as well - see my comments on the two jira
>> earlier today.
>>
>> Regards,
>>
>> Patrick
>>
>>
>> > Regards
>> > Enrico
>> >
>> >
>> >
>> > >
>> > >
>> > > Thank you very much!
>> > > Yours sincerely
>> > > Xiaoqin Fu
>> > >
>> > > On Mon, Aug 5, 2019 at 6:23 PM Xiaoqin Fu <[email protected]>
>> wrote:
>> > >
>> > > > Dear Patrick Hunt:
>> > > > I opened two issues in the JIRA:
>> > > > https://issues.apache.org/jira/browse/ZOOKEEPER-3488 and
>> > > > https://issues.apache.org/jira/browse/ZOOKEEPER-3489
>> > > > I am very sorry that I cannot reply
>> > > >
>> > >
>> >
>> https://lists.apache.org/thread.html/ea0f515bcb994bae54e62584c2c659b83efa4d22e741d4b688c4479b
>> > > > <
>> > >
>> >
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.apache.org_thread.html_ea0f515bcb994bae54e62584c2c659b83efa4d22e741d4b688c4479b-40-253Cuser.zookeeper.apache.org-253E&d=DwMFaQ&c=C3yme8gMkxg_ihJNXS06ZyWk4EJm8LdrrvxQb-Je7sw&r=I7kF79XmGGZ9RLTiorMoewvoyduMXtLcRb1B2epe9cI&m=C_FrvfYh4GtqQesDyKDl4kau6xsDwvLGHAA0IZB9etE&s=mZcIhnGSnLca5JxJbOoKzoTiSuQQZmtyDo_eadoJHcw&e=
>> > > >
>> > > > Please confirm them and give them CVE IDs.
>> > > >
>> > > >
>> > > > Thank you very much!
>> > > > Yours sincerely
>> > > > Xiaoqin Fu
>> > > >
>> > > >
>> > > > On Fri, Aug 2, 2019 at 11:20 AM Patrick Hunt <[email protected]>
>> wrote:
>> > > >
>> > > >> I know. :-) Once a vulnerability has been publicly disclosed there
>> is
>> > no
>> > > >> value in handling it on the security@ list - that's for
>> undisclosed
>> > > >> vulnerabilities to be handled in a responsible (private) manner
>> until
>> > a
>> > > fix
>> > > >> can be released to end users. You can see the process here:
>> > > >> https://www.apache.org/security/committers.html
>> > > >>
>> > > >> Regards,
>> > > >>
>> > > >> Patrick
>> > > >>
>> > > >> On Fri, Aug 2, 2019 at 10:24 AM Xiaoqin Fu <[email protected]>
>> > > wrote:
>> > > >>
>> > > >>> Dear Patrick Hunt:
>> > > >>>
>> > > >>>
>> > >
>> >
>> https://lists.apache.org/thread.html/ea0f515bcb994bae54e62584c2c659b83efa4d22e741d4b688c4479b@
>> > > <user.zookeeper.apache.org>
>> > > >>> <
>> > >
>> >
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.apache.org_thread.html_ea0f515bcb994bae54e62584c2c659b83efa4d22e741d4b688c4479b-40-253Cuser.zookeeper.apache.org-253E&d=DwMFaQ&c=C3yme8gMkxg_ihJNXS06ZyWk4EJm8LdrrvxQb-Je7sw&r=I7kF79XmGGZ9RLTiorMoewvoyduMXtLcRb1B2epe9cI&m=C_FrvfYh4GtqQesDyKDl4kau6xsDwvLGHAA0IZB9etE&s=mZcIhnGSnLca5JxJbOoKzoTiSuQQZmtyDo_eadoJHcw&e=
>> > >
>> > > was
>> > > >>> disclosed by me.
>> > > >>>
>> > > >>> Thank you very much!
>> > > >>> Yours sincerely
>> > > >>> Xiaoqin Fu
>> > > >>>
>> > > >>>
>> > > >>> On Fri, Aug 2, 2019 at 11:04 AM Patrick Hunt <[email protected]>
>> > wrote:
>> > > >>>
>> > > >>>> Hi Xiaoqin Fu. Thank you for the report. The Apache Software
>> > > Foundation
>> > > >>>> and the Apache ZooKeeper project takes security issues very
>> > seriously.
>> > > >>>>
>> > > >>>> Unfortunately this issue has already been publicly disclosed on
>> both
>> > > >>>> the dev and user lists:
>> > > >>>>
>> > > >>>>
>> > >
>> >
>> https://lists.apache.org/thread.html/ea0f515bcb994bae54e62584c2c659b83efa4d22e741d4b688c4479b@
>> > > <user.zookeeper.apache.org>
>> > > >>>> <
>> > >
>> >
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.apache.org_thread.html_ea0f515bcb994bae54e62584c2c659b83efa4d22e741d4b688c4479b-40-253Cuser.zookeeper.apache.org-253E&d=DwMFaQ&c=C3yme8gMkxg_ihJNXS06ZyWk4EJm8LdrrvxQb-Je7sw&r=I7kF79XmGGZ9RLTiorMoewvoyduMXtLcRb1B2epe9cI&m=C_FrvfYh4GtqQesDyKDl4kau6xsDwvLGHAA0IZB9etE&s=mZcIhnGSnLca5JxJbOoKzoTiSuQQZmtyDo_eadoJHcw&e=
>> > > >
>> > > >>>> as such please follow our standard processes documented here:
>> > > >>>>
>> > https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute
>> > > >>>> <
>> > >
>> >
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__cwiki.apache.org_confluence_display_ZOOKEEPER_HowToContribute&d=DwMFaQ&c=C3yme8gMkxg_ihJNXS06ZyWk4EJm8LdrrvxQb-Je7sw&r=I7kF79XmGGZ9RLTiorMoewvoyduMXtLcRb1B2epe9cI&m=C_FrvfYh4GtqQesDyKDl4kau6xsDwvLGHAA0IZB9etE&s=vhQg3XrZS-o0UME9EYvtoCerG-ZNtJhTtbvo1M_MasM&e=
>> > >
>> > > -
>> > > >>>> the community will work with you to get the issue resolved.
>> > > >>>>
>> > > >>>> Regards,
>> > > >>>>
>> > > >>>> Patrick
>> > > >>>>
>> > > >>>>
>> > > >>>> On Fri, Aug 2, 2019 at 3:35 AM Fu, Xiaoqin <[email protected]>
>> > > 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);
>> > > >>>>> ......
>> > > >>>>> }
>> > > >>>>>
>> > > >>>>> Please help me confirm them and give them CVE IDs.
>> > > >>>>>
>> > > >>>>> Thank you very much!
>> > > >>>>> Yours sincerely
>> > > >>>>> Xiaoqin Fu
>> > > >>>>>
>> > > >>>>>
>> > >
>> >
>>
>
