Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic protection against untrusted deserialization by introducing blacklisting/whitelisting capabilities #12230

Merged
merged 1 commit into from May 15, 2018

Conversation

@kwart
Copy link
Contributor

commented Jan 31, 2018

Untrusted deserialization protection for Hazelcast

This pull request introduces java deserialization protection based on class names blacklisting and whitelisting.

The new feature is controlled by a new section in Hazelcast serialization configuration. The feature is not enabled by default, you can enable it by adding <java-serialization-filter/> element into <serialization/> configuration section.

Example:

<hazelcast>
    <serialization>
        <java-serialization-filter>
            <whitelist>
                <class>java.lang.String</class>
                <class>example.Foo</class>
                <package>com.acme.app</package>
                <package>com.acme.app.subpkg</package>
            </whitelist>
            <blacklist>
                <class>com.acme.app.BeanComparator</class>
            </blacklist>
        </java-serialization-filter>
    </serialization>
</hazelcast>

Once the feature is enabled, following filtering rules are used when objects are deserialized.

Filtering rules

  • When whitelist is not provided:
    1. if the deserialized object's getClass().getName() is blacklisted or getClass().getPackage().getName() is blacklisted, then deserialization fails;
    2. deserialization is allowed otherwise.
  • When whitelist is provided:
    1. if the deserialized object's getClass().getName() is blacklisted or getClass().getPackage().getName() is blacklisted, then deserialization fails;
    2. if the deserialized object's getClass().getName() is whitelisted or getClass().getPackage().getName() is whitelisted, then deserialization is allowed;
    3. deserialization fails otherwise.

Failed deserialization means a SecurityException is thrown.

When the blacklist is not explicitly provided, a default hardcoded value with some well known vulnerable class names is used.

The safest way to protect against untrusted deserialization is to use whitelisting, nevertheless it's also hard to maintain such a whitelist.

@kwart kwart added the Team: Core label Jan 31, 2018

@kwart kwart requested review from tkountis and mmedenjak Jan 31, 2018

@mmedenjak mmedenjak requested a review from pveentjer Jan 31, 2018

@kwart kwart force-pushed the kwart:deser-filtering branch 3 times, most recently from c86e877 to b95ae6f Feb 1, 2018

@kwart kwart changed the title Add protection against untrusted deserialization to the MulticastService [Don't merge] Add protection against untrusted deserialization to the MulticastService Feb 3, 2018

@kwart kwart force-pushed the kwart:deser-filtering branch 2 times, most recently from d480d78 to d9d2708 Feb 8, 2018

@kwart kwart changed the title [Don't merge] Add protection against untrusted deserialization to the MulticastService Add basic protection against untrusted deserialization by introducing blacklisting/whitelisting capabilities Feb 9, 2018

@kwart kwart added this to the 3.10 milestone Feb 9, 2018

@kwart kwart self-assigned this Feb 9, 2018

@kwart kwart force-pushed the kwart:deser-filtering branch 2 times, most recently from af2fb28 to d5e1c22 Feb 9, 2018

@Donnerbart
Copy link
Contributor

left a comment

I'm not sure about the config style, but this is already discussed in Slack, so we can see what the outcome is there.

I'm also not sure about the configuration parsing and check order. If I read the code correctly, the DEFAULT_BLACKLIST is overwritten as soon as I define a blacklist property. So I need to look into the code to add the defaults + my custom classes. Maybe adding the configured blacklist is better? But then the whitelist should overrule the blacklist, so you can override our blacklisted defaults.

}

/**
* Throws {@link ClassNotFoundException} if the given class name appears on the blacklist or does not appear on a non-empty

This comment has been minimized.

Copy link
@Donnerbart

Donnerbart Feb 13, 2018

Contributor

ClassNotFoundException doesn't seem a good fit here, since the class might be deployed, but still unwanted to be deserialized. Maybe HazelcastSerializationException?

ruleSysPropBlacklist.setOrClear("java.lang.Test3,java.lang.Test2,java.lang.Test1");
DeserializationChecker.checkClassNameForResolution("java.lang.Test1");
}
}

This comment has been minimized.

Copy link
@Donnerbart

Donnerbart Feb 13, 2018

Contributor

Missing newline at EOF

This comment has been minimized.

Copy link
@drosenbauer

drosenbauer Mar 9, 2018

Hi guys, original reporter here! If a whitelist is present, only classes on the union of all whitelists should be allowed. If no whitelist is present, classes not present on the union of all blacklists should be allowed. In both cases, any defaults should be merged with any additions by user configuration. The user should be required to explicitly remove items on the default blacklist.

For example, Weblogic allows a -class syntax in user filters which removes any matching classes or packages from the list.

I also agree that ClassNotFoundException isn't the right exception to throw here, because it doesn't communicate the correct information. The exception message back to the client should include something indicating that deserialization failed due to filtering, but not give too much information. If local logging is configured, the local log should include more information.

This comment has been minimized.

Copy link
@drosenbauer

drosenbauer Mar 9, 2018

Or more accurately:

If (a class matches a pattern in the whitelist or the whitelist is empty), and it does not also match a pattern in the blacklist, then it is allowed.

If a class matches a pattern in the blacklist, and it does not also match a pattern in the whitelist, then it is not allowed.

Else, it is allowed.

This allows whitelisting com.package.*, then disallowing specific classes in the package.

@kwart

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2018

@Donnerbart, Thanks for the comments, I'll work on them.

I'll rework the configuration to avoid using properties.

Unill the changes are in place, I'm flagging this PR with a "Don't merge" prefix.

@kwart kwart changed the title Add basic protection against untrusted deserialization by introducing blacklisting/whitelisting capabilities [Don't merge] Add basic protection against untrusted deserialization by introducing blacklisting/whitelisting capabilities Feb 13, 2018

@kwart kwart force-pushed the kwart:deser-filtering branch from 591d0e2 to d9ea1b4 Mar 8, 2018

@kwart

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2018

@drosenbauer Thanks for comments.

Current plan is to have following configuration:

<hazelcast>
    <serialization>
        <java-serialization-filter>
            <whitelist>
                <class>java.lang.String</class>
                <class>example.Foo</class>
                <package>com.acme.app</package>
                <package>com.acme.app.subpkg</package>
            </whitelist>
            <blacklist>
                <class>com.acme.app.BeanComparator</class>
            </blacklist>
        </java-serialization-filter>
    </serialization>
</hazelcast>

The exception, which will be thrown by look-ahead ObjectInputStream is java.lang.SecurityException. The deserializer will wrap it into HazelcastSerializationException.

Filtering rules

  • When whitelist is not provided:
    1. if the deserialized object's getClass().getName() is blacklisted or getClass().getPackage().getName() is blacklisted, then deserialization fails;
    2. deserialization is allowed otherwise.
  • When whitelist is provided:
    1. if the deserialized object's getClass().getName() is blacklisted or getClass().getPackage().getName() is blacklisted, then deserialization fails;
    2. if the deserialized object's getClass().getName() is whitelisted or getClass().getPackage().getName() is whitelisted, then deserialization is allowed;
    3. deserialization fails otherwise.

@kwart kwart force-pushed the kwart:deser-filtering branch from d9ea1b4 to 7da84ce Mar 28, 2018

@kwart kwart changed the title [Don't merge] Add basic protection against untrusted deserialization by introducing blacklisting/whitelisting capabilities Add basic protection against untrusted deserialization by introducing blacklisting/whitelisting capabilities Mar 28, 2018

@kwart kwart force-pushed the kwart:deser-filtering branch from 7da84ce to 23d8230 Mar 28, 2018

@kwart kwart requested a review from asimarslan Mar 28, 2018

@kwart kwart force-pushed the kwart:deser-filtering branch 3 times, most recently from 1c5a63e to 35030f1 Mar 28, 2018

@pveentjer
Copy link
Member

left a comment

Litter is created on the fast path for package checks. This is not acceptable from a performance point of view. So I'm blocking this PR from merging.

private final ShadeOfGreyList whitelist;

public SerializationClassNameFilter(JavaSerializationFilterConfig config) {
if (config == null) {

This comment has been minimized.

Copy link
@pveentjer

pveentjer Mar 29, 2018

Member

We have a preconditions utility class for such checks.

/**
* @param javaSerializationFilterConfig the javaSerializationFilterConfig to set
*/
public SerializationConfig setJavaSerializationFilterConfig(JavaSerializationFilterConfig javaSerializationFilterConfig) {

This comment has been minimized.

Copy link
@pveentjer

pveentjer Mar 30, 2018

Member

null check.

If a null filter is allowed, it should be documented.

@@ -125,6 +130,9 @@ public SerializationServiceBuilder setConfig(SerializationConfig config) {
enableCompression = config.isEnableCompression();
enableSharedObject = config.isEnableSharedObject();
allowUnsafe = config.isAllowUnsafe();
JavaSerializationFilterConfig filterConfig = config.getJavaSerializationFilterConfig();
classNameFilter = filterConfig == null ? SerializationClassNameFilter.DEFAULT_CLASSNAME_FILTER

This comment has been minimized.

Copy link
@pveentjer

pveentjer Mar 30, 2018

Member

This defaulting logic should be moved in the config. And in this case since always a filter is created, the config should not allow for a null javaSerializationFilterConfig to be returned.


package com.hazelcast.nio;

public interface ClassNameFilter {

This comment has been minimized.

Copy link
@pveentjer

pveentjer Mar 30, 2018

Member

Can you add documentation to this interface? I guess an exception is thrown when the class isn't allowed, but that isn't clear from the end user.

Also the com.hazelcast.nio package is a mixed bag of public and private code. If the end user shouldn't see this code, this logic should be placed to com.hazelcast.internal.serialization.

}

@Override
protected Class<?> resolveClass(ObjectStreamClass desc) throws ClassNotFoundException {
return ClassLoaderUtil.loadClass(classLoader, desc.getName());
String name = desc.getName();
if (classFilter != null) {

This comment has been minimized.

Copy link
@pveentjer

pveentjer Mar 30, 2018

Member

AFAIK it can't happen that for a regular created serializationservice, the classFilter can be null since if the config is null, a default config is created and used. So afaik this branch normally can't be null.. also there is no proper way to get rid of the check completely (the end user can't trigger the filter to become null).

return true;
}
if (packages != null) {
int dotPosition = className.lastIndexOf(".");

This comment has been minimized.

Copy link
@pveentjer

pveentjer Mar 30, 2018

Member

Litter (the substring) is being created on the fast path. This isn't acceptable.

A potential solution:
1: determine if the class with this package is allowed; so you create your substring. This will only be done once per class.
2: once you have determined the class with this package is valid, replace the classes set by current set with the new class added. So there will be some litter while the system is learning about the classes which are valid, but after that there is no litter.

The only form of synchronization that is needed is to make the classes field volatile and make sure you update the classes field with a cas. the HashSet of classes can remain immutable. Another similar approach would be to use a mutable threadsafe (based on ConcurrentHashMap) set.

@sancar sancar added the Team: Client label Mar 30, 2018

@pveentjer

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

This task has been moved to the 3.11 release. So I'm going to update the milestone.

@pveentjer pveentjer modified the milestones: 3.10, 3.11 Apr 5, 2018

@kwart kwart force-pushed the kwart:deser-filtering branch from 35030f1 to 3ad4e4d Apr 27, 2018

@kwart kwart changed the title Add basic protection against untrusted deserialization by introducing blacklisting/whitelisting capabilities [WIP] Add basic protection against untrusted deserialization by introducing blacklisting/whitelisting capabilities Apr 30, 2018

@kwart kwart force-pushed the kwart:deser-filtering branch 3 times, most recently from 7f673cb to 4169cb1 Apr 30, 2018

@kwart

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2018

I've updated the PR. The comments were mostly addressed.

The feature is not enabled by default now. The default blacklist is used after adding the <java-serialization-filter/> element.

I don't think the package name extraction from a class name is a litter, I like it more than adding classnames without limit to the classname Set, or using an overkill like a cache with a proper eviction mechanism. As the feature is not enabled by default, I think we are safe here now from the performance PoV. Do you agree @pveentjer?

@kwart kwart changed the title [WIP] Add basic protection against untrusted deserialization by introducing blacklisting/whitelisting capabilities Add basic protection against untrusted deserialization by introducing blacklisting/whitelisting capabilities May 2, 2018

@kwart kwart force-pushed the kwart:deser-filtering branch from 4169cb1 to c1a03e6 May 2, 2018

if (!packages.isEmpty()) {
int dotPosition = className.lastIndexOf(".");
if (dotPosition > 0) {
String packageName = className.substring(0, dotPosition);

This comment has been minimized.

Copy link
@pveentjer

pveentjer May 15, 2018

Member

This will lead to litter. You can optimize it by storing the clasname in the 'classes'. So this substring check will be done one per class and then it will be stored in 'classes'. Then you need to make classes modifiable by using either a CHM or replace the whole collection and consider it immutable.

This is the second time I placed this comment.

This comment has been minimized.

Copy link
@kwart

kwart May 15, 2018

Author Contributor

This is the second time I object the suggestion for adding classes to a Set if their packages are listed. It would create another unsafe piece in the code - possible DOS by causing OOM.

This comment has been minimized.

Copy link
@Donnerbart

Donnerbart May 15, 2018

Contributor

Hmm, can't we settle this issue with a JFR test? The current implementation vs. a cached one. And a normal usage scenario vs. a DOS based one (so a handful of classes vs. an attack based with thousands of generated classes). Would be four scenarios in total to compare.

EDIT: Sorry, six scenarios, we should include the current master of course, to see the impact of the feature.

This comment has been minimized.

Copy link
@kwart

kwart May 15, 2018

Author Contributor

👍 I'm going to put together a JMH test.

This comment has been minimized.

Copy link
@pveentjer

pveentjer May 15, 2018

Member
package com.hazelcast;

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.infra.Blackhole;

import java.util.concurrent.TimeUnit;


@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Thread)
@Fork(value = 1, warmups = 1)
@Warmup(iterations = 2)
@Measurement(iterations = 5)
public class ClassFilterBenchmark {

    private ClassFilter classFilter;

    @Setup
    public void prepare() {
        classFilter = new ClassFilter();
        classFilter.addPackages("foo");
    }

    @Benchmark
    public void litter(Blackhole blackhole) {
        blackhole.consume(classFilter.isListed_litter("foo.bar"));
    }

    @Benchmark
    public void noLitter(Blackhole blackhole) {
        blackhole.consume(classFilter.isListed_noLitter("foo.bar"));
    }
}

Benchmark                      Mode  Cnt   Score   Error  Units
ClassFilterBenchmark.litter    avgt    5  30.205 ± 7.995  ns/op
ClassFilterBenchmark.noLitter  avgt    5   6.386 ± 0.091  ns/op
package com.hazelcast;

import static com.hazelcast.util.Preconditions.checkNotNull;
import static java.util.Collections.unmodifiableSet;

import java.util.Collection;
import java.util.Collections;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

/**
 * Holds blacklist and whitelist configuration in java deserialization configuration.
 */
public class ClassFilter {

    private final Set<String> classes = Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>());
    private final Set<String> packages = Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>());

    /**
     * Returns unmodifiable set of class names.
     */
    public Set<String> getClasses() {
        return unmodifiableSet(classes);
    }

    /**
     * Returns unmodifiable set of package names.
     */
    public Set<String> getPackages() {
        return unmodifiableSet(packages);
    }

    public ClassFilter addClasses(String... names) {
        checkNotNull(names);
        for (String name : names) {
            classes.add(name);
        }
        return this;
    }

    public ClassFilter setClasses(Collection<String> names) {
        checkNotNull(names);
        classes.clear();
        classes.addAll(names);
        return this;
    }

    public ClassFilter addPackages(String... names) {
        checkNotNull(names);
        for (String name : names) {
            packages.add(name);
        }
        return this;
    }

    public ClassFilter setPackages(Collection<String> names) {
        checkNotNull(names);
        packages.clear();
        packages.addAll(names);
        return this;
    }

    public boolean isEmpty() {
        return classes.isEmpty() && packages.isEmpty();
    }

    public boolean isListed_litter(String className) {
        if (classes.contains(className)) {
            return true;
        }
        if (!packages.isEmpty()) {
            int dotPosition = className.lastIndexOf(".");
            if (dotPosition > 0) {
                String packageName = className.substring(0, dotPosition);
                return packages.contains(packageName);
            }
        }
        return false;
    }

    public boolean isListed_noLitter(String className) {
        if (classes.contains(className)) {
            return true;
        }

        if (!packages.isEmpty()) {
            int dotPosition = className.lastIndexOf(".");
            if (dotPosition > 0) {
                String packageName = className.substring(0, dotPosition);

                if(packages.contains(packageName)){
                    classes.add(className);
                    return true;
                }
            }
        }
        return false;
    }

    @Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        result = prime * result + ((classes == null) ? 0 : classes.hashCode());
        result = prime * result + ((packages == null) ? 0 : packages.hashCode());
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj) {
            return true;
        }
        if (obj == null || getClass() != obj.getClass()) {
            return false;
        }
        ClassFilter other = (ClassFilter) obj;
        return ((classes == null && other.classes == null) || (classes != null && classes.equals(other.classes)))
                && ((packages == null && other.packages == null) || (packages != null && packages.equals(other.packages)));
    }

    @Override
    public String toString() {
        return "ClassFilter{classes=" + classes + ", packages=" + packages + "}";
    }

}

This comment has been minimized.

Copy link
@pveentjer

pveentjer May 15, 2018

Member

I added the gc profiler

Benchmark                                                     Mode  Cnt     Score     Error   Units
ClassFilterBenchmark.litter                                   avgt    5    29.432 ±   1.990   ns/op
ClassFilterBenchmark.litter:·gc.alloc.rate                    avgt    5  1555.331 ± 103.111  MB/sec
ClassFilterBenchmark.litter:·gc.alloc.rate.norm               avgt    5    48.000 ±   0.001    B/op
ClassFilterBenchmark.litter:·gc.churn.PS_Eden_Space           avgt    5  1550.413 ± 577.469  MB/sec
ClassFilterBenchmark.litter:·gc.churn.PS_Eden_Space.norm      avgt    5    47.876 ±  18.812    B/op
ClassFilterBenchmark.litter:·gc.churn.PS_Survivor_Space       avgt    5     0.181 ±   0.231  MB/sec
ClassFilterBenchmark.litter:·gc.churn.PS_Survivor_Space.norm  avgt    5     0.006 ±   0.007    B/op
ClassFilterBenchmark.litter:·gc.count                         avgt    5    21.000            counts
ClassFilterBenchmark.litter:·gc.time                          avgt    5    43.000                ms
ClassFilterBenchmark.noLitter                                 avgt    5     6.365 ±   0.122   ns/op
ClassFilterBenchmark.noLitter:·gc.alloc.rate                  avgt    5     0.001 ±   0.001  MB/sec
ClassFilterBenchmark.noLitter:·gc.alloc.rate.norm             avgt    5    ≈ 10⁻⁵              B/op
ClassFilterBenchmark.noLitter:·gc.count                       avgt    5       ≈ 0            counts

@kwart kwart force-pushed the kwart:deser-filtering branch from c1a03e6 to 57944cb May 15, 2018

@codecov

This comment has been minimized.

Copy link

commented May 15, 2018

Codecov Report

Merging #12230 into master will increase coverage by 0.2%.
The diff coverage is 80.4%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #12230     +/-   ##
===========================================
+ Coverage     76.22%   76.43%   +0.2%     
- Complexity    34722    34868    +146     
===========================================
  Files          3032     3035      +3     
  Lines        129595   129819    +224     
  Branches      15168    15203     +35     
===========================================
+ Hits          98785    99227    +442     
+ Misses        25073    24913    -160     
+ Partials       5737     5679     -58
Impacted Files Coverage Δ Complexity Δ
...rialization/impl/AbstractSerializationService.java 87.59% <100%> (+0.58%) 86 <0> (-1) ⬇️
...om/hazelcast/nio/SerializationClassNameFilter.java 100% <100%> (ø) 5 <5> (?)
...ation/impl/DefaultSerializationServiceBuilder.java 81.86% <100%> (+1.64%) 56 <0> (+1) ⬆️
...nal/serialization/impl/JavaDefaultSerializers.java 86.33% <100%> (+0.19%) 1 <0> (ø) ⬇️
...java/com/hazelcast/config/SerializationConfig.java 89.02% <100%> (+0.41%) 43 <2> (+2) ⬆️
...rc/main/java/com/hazelcast/config/ClassFilter.java 52.83% <52.83%> (ø) 17 <17> (?)
...azelcast/config/JavaSerializationFilterConfig.java 53.84% <53.84%> (ø) 8 <8> (?)
...elcast/src/main/java/com/hazelcast/nio/IOUtil.java 73.12% <66.66%> (-0.32%) 78 <1> (ø)
.../com/hazelcast/config/AbstractXmlConfigHelper.java 70.41% <86.95%> (+1.66%) 71 <6> (+7) ⬆️
.../spring/AbstractHazelcastBeanDefinitionParser.java 86.82% <88.46%> (+0.46%) 1 <0> (ø) ⬇️
... and 119 more
@kwart

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2018

Thanks @pveentjer for review and pushing for the solution with better performance!

I'm going to squash the commits.

@kwart kwart force-pushed the kwart:deser-filtering branch from 57944cb to c1c3135 May 15, 2018

@kwart kwart merged commit 5a47697 into hazelcast:master May 15, 2018

1 check failed

default Test `FAIL`ed. https://hazelcast-l337.ci.cloudbees.com/job/new-lab-fast-pr/15605/
Details

@kwart kwart deleted the kwart:deser-filtering branch Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.