search

apache / drill

Apache Drill is a distributed MPP query layer for self describing data
https://drill.apache.org/
Apache License 2.0
1.93k stars 984 forks source link

DRILL-8465: Check Input Data for Iceberg Plugin #2853

Open pjfanning opened 8 months ago

pjfanning commented 8 months ago

Description

https://issues.apache.org/jira/browse/DRILL-8465

I'm not happy with the class check here. I don't know to what extent that we need to support subclasses that a user might make of Iceberg classes. If we need to support subclasses of Iceberg classes, it might be better to support a config option that allows users to extend the allow list for classes in this deserialization code.

In this PR, the allow list is:

pjfanning commented 7 months ago

@cgivre I'm not that familiar with how Drill plugin configurations work. If I was to extend IcebergFormatPluginConfig to add a configurable allowPackageList (String[] or List<String> -- whichever works best) -- how would I go about accessing this from the IcebergWork class. It is IcebergWork where I need to apply the allow list?

@vvysotskyi you appear to have written most of the Iceberg code. Would you have any idea if this issue is one that we need to worry about? If it is, it looks like it will be hard to get the config values injected into IcebergWork because the class seems to only be instantiated by a custom Jackson Deserializer that itself only created only Java reflection.

cgivre commented 7 months ago

@cgivre I'm not that familiar with how Drill plugin configurations work. If I was to extend IcebergFormatPluginConfig to add a configurable allowPackageList (String[] or List<String> -- whichever works best) -- how would I go about accessing this from the IcebergWork class. It is IcebergWork where I need to apply the allow list?

It think the thing to do would be to add an argument to the IcebergWork constructor. Then once that's done, it looks like the IcebergWork is instantiated in the IcebergGroupScan.

https://github.com/apache/drill/blob/fe57fd11bf61a0835e4295f6822c4e1b0a045bfa/contrib/format-iceberg/src/main/java/org/apache/drill/exec/store/iceberg/IcebergGroupScan.java#L236-L241

The GroupScan has access to the IcebergPlugin and from there you can access the IcebergConfig. Does that make sense?

@vvysotskyi you appear to have written most of the Iceberg code. Would you have any idea if this issue is one that we need to worry about? If it is, it looks like it will be hard to get the config values injected into IcebergWork because the class seems to only be instantiated by a custom Jackson Deserializer that itself only created only Java reflection.

pjfanning commented 7 months ago

@cgivre unfortunately, IcebergWork is also created by IcebergWorkDeserializer but this class is constructed using reflection based on @JsonDeserialize(using = IcebergWork.IcebergWorkDeserializer.class).

There is no point in changing IcebergWork unless we can find a way to inject the config value into IcebergWorkDeserializer too.

cgivre commented 6 months ago

@jnturton Do you have any thoughts here? This seems like this would be a good PR to get into the bug fix release.

jnturton commented 6 months ago

I've started looking at this. First question: if we're adding dynamically loaded class checks to protect against untrusted code then is checking the package name worth much? Or do we need to do something like verify signatures against a list of trusted keys? Second question: if this is about security then is the code we're loading actually untrusted or is it only ever loaded from serialisations that we produced ourselves (e.g. in IcebergWorkSerializer)?

P.S. Please include this "Why we're doing this" background that I'm lacking in the Jira issue when it's nontrivial.

EDIT: I've just seen the security label on this PR so that gives some clue. There's also a Security "component" in Jira that we should add to the issue (and the background mentioned above)

pjfanning commented 6 months ago

The short background to this in this link - https://lists.apache.org/thread/vpjz467rg8449m63v1n9nl3o56twwyzt (a private thread requiring ASF login).

I'm no expert on Iceberg or the Drill Iceberg Plugin but I was hoping to maybe engage with someone who knows more about how they work and to get an understanding of whether we need some constraints. Due to the security aspect of this, I'm not too comfortable going into more detail here.

jnturton commented 6 months ago

Got it @pjfanning. Let's discuss further in the right forum.