Fix CVE-2021-25646 (#10818)

This commit is contained in:
Jihoon Son 2021-02-04 11:21:43 -08:00 committed by GitHub
parent 397e7455ba
commit 3f8f00a231
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 521 additions and 5 deletions

View File

@ -19,8 +19,10 @@
package org.apache.druid.guice;
import com.fasterxml.jackson.databind.AnnotationIntrospector;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.introspect.AnnotationIntrospectorPair;
import com.google.common.annotations.VisibleForTesting;
import com.google.inject.Binder;
import com.google.inject.Inject;
import com.google.inject.Injector;
@ -83,14 +85,28 @@ public class DruidSecondaryModule implements Module
return smileMapper;
}
private void setupJackson(Injector injector, final ObjectMapper mapper)
@VisibleForTesting
public static void setupJackson(Injector injector, final ObjectMapper mapper)
{
final GuiceAnnotationIntrospector guiceIntrospector = new GuiceAnnotationIntrospector();
mapper.setInjectableValues(new GuiceInjectableValues(injector));
setupAnnotationIntrospector(mapper, new GuiceAnnotationIntrospector());
}
@VisibleForTesting
public static void setupAnnotationIntrospector(
final ObjectMapper mapper,
final AnnotationIntrospector annotationIntrospector
)
{
mapper.setAnnotationIntrospectors(
new AnnotationIntrospectorPair(guiceIntrospector, mapper.getSerializationConfig().getAnnotationIntrospector()),
new AnnotationIntrospectorPair(guiceIntrospector, mapper.getDeserializationConfig().getAnnotationIntrospector())
new AnnotationIntrospectorPair(
annotationIntrospector,
mapper.getSerializationConfig().getAnnotationIntrospector()
),
new AnnotationIntrospectorPair(
annotationIntrospector,
mapper.getDeserializationConfig().getAnnotationIntrospector()
)
);
}
}

View File

@ -20,8 +20,12 @@
package org.apache.druid.guice;
import com.fasterxml.jackson.annotation.JacksonInject;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.introspect.Annotated;
import com.fasterxml.jackson.databind.introspect.AnnotatedMember;
import com.fasterxml.jackson.databind.introspect.AnnotatedMethod;
import com.fasterxml.jackson.databind.introspect.AnnotatedParameter;
import com.fasterxml.jackson.databind.introspect.NopAnnotationIntrospector;
import com.google.inject.BindingAnnotation;
import com.google.inject.Key;
@ -56,4 +60,54 @@ public class GuiceAnnotationIntrospector extends NopAnnotationIntrospector
}
return Key.get(m.getGenericType(), guiceAnnotation);
}
/**
* This method is used to find what property to ignore in deserialization. Jackson calls this method
* per every class and every constructor parameter.
*
* This implementation returns a {@link JsonIgnoreProperties.Value#empty()} that allows empty names if
* the parameters has the {@link JsonProperty} annotation. Otherwise, it returns
* {@code JsonIgnoreProperties.Value.forIgnoredProperties("")} that does NOT allow empty names.
* This behavior is to work around a bug in Jackson deserializer (see the below comment for details) and
* can be removed in the future after the bug is fixed.
* For example, suppose a constructor like below:
*
* <pre>{@code
* @JsonCreator
* public ClassWithJacksonInject(
* @JsonProperty("val") String val,
* @JacksonInject InjectedParameter injected
* )
* }</pre>
*
* During deserializing a JSON string into this class, this method will be called at least twice,
* one for {@code val} and another for {@code injected}. It will return {@code Value.empty()} for {@code val},
* while {Value.forIgnoredProperties("")} for {@code injected} because the later does not have {@code JsonProperty}.
* As a result, {@code injected} will be ignored during deserialization since it has no name.
*/
@Override
public JsonIgnoreProperties.Value findPropertyIgnorals(Annotated ac)
{
// We should not allow empty names in any case. However, there is a known bug in Jackson deserializer
// with ignorals (_arrayDelegateDeserializer is not copied when creating a contextual deserializer.
// See https://github.com/FasterXML/jackson-databind/issues/3022 for more details), which makes array
// deserialization failed even when the array is a valid field. To work around this bug, we return
// an empty ignoral when the given Annotated is a parameter with JsonProperty that needs to be deserialized.
// This is valid because every property with JsonProperty annoation should have a non-empty name.
// We can simply remove the below check after the Jackson bug is fixed.
//
// This check should be fine for so-called delegate creators that have only one argument without
// JsonProperty annotation, because this method is not even called for the argument of
// delegate creators. I'm not 100% sure why it's not called, but guess it's because the argument
// is some Java type that Jackson already knows how to deserialize. Since there is only one argument,
// Jackson perhaps is able to just deserialize it without introspection.
if (ac instanceof AnnotatedParameter) {
final AnnotatedParameter ap = (AnnotatedParameter) ac;
if (ap.hasAnnotation(JsonProperty.class)) {
return JsonIgnoreProperties.Value.empty();
}
}
return JsonIgnoreProperties.Value.forIgnoredProperties("");
}
}

View File

@ -19,9 +19,12 @@
package org.apache.druid.data.input.impl;
import com.fasterxml.jackson.databind.AnnotationIntrospector;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import junit.framework.Assert;
import org.apache.druid.guice.DruidSecondaryModule;
import org.apache.druid.guice.GuiceAnnotationIntrospector;
import org.junit.Test;
import java.util.Arrays;
@ -34,6 +37,8 @@ public class DimensionsSpecSerdeTest
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
static {
AnnotationIntrospector introspector = new GuiceAnnotationIntrospector();
DruidSecondaryModule.setupAnnotationIntrospector(OBJECT_MAPPER, introspector);
OBJECT_MAPPER.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
}

View File

@ -0,0 +1,63 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.druid.data.input.impl;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.AnnotationIntrospector;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.apache.druid.data.input.impl.DimensionSchema.MultiValueHandling;
import org.apache.druid.guice.DruidSecondaryModule;
import org.apache.druid.guice.GuiceAnnotationIntrospector;
import org.junit.Assert;
import org.junit.Test;
public class StringDimensionSchemaTest
{
private final ObjectMapper jsonMapper;
public StringDimensionSchemaTest()
{
jsonMapper = new ObjectMapper();
AnnotationIntrospector introspector = new GuiceAnnotationIntrospector();
DruidSecondaryModule.setupAnnotationIntrospector(jsonMapper, introspector);
jsonMapper.registerSubtypes(StringDimensionSchema.class);
}
@Test
public void testDeserializeFromSimpleString() throws JsonProcessingException
{
final String json = "\"dim\"";
final StringDimensionSchema schema = (StringDimensionSchema) jsonMapper.readValue(json, DimensionSchema.class);
Assert.assertEquals(new StringDimensionSchema("dim"), schema);
}
@Test
public void testDeserializeFromJson() throws JsonProcessingException
{
final String json = "{\n"
+ " \"type\" : \"StringDimensionSchema\",\n"
+ " \"name\" : \"dim\",\n"
+ " \"multiValueHandling\" : \"SORTED_SET\",\n"
+ " \"createBitmapIndex\" : false\n"
+ "}";
final StringDimensionSchema schema = (StringDimensionSchema) jsonMapper.readValue(json, DimensionSchema.class);
Assert.assertEquals(new StringDimensionSchema("dim", MultiValueHandling.SORTED_SET, false), schema);
}
}

View File

@ -0,0 +1,353 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.druid.guice;
import com.fasterxml.jackson.annotation.JacksonInject;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonValue;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.collect.ImmutableList;
import com.google.inject.Guice;
import com.google.inject.Injector;
import com.google.inject.Module;
import org.junit.Assert;
import org.junit.Test;
import org.junit.experimental.runners.Enclosed;
import org.junit.runner.RunWith;
import javax.annotation.Nullable;
import javax.validation.Validation;
import javax.validation.Validator;
import java.util.List;
import java.util.Properties;
@RunWith(Enclosed.class)
public class DruidSecondaryModuleTest
{
private static final String PROPERTY_NAME = "druid.injected.val";
private static final String PROPERTY_VALUE = "this is the legit val";
public static class ConstructorWithJacksonInjectTest
{
@Test
public void testInjectWithAnEmptyPropertyNotOverrideInjection() throws JsonProcessingException
{
final Properties props = new Properties();
props.setProperty(PROPERTY_NAME, PROPERTY_VALUE);
final Injector injector = makeInjectorWithProperties(props);
final ObjectMapper mapper = makeObjectMapper(injector);
final String json = "{\"test\": \"this is an injection test\", \"\": \"nice try\" }";
final ClassWithJacksonInject object = mapper.readValue(json, ClassWithJacksonInject.class);
Assert.assertEquals("this is an injection test", object.test);
Assert.assertEquals(PROPERTY_VALUE, object.injected.val);
}
@Test
public void testInjectNormal() throws JsonProcessingException
{
final Properties props = new Properties();
props.setProperty(PROPERTY_NAME, PROPERTY_VALUE);
final Injector injector = makeInjectorWithProperties(props);
final ObjectMapper mapper = makeObjectMapper(injector);
final String json = "{\"test\": \"this is an injection test\" }";
final ClassWithJacksonInject object = mapper.readValue(json, ClassWithJacksonInject.class);
Assert.assertEquals("this is an injection test", object.test);
Assert.assertEquals(PROPERTY_VALUE, object.injected.val);
}
@Test
public void testInjectClassWithEmptyProperty() throws JsonProcessingException
{
final Properties props = new Properties();
props.setProperty(PROPERTY_NAME, PROPERTY_VALUE);
final Injector injector = makeInjectorWithProperties(props);
final ObjectMapper mapper = makeObjectMapper(injector);
final String json = "{\"test\": \"this is an injection test\", \"\": \"nice try\" }";
final ClassWithEmptyProperty object = mapper.readValue(json, ClassWithEmptyProperty.class);
Assert.assertEquals("this is an injection test", object.test);
Assert.assertEquals(PROPERTY_VALUE, object.injected.val);
}
private static class ClassWithJacksonInject
{
private final String test;
private InjectedParameter injected;
@JsonCreator
public ClassWithJacksonInject(
@JsonProperty("test") String test,
@JacksonInject InjectedParameter injected
)
{
this.test = test;
this.injected = injected;
}
@JsonProperty
public String getTest()
{
return test;
}
}
private static class ClassWithEmptyProperty
{
private final String test;
private InjectedParameter injected;
@JsonCreator
public ClassWithEmptyProperty(
@JsonProperty("test") String test,
@JacksonInject @JsonProperty("") InjectedParameter injected
)
{
this.test = test;
this.injected = injected;
}
@JsonProperty
public String getTest()
{
return test;
}
}
}
public static class ConstructorWithoutJacksonInjectTest
{
@Test
public void testInjectionWithEmptyPropertyName() throws JsonProcessingException
{
final Properties props = new Properties();
final Injector injector = makeInjectorWithProperties(props);
final ObjectMapper mapper = makeObjectMapper(injector);
final String json = "[\"this is\", \"an injection test\"]";
final ClassWithConstructorOfEmptyName object = mapper.readValue(json, ClassWithConstructorOfEmptyName.class);
Assert.assertEquals(ImmutableList.of("this is", "an injection test"), object.getTest());
}
@Test
public void testInjectEmptyListWithEmptyPropertyName() throws JsonProcessingException
{
final Properties props = new Properties();
final Injector injector = makeInjectorWithProperties(props);
final ObjectMapper mapper = makeObjectMapper(injector);
final String json = "[]";
final ClassWithConstructorOfEmptyName object = mapper.readValue(json, ClassWithConstructorOfEmptyName.class);
Assert.assertEquals(ImmutableList.of(), object.getTest());
}
@Test
public void testInjectClassWithFactoryMethodOfEmptyPropertyName() throws JsonProcessingException
{
final Properties props = new Properties();
final Injector injector = makeInjectorWithProperties(props);
final ObjectMapper mapper = makeObjectMapper(injector);
final String json = "[\"this is\", \"an injection test\"]";
final ClassWithFactoryMethodOfEmptyName object = mapper.readValue(json, ClassWithFactoryMethodOfEmptyName.class);
Assert.assertEquals(ImmutableList.of("this is", "an injection test"), object.getTest());
}
@Test
public void testInjectEmptyListToClassWithFactoryMethodOfEmptyPropertyName() throws JsonProcessingException
{
final Properties props = new Properties();
final Injector injector = makeInjectorWithProperties(props);
final ObjectMapper mapper = makeObjectMapper(injector);
final String json = "[]";
final ClassWithFactoryMethodOfEmptyName object = mapper.readValue(json, ClassWithFactoryMethodOfEmptyName.class);
Assert.assertEquals(ImmutableList.of(), object.getTest());
}
@Test
public void testInjectClassOfEmptyConstructor() throws JsonProcessingException
{
final Properties props = new Properties();
props.setProperty(PROPERTY_NAME, PROPERTY_VALUE);
final Injector injector = makeInjectorWithProperties(props);
final ObjectMapper mapper = makeObjectMapper(injector);
final String json = "{}";
final ClassOfEmptyConstructor object = mapper.readValue(json, ClassOfEmptyConstructor.class);
Assert.assertEquals("empty constructor", object.val);
}
private static class ClassWithConstructorOfEmptyName
{
private final List<String> test;
@JsonCreator
public ClassWithConstructorOfEmptyName(List<String> test)
{
this.test = test;
}
@JsonValue
public List<String> getTest()
{
return test;
}
}
private static class ClassWithFactoryMethodOfEmptyName
{
private final List<String> test;
@JsonCreator
public static ClassWithFactoryMethodOfEmptyName create(List<String> test)
{
return new ClassWithFactoryMethodOfEmptyName(test);
}
private ClassWithFactoryMethodOfEmptyName(List<String> test)
{
this.test = test;
}
@JsonValue
public List<String> getTest()
{
return test;
}
}
private static class ClassOfEmptyConstructor
{
private final String val;
@JsonCreator
public ClassOfEmptyConstructor()
{
this.val = "empty constructor";
}
}
}
public static class ClassOfMultipleJsonCreatorsTest
{
@Test
public void testDeserializeUsingMultiArgumentsConstructor() throws JsonProcessingException
{
final Properties props = new Properties();
props.setProperty(PROPERTY_NAME, PROPERTY_VALUE);
final Injector injector = makeInjectorWithProperties(props);
final ObjectMapper mapper = makeObjectMapper(injector);
final String json = "{\"val\": \"this is an injection test\", \"valLen\": 5, \"\": \"nice try\" }";
final ClassOfMultipleJsonCreators object = mapper.readValue(json, ClassOfMultipleJsonCreators.class);
Assert.assertEquals("this is an injection test", object.val);
Assert.assertEquals(5, object.valLen);
Assert.assertNotNull(object.injected);
Assert.assertEquals(PROPERTY_VALUE, object.injected.val);
}
@Test
public void testDeserializeUsingDelegateConstructor() throws JsonProcessingException
{
final Properties props = new Properties();
props.setProperty(PROPERTY_NAME, PROPERTY_VALUE);
final Injector injector = makeInjectorWithProperties(props);
final ObjectMapper mapper = makeObjectMapper(injector);
final String json = "\"this is an injection test\"";
final ClassOfMultipleJsonCreators object = mapper.readValue(json, ClassOfMultipleJsonCreators.class);
Assert.assertEquals("this is an injection test", object.val);
Assert.assertEquals(object.val.length(), object.valLen);
Assert.assertNull(object.injected);
}
private static class ClassOfMultipleJsonCreators
{
private final String val;
private final int valLen;
@Nullable
private final InjectedParameter injected;
@JsonCreator
public ClassOfMultipleJsonCreators(
@JsonProperty("val") String val,
@JsonProperty("valLen") int valLen,
@JacksonInject @Nullable InjectedParameter injected
)
{
this.val = val;
this.valLen = valLen;
this.injected = injected;
}
@JsonCreator
public static ClassOfMultipleJsonCreators create(String val)
{
return new ClassOfMultipleJsonCreators(val, val.length(), null);
}
@JsonProperty
public String getVal()
{
return val;
}
@JsonProperty
public int getValLen()
{
return valLen;
}
}
}
private static class InjectedParameter
{
@JsonProperty
private String val;
}
private static Injector makeInjectorWithProperties(final Properties props)
{
return Guice.createInjector(
ImmutableList.of(
new DruidGuiceExtensions(),
(Module) binder -> {
binder.bind(Validator.class).toInstance(Validation.buildDefaultValidatorFactory().getValidator());
binder.bind(JsonConfigurator.class).in(LazySingleton.class);
binder.bind(Properties.class).toInstance(props);
JsonConfigProvider.bind(binder, "druid.injected", InjectedParameter.class);
}
)
);
}
private static ObjectMapper makeObjectMapper(Injector injector)
{
final ObjectMapper mapper = new ObjectMapper();
DruidSecondaryModule.setupJackson(injector, mapper);
return mapper;
}
}

View File

@ -30,6 +30,7 @@ import org.apache.druid.client.indexing.IndexingServiceClient;
import org.apache.druid.client.indexing.NoopIndexingServiceClient;
import org.apache.druid.data.input.impl.NoopInputFormat;
import org.apache.druid.data.input.impl.NoopInputSource;
import org.apache.druid.guice.DruidSecondaryModule;
import org.apache.druid.guice.FirehoseModule;
import org.apache.druid.indexing.common.stats.DropwizardRowIngestionMetersFactory;
import org.apache.druid.indexing.common.task.IndexTaskClientFactory;
@ -43,6 +44,7 @@ import org.apache.druid.math.expr.ExprMacroTable;
import org.apache.druid.query.expression.LookupEnabledTestExprMacroTable;
import org.apache.druid.segment.IndexIO;
import org.apache.druid.segment.IndexMergerV9;
import org.apache.druid.segment.TestHelper;
import org.apache.druid.segment.incremental.RowIngestionMetersFactory;
import org.apache.druid.segment.loading.LocalDataSegmentPuller;
import org.apache.druid.segment.loading.LocalLoadSpec;
@ -115,6 +117,7 @@ public class TestUtils
}
}
);
DruidSecondaryModule.setupAnnotationIntrospector(jsonMapper, TestHelper.makeAnnotationIntrospector());
List<? extends Module> firehoseModules = new FirehoseModule().getJacksonModules();
firehoseModules.forEach(jsonMapper::registerModule);

View File

@ -19,12 +19,16 @@
package org.apache.druid.segment;
import com.fasterxml.jackson.databind.AnnotationIntrospector;
import com.fasterxml.jackson.databind.InjectableValues;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.introspect.AnnotatedMember;
import com.google.common.base.Preconditions;
import com.google.common.collect.Lists;
import org.apache.druid.data.input.MapBasedRow;
import org.apache.druid.data.input.Row;
import org.apache.druid.guice.DruidSecondaryModule;
import org.apache.druid.guice.GuiceAnnotationIntrospector;
import org.apache.druid.jackson.DefaultObjectMapper;
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.java.util.common.guava.Sequence;
@ -71,9 +75,26 @@ public class TestHelper
return new IndexIO(JSON_MAPPER, columnConfig);
}
public static AnnotationIntrospector makeAnnotationIntrospector()
{
// Prepare annotationIntrospector with similar logic, except skip Guice loading
// because most tests don't use Guice injection.
return new GuiceAnnotationIntrospector()
{
@Override
public Object findInjectableValueId(AnnotatedMember m)
{
return null;
}
};
}
public static ObjectMapper makeJsonMapper()
{
final ObjectMapper mapper = new DefaultObjectMapper();
AnnotationIntrospector introspector = makeAnnotationIntrospector();
DruidSecondaryModule.setupAnnotationIntrospector(mapper, introspector);
mapper.setInjectableValues(
new InjectableValues.Std()
.addValue(ExprMacroTable.class.getName(), TestExprMacroTable.INSTANCE)
@ -86,6 +107,7 @@ public class TestHelper
public static ObjectMapper makeSmileMapper()
{
final ObjectMapper mapper = new DefaultObjectMapper();
DruidSecondaryModule.setupAnnotationIntrospector(mapper, makeAnnotationIntrospector());
mapper.setInjectableValues(
new InjectableValues.Std()
.addValue(ExprMacroTable.class.getName(), TestExprMacroTable.INSTANCE)