Skip to content

Commit 3871e9a

Browse files
Replace serde_cbor with ciborium due to security vulnerability (#3855)
2 parents 191c577 + 755dd28 commit 3871e9a

File tree

5 files changed

+143
-18
lines changed

5 files changed

+143
-18
lines changed

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/CargoDependency.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,7 @@ data class CargoDependency(
310310
val Hound: CargoDependency = CargoDependency("hound", CratesIo("3.4.0"), DependencyScope.Dev)
311311
val PrettyAssertions: CargoDependency =
312312
CargoDependency("pretty_assertions", CratesIo("1.3.0"), DependencyScope.Dev)
313-
val SerdeCbor: CargoDependency = CargoDependency("serde_cbor", CratesIo("0.11"), DependencyScope.Dev)
313+
val Ciborium: CargoDependency = CargoDependency("ciborium", CratesIo("0.2"), DependencyScope.Dev)
314314
val SerdeJson: CargoDependency = CargoDependency("serde_json", CratesIo("1.0.0"), DependencyScope.Dev)
315315
val Smol: CargoDependency = CargoDependency("smol", CratesIo("1.2.0"), DependencyScope.Dev)
316316
val TempFile: CargoDependency = CargoDependency("tempfile", CratesIo("3.2.0"), DependencyScope.Dev)

codegen-serde/src/test/kotlin/software/amazon/smithy/rust/codegen/serde/SerdeDecoratorTest.kt

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ class SerdeDecoratorTest {
341341
arrayOf(
342342
"crate" to RustType.Opaque(ctx.moduleUseName()),
343343
"serde_json" to CargoDependency("serde_json", CratesIo("1")).toDevDependency().toType(),
344-
"serde_cbor" to CargoDependency("serde_cbor", CratesIo("0.11.2")).toDevDependency().toType(),
344+
"ciborium" to CargoDependency.Ciborium.toType(),
345345
// we need the derive feature
346346
"serde" to CargoDependency.Serde.toDevDependency().toType(),
347347
)
@@ -437,7 +437,9 @@ class SerdeDecoratorTest {
437437
use #{crate}::serde::*;
438438
let input = StreamingInput::builder().data(ByteStream::from_static(b"123")).build().unwrap();
439439
let settings = SerializationSettings::default();
440-
let serialized = #{serde_cbor}::to_vec(&input.serialize_ref(&settings)).expect("failed to serialize");
440+
let mut serialized = Vec::new();
441+
#{ciborium}::ser::into_writer(&input.serialize_ref(&settings), &mut serialized)
442+
.expect("failed to serialize input into CBOR format using `ciborium`");
441443
assert_eq!(serialized, b"\xa1ddataC123");
442444
""",
443445
*codegenScope,

codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/serialize/CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest.kt

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ import java.util.logging.Logger
6262
/**
6363
* This lives in `codegen-server` because we want to run a full integration test for convenience,
6464
* but there's really nothing server-specific here. We're just testing that the CBOR (de)serializers work like
65-
* the ones generated by `serde_cbor`. This is a good exhaustive litmus test for correctness, since `serde_cbor`
65+
* the ones generated by `ciborium`. This is a good exhaustive litmus test for correctness, since `ciborium`
6666
* is battle-tested.
6767
*/
6868
internal class CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest {
@@ -175,7 +175,7 @@ internal class CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest {
175175
}
176176

177177
@Test
178-
fun `serde_cbor round trip`() {
178+
fun `ciborium round trip`() {
179179
val addDeriveSerdeSerializeDeserializeDecorator =
180180
object : ServerCodegenDecorator {
181181
override val name: String = "Add `#[derive(serde::Serialize, serde::Deserialize)]`"
@@ -240,7 +240,7 @@ internal class CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest {
240240
val codegenScope =
241241
arrayOf(
242242
"AssertEq" to RuntimeType.PrettyAssertions.resolve("assert_eq!"),
243-
"SerdeCbor" to CargoDependency.SerdeCbor.toType(),
243+
"ciborium" to CargoDependency.Ciborium.toType(),
244244
)
245245

246246
val instantiator = ServerInstantiator(codegenContext, ignoreMissingMembers = true, withinTest = true)
@@ -278,14 +278,14 @@ internal class CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest {
278278
if (expectFail.contains(test.id)) {
279279
writeWithNoFormatting("#[should_panic]")
280280
}
281-
unitTest("we_serialize_and_serde_cbor_deserializes_${test.id.toSnakeCase()}_${test.kind.toString().toSnakeCase()}") {
281+
unitTest("we_serialize_and_ciborium_deserializes_${test.id.toSnakeCase()}_${test.kind.toString().toSnakeCase()}") {
282282
rustTemplate(
283283
"""
284284
let expected = #{InstantiateShape:W};
285285
let bytes = #{SerializeFn}(&expected)
286286
.expect("our generated CBOR serializer failed");
287-
let actual = #{SerdeCbor}::from_slice(&bytes)
288-
.expect("serde_cbor failed deserializing from bytes");
287+
let actual = #{ciborium}::from_reader(::std::io::Cursor::new(&bytes))
288+
.expect("failed to deserialize bytes with `ciborium`");
289289
#{AssertEq}(expected, actual);
290290
""",
291291
"InstantiateShape" to instantiator.generate(targetShape, params),
@@ -330,12 +330,13 @@ internal class CborSerializerAndParserGeneratorSerdeRoundTripIntegrationTest {
330330
if (expectFail.contains(test.id)) {
331331
writeWithNoFormatting("#[should_panic]")
332332
}
333-
unitTest("serde_cbor_serializes_and_we_deserialize_${test.id.toSnakeCase()}_${test.kind.toString().toSnakeCase()}") {
333+
unitTest("ciborium_serializes_and_we_deserialize_${test.id.toSnakeCase()}_${test.kind.toString().toSnakeCase()}") {
334334
rustTemplate(
335335
"""
336336
let expected = #{InstantiateShape:W};
337-
let bytes: Vec<u8> = #{SerdeCbor}::to_vec(&expected)
338-
.expect("serde_cbor failed serializing to `Vec<u8>`");
337+
let mut bytes = Vec::new();
338+
#{ciborium}::into_writer(&expected, &mut bytes)
339+
.expect("failed to serialize to `Vec<u8>` with `ciborium`");
339340
let input = #{InputBuilder}::default();
340341
let input = #{DeserializeFn}(&bytes, input)
341342
.expect("our generated CBOR deserializer failed");

rust-runtime/aws-smithy-protocol-test/Cargo.toml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[package]
22
name = "aws-smithy-protocol-test"
3-
version = "0.62.0"
3+
version = "0.63.0"
44
authors = ["AWS Rust SDK Team <[email protected]>", "Russell Cohen <[email protected]>"]
55
description = "A collection of library functions to validate HTTP requests against Smithy protocol tests."
66
edition = "2021"
@@ -12,7 +12,7 @@ repository = "https://github.com/smithy-lang/smithy-rs"
1212
assert-json-diff = "1.1"
1313
base64-simd = "0.8"
1414
cbor-diag = "0.1.12"
15-
serde_cbor = "0.11"
15+
ciborium = "0.2"
1616
http = "0.2.1"
1717
pretty_assertions = "1.3"
1818
regex-lite = "0.1.5"

rust-runtime/aws-smithy-protocol-test/src/lib.rs

Lines changed: 126 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -415,23 +415,105 @@ fn try_json_eq(expected: &str, actual: &str) -> Result<(), ProtocolTestFailure>
415415
}
416416
}
417417

418+
/// Compares two `ciborium::value::Value` instances for semantic equality.
419+
///
420+
/// This function recursively compares two CBOR values, correctly handling arrays and maps
421+
/// according to the CBOR specification. Arrays are compared element-wise in order,
422+
/// while maps are compared without considering the order of key-value pairs.
423+
fn cbor_values_equal(
424+
a: &ciborium::value::Value,
425+
b: &ciborium::value::Value,
426+
) -> Result<bool, ProtocolTestFailure> {
427+
match (a, b) {
428+
(ciborium::value::Value::Array(a_array), ciborium::value::Value::Array(b_array)) => {
429+
// Both arrays should be equal in size.
430+
if a_array.len() != b_array.len() {
431+
return Ok(false);
432+
}
433+
// Compare arrays element-wise.
434+
for (a_elem, b_elem) in a_array.iter().zip(b_array.iter()) {
435+
if !cbor_values_equal(a_elem, b_elem)? {
436+
return Ok(false);
437+
}
438+
}
439+
Ok(true)
440+
}
441+
442+
// Convert `ciborium::value::Value::Map` to a `HashMap`, and then compare the values of
443+
// each key in `a` with those in `b`.
444+
(ciborium::value::Value::Map(a_map), ciborium::value::Value::Map(b_map)) => {
445+
if a_map.len() != b_map.len() {
446+
return Ok(false);
447+
}
448+
449+
let b_hashmap = ciborium_map_to_hashmap(b_map)?;
450+
// Each key in `a` should exist in `b`, and the values should match.
451+
for a_key_value in a_map.iter() {
452+
let (a_key, a_value) = get_text_key_value(a_key_value)?;
453+
match b_hashmap.get(a_key) {
454+
Some(b_value) => {
455+
if !cbor_values_equal(a_value, b_value)? {
456+
return Ok(false);
457+
}
458+
}
459+
None => return Ok(false),
460+
}
461+
}
462+
Ok(true)
463+
}
464+
465+
(ciborium::value::Value::Float(a_float), ciborium::value::Value::Float(b_float)) => {
466+
Ok(a_float == b_float || (a_float.is_nan() && b_float.is_nan()))
467+
}
468+
469+
_ => Ok(a == b),
470+
}
471+
}
472+
473+
/// Converts a `ciborium::value::Value::Map` into a `HashMap<&String, &ciborium::value::Value>`.
474+
///
475+
/// CBOR maps (`Value::Map`) are internally represented as vectors of key-value pairs,
476+
/// and direct comparison is affected by the order of these pairs.
477+
/// Since the CBOR specification treats maps as unordered collections,
478+
/// this function transforms the vector into a `HashMap`, for order-independent comparisons
479+
/// between maps.
480+
fn ciborium_map_to_hashmap(
481+
cbor_map: &[(ciborium::value::Value, ciborium::value::Value)],
482+
) -> Result<std::collections::HashMap<&String, &ciborium::value::Value>, ProtocolTestFailure> {
483+
cbor_map.iter().map(get_text_key_value).collect()
484+
}
485+
486+
/// Extracts a string key and its associated value from a CBOR key-value pair.
487+
/// Returns a `ProtocolTestFailure::InvalidBodyFormat` error if the key is not a text value.
488+
fn get_text_key_value(
489+
(key, value): &(ciborium::value::Value, ciborium::value::Value),
490+
) -> Result<(&String, &ciborium::value::Value), ProtocolTestFailure> {
491+
match key {
492+
ciborium::value::Value::Text(key_str) => Ok((key_str, value)),
493+
_ => Err(ProtocolTestFailure::InvalidBodyFormat {
494+
expected: "a text key as map entry".to_string(),
495+
found: format!("{:?}", key),
496+
}),
497+
}
498+
}
499+
418500
fn try_cbor_eq<T: AsRef<[u8]> + Debug>(
419501
actual_body: T,
420502
expected_body: &str,
421503
) -> Result<(), ProtocolTestFailure> {
422504
let decoded = base64_simd::STANDARD
423505
.decode_to_vec(expected_body)
424506
.expect("smithy protocol test `body` property is not properly base64 encoded");
425-
let expected_cbor_value: serde_cbor::Value =
426-
serde_cbor::from_slice(decoded.as_slice()).expect("expected value must be valid CBOR");
427-
let actual_cbor_value: serde_cbor::Value = serde_cbor::from_slice(actual_body.as_ref())
507+
let expected_cbor_value: ciborium::value::Value =
508+
ciborium::de::from_reader(decoded.as_slice()).expect("expected value must be valid CBOR");
509+
let actual_cbor_value: ciborium::value::Value = ciborium::de::from_reader(actual_body.as_ref())
428510
.map_err(|e| ProtocolTestFailure::InvalidBodyFormat {
429511
expected: "cbor".to_owned(),
430512
found: format!("{} {:?}", e, actual_body),
431513
})?;
432514
let actual_body_base64 = base64_simd::STANDARD.encode_to_string(&actual_body);
433515

434-
if expected_cbor_value != actual_cbor_value {
516+
if !cbor_values_equal(&expected_cbor_value, &actual_cbor_value)? {
435517
let expected_body_annotated_hex: String = cbor_diag::parse_bytes(&decoded)
436518
.expect("smithy protocol test `body` property is not valid CBOR")
437519
.to_hex();
@@ -599,6 +681,46 @@ mod tests {
599681
validate_body(actual, expected, MediaType::Json).expect_err("bodies do not match");
600682
}
601683

684+
#[test]
685+
fn test_validate_cbor_body() {
686+
let base64_encode = |v: &[u8]| base64_simd::STANDARD.encode_to_string(v);
687+
688+
// The following is the CBOR representation of `{"abc": 5 }`.
689+
let actual = [0xbf, 0x63, 0x61, 0x62, 0x63, 0x05, 0xff];
690+
// The following is the base64-encoded CBOR representation of `{"abc": 5 }` using a definite length map.
691+
let expected_base64 = base64_encode(&[0xA1, 0x63, 0x61, 0x62, 0x63, 0x05]);
692+
693+
validate_body(actual, expected_base64.as_str(), MediaType::Cbor)
694+
.expect("unexpected mismatch between CBOR definite and indefinite map encodings");
695+
696+
// The following is the CBOR representation of `{"a":1, "b":2}`.
697+
let actual = [0xBF, 0x61, 0x61, 0x01, 0x61, 0x62, 0x02, 0xFF];
698+
// The following is the base64-encoded CBOR representation of `{"b":2, "a":1}`.
699+
let expected_base64 = base64_encode(&[0xBF, 0x61, 0x62, 0x02, 0x61, 0x61, 0x01, 0xFF]);
700+
validate_body(actual, expected_base64.as_str(), MediaType::Cbor)
701+
.expect("different ordering in CBOR decoded maps do not match");
702+
703+
// The following is the CBOR representation of `{"a":[1,2,{"b":3, "c":4}]}`.
704+
let actual = [
705+
0xBF, 0x61, 0x61, 0x9F, 0x01, 0x02, 0xBF, 0x61, 0x62, 0x03, 0x61, 0x63, 0x04, 0xFF,
706+
0xFF, 0xFF,
707+
];
708+
// The following is the base64-encoded CBOR representation of `{"a":[1,2,{"c":4, "b":3}]}`.
709+
let expected_base64 = base64_encode(&[
710+
0xBF, 0x61, 0x61, 0x9F, 0x01, 0x02, 0xBF, 0x61, 0x63, 0x04, 0x61, 0x62, 0x03, 0xFF,
711+
0xFF, 0xFF,
712+
]);
713+
validate_body(actual, expected_base64.as_str(), MediaType::Cbor)
714+
.expect("different ordering in CBOR decoded maps do not match");
715+
716+
// The following is the CBOR representation of `{"a":[1,2]}`.
717+
let actual = [0xBF, 0x61, 0x61, 0x9F, 0x01, 0x02, 0xFF, 0xFF];
718+
// The following is the CBOR representation of `{"a":[2,1]}`.
719+
let expected_base64 = base64_encode(&[0xBF, 0x61, 0x61, 0x9F, 0x02, 0x01, 0xFF, 0xFF]);
720+
validate_body(actual, expected_base64.as_str(), MediaType::Cbor)
721+
.expect_err("arrays in CBOR should follow strict ordering");
722+
}
723+
602724
#[test]
603725
fn test_validate_xml_body() {
604726
let expected = r#"<a>

0 commit comments

Comments
 (0)