[
https://issues.apache.org/jira/browse/SLING-10700?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17392450#comment-17392450
]
Artem Smotrakov commented on SLING-10700:
-----------------------------------------
The following pull request address the issues above
https://github.com/apache/sling-org-apache-sling-discovery-base/pull/5
> Improve TopologyRequestValidator code
> -------------------------------------
>
> Key: SLING-10700
> URL: https://issues.apache.org/jira/browse/SLING-10700
> Project: Sling
> Issue Type: Improvement
> Reporter: Artem Smotrakov
> Priority: Major
> Attachments: TopologyRequestValidator.patch
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> (agreed to open a Jira issue after discussing this on secru...@apache.org)
>
> I noticed several security problems in TopologyRequestValidator
>
> [https://github.com/apache/sling-org-apache-sling-discovery-base/blob/df08b0dfa40feaed4f0fd1575d688691a7a0d946/src/main/java/org/apache/sling/discovery/base/connectors/ping/TopologyRequestValidator.java]
>
> They don't look severe but it may be worth addressing them.
>
> 1. The class uses String.equals() for validating a MAC. This method doesn't
> implement a constant-time algorithm for comparing inputs. As a result, it may
> allow an attacker to run a timing attack that can uncover a valid MAC.
> Instead, it would be better to use MessageDigest.isEquals()
>
> [https://github.com/apache/sling-org-apache-sling-discovery-base/blob/df08b0dfa40feaed4f0fd1575d688691a7a0d946/src/main/java/org/apache/sling/discovery/base/connectors/ping/TopologyRequestValidator.java#L367]
>
> 2. The method getCipherKey() uses PBKDF2 for generating a key for MAC. NIST
> recommends to use a higher iteration count
>
> [https://pages.nist.gov/800-63-3/sp800-63b.html#sec5]
>
> Furthermore, it might be better to use SHA-256 (or even better SHA-512). That
> would increase the cost of an attack.
>
> 3. It might be better to close GZIPInputStream in a try-finally block to
> avoid unreleased streams in case an exception occurs. Unclosed streams may
> help to run a DoS attack. I didn't look into the implementation of
> GZIPInputStream though.
>
> At first I thought the the class uses a static IV for a cipher
>
> [https://github.com/apache/sling-org-apache-sling-discovery-base/blob/df08b0dfa40feaed4f0fd1575d688691a7a0d946/src/main/java/org/apache/sling/discovery/base/connectors/ping/TopologyRequestValidator.java#L466]
>
> but debugging TopologyRequestValidatorTest showed that the IV is actually
> random. Well, at least when you use the latest JDK 8. To be on the safer
> side, it may be better to set a random IV explicitly.
>
> I am attaching a draft patch that addresses most of the points above (except
> moving to SHA-512 and setting an explicit IV).
>
> Please feel free to use this patch, or I can open a pull request if it is
> fine. You can also create a private branch and a security advisory on GitHub,
> then I can open a pull request against that branch.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
