Artem Smotrakov created SLING-10700:
---------------------------------------
Summary: Improve TopologyRequestValidator code
Key: SLING-10700
URL: https://issues.apache.org/jira/browse/SLING-10700
Project: Sling
Issue Type: Improvement
Reporter: Artem Smotrakov
Attachments: TopologyRequestValidator.patch
(agreed to open a Jira issue after discussing this on [email protected])
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)
