Kriskras99 commented on code in PR #642:
URL: https://github.com/apache/avro-rs/pull/642#discussion_r3860496892
##########
avro/src/util.rs:
##########
@@ -32,6 +33,15 @@ use std::{
pub const DEFAULT_MAX_ALLOCATION_BYTES: usize = 512 * 1024 * 1024;
static MAX_ALLOCATION_BYTES: OnceLock<usize> = OnceLock::new();
+/// Maximum recursion depth when decoding or resolving Avro-encoded values.
+///
+/// This protects against stack exhaustion (an abort, not a catchable error)
from deeply nested
+/// data: a recursive schema lets an attacker drive one recursion level with
roughly one wire byte.
+///
+/// See [`max_decode_recursion_depth`] to change this limit.
+pub const DEFAULT_MAX_DECODE_RECURSION_DEPTH: usize = 32;
Review Comment:
At 62 the `decode::tests::avro_rs_642_resolve_recursion_depth_is_bounded`
test overflows.
At 558(!) the
`recursion_depth_deser::avro_rs_642_recursion_depth_is_bounded` test overflows.
So I think there might be some optimisation room left for the decode
implementation (not for this PR obviously).
##########
avro/src/serde/deser_schema/mod.rs:
##########
Review Comment:
This should also increment the recursion depth as it does not use `new`
##########
avro/src/util.rs:
##########
@@ -32,6 +33,15 @@ use std::{
pub const DEFAULT_MAX_ALLOCATION_BYTES: usize = 512 * 1024 * 1024;
static MAX_ALLOCATION_BYTES: OnceLock<usize> = OnceLock::new();
+/// Maximum recursion depth when decoding or resolving Avro-encoded values.
+///
+/// This protects against stack exhaustion (an abort, not a catchable error)
from deeply nested
+/// data: a recursive schema lets an attacker drive one recursion level with
roughly one wire byte.
Review Comment:
```suggestion
/// This protects against stack exhaustion aborts from deeply nested data.
```
The attack information is a nice tidbit, but practically useless for the
user.
##########
avro/src/serde/deser_schema/mod.rs:
##########
@@ -88,8 +94,16 @@ impl<'s, 'r, R: Read, S: Borrow<Schema>>
SchemaAwareDeserializer<'s, 'r, R, S> {
pub fn new(
reader: &'r mut R,
schema: &'s Schema,
- config: Config<'s, S>,
+ mut config: Config<'s, S>,
) -> Result<Self, Error> {
+ // Bound the recursion depth so deeply nested (possibly hostile) data
+ // yields an error instead of exhausting the stack. A recursive schema
+ // lets roughly one wire byte drive one nesting level.
Review Comment:
```suggestion
// Bound the recursion depth to guard against stack exhaustion
```
##########
avro/tests/recursion_depth_deser.rs:
##########
@@ -0,0 +1,51 @@
+// 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.
+
+use apache_avro::Schema;
+use apache_avro::reader::datum::GenericDatumReader;
+use apache_avro::util::max_decode_recursion_depth;
+use apache_avro_test_helper::TestResult;
+use serde::{Deserialize, Serialize};
+
+// This is an IT test because it sets the default recursion depth limit
(OnceLock).
Review Comment:
```suggestion
// This is an integration test because it sets the default recursion depth
limit (OnceLock).
```
##########
avro/src/serde/deser_schema/mod.rs:
##########
@@ -88,8 +94,16 @@ impl<'s, 'r, R: Read, S: Borrow<Schema>>
SchemaAwareDeserializer<'s, 'r, R, S> {
pub fn new(
reader: &'r mut R,
schema: &'s Schema,
- config: Config<'s, S>,
+ mut config: Config<'s, S>,
) -> Result<Self, Error> {
+ // Bound the recursion depth so deeply nested (possibly hostile) data
+ // yields an error instead of exhausting the stack. A recursive schema
+ // lets roughly one wire byte drive one nesting level.
+ config.recursion_depth += 1;
+ let maximum = decode_recursion_limit();
+ if config.recursion_depth > maximum {
+ return Err(Details::DecodeRecursionLimit { maximum }.into());
+ }
Review Comment:
I think it would be better to split this off into an implementation on
`Config`, otherwise this would need to be duplicated in `with_different_schema`
##########
avro/src/serde/deser_schema/mod.rs:
##########
@@ -50,6 +50,12 @@ pub struct Config<'s, S: Borrow<Schema>> {
pub names: &'s HashMap<Name, S>,
/// Was the data serialized with `human_readable`.
pub human_readable: bool,
+ /// Current recursion depth of the deserializer.
+ ///
+ /// Every nesting level of the deserialized value creates a new
+ /// [`SchemaAwareDeserializer`], which increments this; the depth is
+ /// bounded by [`crate::util::max_decode_recursion_depth`].
+ pub(crate) recursion_depth: usize,
Review Comment:
```suggestion
pub recursion_depth: usize,
```
`Config` is not a public type, so `pub(crate)` does nothing
##########
avro/tests/recursion_depth_value_resolve.rs:
##########
@@ -0,0 +1,53 @@
+// 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.
+
+use apache_avro::Schema;
+use apache_avro::types::Value;
+use apache_avro::util::max_decode_recursion_depth;
+use apache_avro_test_helper::TestResult;
+
+// This is an IT test because it sets the default recursion depth limit
(OnceLock).
Review Comment:
```suggestion
// This is an integration test because it sets the default recursion depth
limit (OnceLock).
```
This file can be merged with the other integration test right?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
