Hi Xiaoqin,
I agree that we have a lot of unnecessary log guards that can be removed
(even where there are string concatenations, as the parameterized '{}' log
function solves the performance problem.
There is a checkstyle patch undergoing which fixes some of this. [1]
I also agree we should be careful with what we log. I even had an issue
recently where debugging authentication problem was hard, due to few logs
even in debug level.
I personally don't see socket info and config dir critical data, because if
someone has access to the logs it is very easy to find out these
information.
And as far as I know, the log guards has no security responsibility
whatsoever, it won't prevent any information leakage. For example, putting
a debug log guard for a debug log level message does nothing in this
manner, as if we are lower then debug level, the message stays within the
process, nothing will be printed anywhere. If debug level is on, it will be
printed to IO anyway.
Regards,
Norbert
[1] https://github.com/apache/zookeeper/pull/1049
On Tue, Aug 13, 2019 at 10:28 AM Xiaoqin Fu <[email protected]> wrote:
> 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
> >> > > >>>>>
> >> > > >>>>>
> >> > >
> >> >
> >>
> >
>
