#493: Checking access to all methods of unsafe classes

[michalstodolny]

No description provided.

[ebussieres]

I fear that it's gonna affect the performance since all call will add a check using instanceof. Can you run a benchmark with current version and with your modification please ? https://github.com/PebbleTemplates/pebble-performance-test

Also, I think that we can remove the current code which is using a list of blacklisted methods in MemberCacheUtils class.

[michalstodolny]

I fear that it's gonna affect the performance since all call will add a check using instanceof. Can you run a benchmark with current version and with your modification please ? https://github.com/PebbleTemplates/pebble-performance-test

Also, I think that we can remove the current code which is using a list of blacklisted methods in MemberCacheUtils class.

Thank you for your comment.
I checked the performance tests and could not observe any noticeable differences.
instanceof operator should not degrade performance as far as I know.

I agree when it comes to removal of the current blacklist implementation.
Although I moved the code to test sources as it makes unit tests easier to write when methods are defined in properties files.

[ebussieres]

I ran a benchmark and I have a performance drop with your modifications (almost 10%) :

Before:
Benchmark Mode Cnt Score Error Units
Pebble.benchmark thrpt 50 28681,314 ± 1144,635 ops/s

After
Benchmark Mode Cnt Score Error Units
Pebble.benchmark thrpt 50 26229,409 ± 471,470 ops/s

[bfitgar]

Hi - Are there any plans to merge this into master? If not, bigger question would be are there any plans to mitigate CVE-2019-19899 in Pebble?

[ebussieres]

There's a performance drop but I think that's the cost for security. I'll take a second look at it

[slandelle]

@ebussieres Are you sure going with a blacklist strategy is fine? Jackson was doing this for years and finally switched to a whitelist strategy in 2.10.

[ebussieres]

@slandelle : Can you elaborate on your whitelist strategy ? Did you mean to check the package of the class beeing called and check if it's in the whitelist ?

[bfitgar]

Hi all - not sure if PR approval is needed - whats the next steps to getting Michal's commits merged into master? This would hopefully complete all issues assoc with 3.1.4 and could be released - is that correct Eric?

[ebussieres]

I'm waiting a reply from @slandelle. Otherwise, i'll merge it. Anyway, I'll probably release v3.1.4 in 2-3 weeks.

[slandelle]

@ebussieres My concern is that you'll end up patching this backlist of classes and methods over and over again because there's a hack you hadn't thought.

IMHO, users should be responsible for configuring which classes and methods are safe for their use case and add them in a whitelist. By default, this list would be empty and you can't call any arbitrary method. My2cents.

[cjbrooks12]

If MethodAccessValidator is turned into an interface, then it could be provided in the engine builder. The default could be an empty whitelist (secure by default), but this blacklist strategy could be provided as an easy option for users not needing that level of security. And of course, users can write their own implementation to customize their own whitelists.

[ebussieres]

@slandelle @cjbrooks12 I started working on a whitelist strategy and it's kind of hard to implement too. You need to whitelist some classes in the jdk (List, Map etc.), a lot of classes in pebble core too.

I started working with package whitelisting that you provide to the pebbleEngine but we need to have some method granularity to block methods like getClass but allow method like equals/hashCode/toString.

Any thoughts ?

[ebussieres]

@slandelle @cjbrooks12 I made another PR with a MethodAccessValidator interface. I made a NoOp method validator to bypass security check and a Default one with the implementation that was made in this PR.

If you can take a look, that would be appreciated (#511)

[cjbrooks12]

Sure, I'll take a look at it tonight or tomorrow

[cjbrooks12]

@ebussieres I've been playing around with PR #511, and the functionality is looking good. Running it with Orchid to generate its docs is seeing about a 10% decrease in performance between the no-op and blacklist implementations, all other things equal. Here's a table of the test runs:

no-op mean	no-op median	blacklist mean	blacklist median
112ms	49ms	113ms	55ms
108ms	50ms	115ms	56ms
108ms	52ms	115ms	54ms
132ms	51ms	150ms	57ms
109ms	51ms	130ms	65ms

9.1% difference (mean)

12.6% difference (median)

[slandelle]

All those classes can only be defined in the JDK. Just wondering if if would speeding things to use class identity equality:

Class<?> clazz = object.getClass();
return clazz == Class.class || clazz == Runtime.class etc

[ebussieres]

Performance seems to be the same when running with https://github.com/PebbleTemplates/pebble-performance-test

instanceof results:

Benchmark          Mode  Cnt      Score     Error  Units
Pebble.benchmark  thrpt   50  29929,876 ± 220,974  ops/s

class identity quality:

Benchmark          Mode  Cnt      Score     Error  Units
Pebble.benchmark  thrpt   50  30196,533 ± 409,231  ops/s

But if I use this method, I need to add some reflect class to the list like this, otherwise some tests are failing

Class<?> clazz = object.getClass();
    boolean methodForbidden = clazz == Class.class
        || clazz == Runtime.class
        || clazz == Thread.class
        || clazz == ThreadGroup.class
        || clazz == System.class
        || clazz == AccessibleObject.class
        || clazz == Method.class
        || clazz == Field.class
        || clazz == Constructor.class
        || this.isUnsafeMethod(method);

[ebussieres]

I'll close this one in favor of #511