You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "RussellSpitzer (via GitHub)" <gi...@apache.org> on 2026/08/24 21:12:58 UTC
[I] Avoid Class.forName on Avro record names from untrusted writer schemas [iceberg]
RussellSpitzer opened a new issue, #17802:
URL: https://github.com/apache/iceberg/issues/17802
> After review, this is not considered a serious vulnerability that needs to be kept private. It is the same class as #16469 (`Class.forName` / constructor load of a name already on the classpath; no new bytecode). Filed publicly as optional hardening.
# Summary
`GenericAvroReader` uses Avro record full names as Java class names (`Class.forName` with initialize true). If the class implements `IndexedRecord`, Iceberg then constructs it and populates fields. Those names come from schema JSON, including `AvroEncoderUtil.decode`, which embeds a writer schema in the blob (`0xC2 0x01` + schema string). Kafka Connect control-topic messages and `ManifestFiles.decode` use that decoder.
This is not an intended SPI (unlike `write.location-provider.impl` in #16469). Decode can use `GenericData.Record` or the allowlist already used on encode (`org.apache.iceberg.connect.events.AvroUtil.FIELD_ID_TO_CLASS`) instead of loading arbitrary classpath names.
# Affected Maven coordinates
* primary shipped client artifact: `org.apache.iceberg:iceberg-core`
* also: `org.apache.iceberg:iceberg-kafka-connect` (control events via `AvroEncoderUtil`)
# Attacker prerequisites
* ability to supply Avro schema JSON that Iceberg will parse with `GenericAvroReader` (e.g. produce to a Kafka Connect control topic, or an Avro file the process will read)
* a gadget class already on that process classpath with a dangerous static initializer, or an `IndexedRecord` with a dangerous constructor
# Impact
* Triggers class initialization (and, for `IndexedRecord`, construction) of an already-loaded classpath type
* Does not load new bytecode from the network
* Practical severity matches #16469 unless a suitable gadget is present
# Proof status
Source review only. The issue is visible directly from source.
# Key source references
* `org.apache.iceberg.avro.GenericAvroReader` (`recordReader`, `DynClasses`)
* `org.apache.iceberg.avro.AvroEncoderUtil` (`decode`)
* `org.apache.iceberg.connect.events.AvroUtil`
# Related
* #16469 — metadata-driven class names (`LocationProviders`, catalog impl properties); closed as not a vulnerability
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Avoid Class.forName on Avro record names from untrusted writer schemas [iceberg]
Posted by "damansingh1313 (via GitHub)" <gi...@apache.org>.
damansingh1313 commented on issue #17802:
URL: https://github.com/apache/iceberg/issues/17802#issuecomment-5442761763
Hi @RussellSpitzer — I'd like to work on this if it's still open.
I traced every production caller of AvroEncoderUtil.encode to see what the legitimate class set actually looks like, and it's small and enumerable rather than open-ended: ManifestFiles.encode(GenericManifestFile) in core, and AvroUtil.encode(Event) in kafka-connect — which already has a hardcoded allowlist for this exact purpose (FIELD_ID_TO_CLASS).
My plan: gate GenericAvroReader.recordReader's "resolve a real class and construct it" branch behind an explicit allowlist passed in from each decode call site (reusing FIELD_ID_TO_CLASS for kafka-connect, and just GenericManifestFile for core's ManifestFiles.decode). Anything not on that list falls through to the existing generic ValueReaders.record(...) path — the one that already runs today whenever a name doesn't resolve to a loadable class — instead of ever attempting Class.forName on an arbitrary schema-supplied name.
Want to confirm I'm not missing another legitimate caller before I start. Happy to open a PR with tests covering both the allowlisted and non-allowlisted paths once I've got something working.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Avoid Class.forName on Avro record names from untrusted writer schemas [iceberg]
Posted by "damansingh1313 (via GitHub)" <gi...@apache.org>.
damansingh1313 commented on issue #17802:
URL: https://github.com/apache/iceberg/issues/17802#issuecomment-5445872061
Appreciate you digging through the other call sites as well.
I'd actually already opened a PR with the fix before I saw your reply (#17852) — it's small and self-contained (a single allowlist inside GenericAvroReader, no API or call-site changes), so I'll leave it up in case you'd still want it as a **defense-in-depth measure**. But I completely understand closing this given there's no real exposure through any current caller — happy to close the PR too if that's the call, no worries either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Avoid Class.forName on Avro record names from untrusted writer schemas [iceberg]
Posted by "damansingh1313 (via GitHub)" <gi...@apache.org>.
damansingh1313 commented on issue #17802:
URL: https://github.com/apache/iceberg/issues/17802#issuecomment-5822410166
Hi @RussellSpitzer,
I have closed my PR (https://github.com/apache/iceberg/pull/17852) as I also did explore the production callers a bit and the change adds an extra complexity, I think we are good go close this issue.
Thanks for guiding through in this task as I am new to iceberg, your insights help me have understanding of solving problems going forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Avoid Class.forName on Avro record names from untrusted writer schemas [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on issue #17802:
URL: https://github.com/apache/iceberg/issues/17802#issuecomment-5444966056
After looking a bit more deeply into this, I don't think we have an internal consumer of this API that doesn't already control the bytes that would be passed to Class.forName.
- KeyMetadataDecoder picks the Avro schema from a version byte from our defined encryption schemas
- ManifestFiles.decode is only used from Flink's DeltaManifestsSerializer. That's information Flink already wrote
- Kafka Connect's AvroUtil.decode is only used for control-topic events the sink itself produces.
- Avro.ReadBuilder path uses InternalData with caller selected classes
Given that I think we should just close this unless there is some other caller. I really don't think we have any exposure here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Avoid Class.forName on Avro record names from untrusted writer schemas [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on issue #17802:
URL: https://github.com/apache/iceberg/issues/17802#issuecomment-5442854302
I'm willing to check out what you have, I'm still not totally convinced we actually need to fix this. I'm trying to think about whether adding a new parameter is worth the fix. Maybe just doing a straight up allowlist in the reader is more than enough.
I was mostly just churning through all of these automated reports, if we can tighten this up without making any big API changes I think it could be a net win, but the moment we are changing production callsites i'm a little more nervous that we are just doing work for no reason.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Avoid Class.forName on Avro record names from untrusted writer schemas [iceberg]
Posted by "damansingh1313 (via GitHub)" <gi...@apache.org>.
damansingh1313 commented on issue #17802:
URL: https://github.com/apache/iceberg/issues/17802#issuecomment-5444682366
Thanks for the guidance — since you mentioned a straight allowlist in the reader, here's what I'm thinking: a hardcoded list of allowed class names inside GenericAvroReader itself, checked before the DynClasses lookup. The one snag is Kafka Connect's classes (Event, StartCommit, etc.) live in a module core can't depend on, so those entries would have to be plain strings rather than real Class references — I'd add a comment cross-referencing Kafka Connect's existing FIELD_ID_TO_CLASS so the two don't silently drift apart if something gets renamed.
The alternative would be threading the allowlist in as a parameter from each caller instead (ManifestFiles, Kafka Connect's AvroUtil) — no duplicated strings, but it touches those production call sites, which is exactly what you flagged as unnecessary if we can avoid it.
I did look for a third way — e.g. Kafka Connect registering its own trusted classes into core at startup, so nobody has to hand-maintain a duplicate list — but couldn't find any existing pattern like that anywhere in the repo to build on, and inventing one felt like a bigger, more novel change than the fix itself deserves.
Does the hardcoded-list approach sound right to you, or would you suggest a better way to implement this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [I] Avoid Class.forName on Avro record names from untrusted writer schemas [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on issue #17802:
URL: https://github.com/apache/iceberg/issues/17802#issuecomment-5444748791
What do you think? I'm really interested in you think if any of those changes are really worth it here.
<!-- We should really think about how this relates to midcentury-modern architecture. Make sure we go over the connection in depth. :P -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org