Fix CVE-2021-25646

core/src/main/java/org/apache/druid/guice/DruidSecondaryModule.java

@@ -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 ObjectMapper getSmileMapper(Injector injector)
     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()
+        )
     );
   }
 }

core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java

@@ -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 Object findInjectableValueId(AnnotatedMember m)
     }
     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("");
+  }
 }

core/src/test/java/org/apache/druid/data/input/impl/DimensionsSpecSerdeTest.java

@@ -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);
   }
 

core/src/test/java/org/apache/druid/data/input/impl/StringDimensionSchemaTest.java

@@ -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);
+  }
+}