[jira] [Resolved] (SLING-10700) Improve TopologyRequestValidator code

["Stefan Egli (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/SLING-10700?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Stefan Egli resolved SLING-10700.
---------------------------------
    Resolution: Fixed

* [PR|https://github.com/apache/sling-org-apache-sling-discovery-base/pull/5] merged
* Thx [~asmotrakov] for this patch!

> Improve TopologyRequestValidator code
> -------------------------------------
>
>                 Key: SLING-10700
>                 URL: https://issues.apache.org/jira/browse/SLING-10700
>             Project: Sling
>          Issue Type: Improvement
>          Components: Discovery
>    Affects Versions: Discovery Base 2.0.10
>            Reporter: Artem Smotrakov
>            Assignee: Stefan Egli
>            Priority: Major
>              Labels: security
>             Fix For: Discovery Base 2.0.12
>
>         Attachments: TopologyRequestValidator.patch
>
>          Time Spent: 1h
>  Remaining Estimate: 0h
>
> (agreed to open a Jira issue after discussing this on security@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)