pentaho / pentaho-platform

Pentaho BA Server Core
http://www.pentaho.com
Other
472 stars 723 forks source link

[PPP-5105] Denial Of Service when changing file ACLs #5624

Closed befc closed 5 months ago

befc commented 5 months ago

Refactored the code according to the suggestion made in the ticket's description by @dcleao. This will effectively correct the performance issue with the previous implementation by using the regex without wildcards and make usage of the find() method, that performs a lot better than matches().

The target code is being covered by FileResourceTest.validateSecurityPrincipal(), from my evaluation, no further UTs seem to be necessary with this change.

Conducted some benchmark tests with JMH to better understand the impact and improvements with this change for both valid and invalid strings, before and after refactor: Short strings (~10 char.):

Benchmark                                                       Mode  Cnt    Score    Error  Units
SecurityPrincipalValidatorBenchmark.benchmarkOriginalInvalid    avgt   25  199.008 ± 15.979  ns/op
SecurityPrincipalValidatorBenchmark.benchmarkOriginalValid      avgt   25  115.592 ±  5.005  ns/op
SecurityPrincipalValidatorBenchmark.benchmarkRefactoredInvalid  avgt   25   51.211 ±  1.235  ns/op
SecurityPrincipalValidatorBenchmark.benchmarkRefactoredValid    avgt   25   58.705 ±  0.698  ns/op

Long string (1000 char.):

Benchmark                                                       Mode  Cnt     Score     Error  Units
SecurityPrincipalValidatorBenchmark.benchmarkOriginalInvalid    avgt   25  1333.379 ±  47.356  ns/op
SecurityPrincipalValidatorBenchmark.benchmarkOriginalValid      avgt   25  3557.988 ± 114.289  ns/op
SecurityPrincipalValidatorBenchmark.benchmarkRefactoredInvalid  avgt   25   713.302 ±  21.342  ns/op
SecurityPrincipalValidatorBenchmark.benchmarkRefactoredValid    avgt   25   791.151 ±  12.725  ns/op
hitachivantarasonarqube[bot] commented 5 months ago

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

buildguy commented 5 months ago
[![👍 Frogbot scanned this pull request and found that it did not add vulnerable dependencies.](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/noVulnerabilityBannerPR.png)](https://docs.jfrog-applications.jfrog.io/jfrog-applications/frogbot)
Note: ---
**Frogbot** also supports **Contextual Analysis, Secret Detection, IaC and SAST Vulnerabilities Scanning**. This features are included as part of the [JFrog Advanced Security](https://jfrog.com/advanced-security) package, which isn't enabled on your system.

[🐸 JFrog Frogbot](https://docs.jfrog-applications.jfrog.io/jfrog-applications/frogbot)
buildguy commented 5 months ago

:white_check_mark: Build finished in 28m 50s

Build command:

mvn clean verify -B -e -Daudit -Djs.no.sandbox -pl extensions

:ok_hand: All tests passed!

Tests run: 1563, Failures: 0, Skipped: 5    Test Results


:information_source: This is an automatic message