Merge branch 'proptest' for CVE-2024-39697

Author: rubdos

.github/workflows/build.yml

@@ -25,7 +25,13 @@ jobs:
         toolchain: ["stable", "beta"]
         coverage: [false]
         tests: [true]
+        proptest_max: [false]
         include:
+          # We run the proptests with the stable toolchain on more iterations
+          - toolchain: "stable"
+            coverage: false
+            tests: true
+            proptest_max: true
           - toolchain: "nightly"
             coverage: true
             tests: true
@@ -60,11 +66,20 @@ jobs:
 
       - name: Run tests
         uses: actions-rs/cargo@v1
-        if: ${{ !matrix.coverage && matrix.tests }}
+        if: ${{ !matrix.coverage && matrix.tests && !matrix.proptest_max }}
         with:
           command: test
           args: --all-targets --no-fail-fast
 
+      - name: Run tests
+        uses: actions-rs/cargo@v1
+        if: ${{ !matrix.coverage && matrix.tests && matrix.proptest_max }}
+        env:
+          PROPTEST_CASES: 65536
+        with:
+          command: test
+          args: --all-targets --release --no-fail-fast
+
       - name: Run tests
         uses: actions-rs/cargo@v1
         if: ${{ matrix.coverage && matrix.tests }}

Cargo.toml

@@ -41,6 +41,7 @@ criterion = ">=0.4, <=0.5"
 doc-comment = "0.3"
 rstest = ">= 0.13, <=0.19"
 rstest_reuse = "0.6"
+proptest = "1.0.0"
 
 [[bench]]
 name = "parsing"

src/national_number.rs

@@ -22,13 +22,16 @@ pub struct NationalNumber {
 }
 
 impl NationalNumber {
-    pub fn new(value: u64, zeros: u8) -> Self {
+    pub fn new(value: u64, zeros: u8) -> Result<Self, crate::error::Parse> {
         // E.164 specifies a maximum of 15 decimals, which corresponds to slightly over 48.9 bits.
         // 56 bits ought to cut it here.
-        assert!(value < (1 << 56), "number too long");
-        Self {
-            value: ((zeros as u64) << 56) | value,
+        if value >= (1 << 56) {
+            return Err(crate::error::Parse::TooLong);
         }
+
+        Ok(Self {
+            value: ((zeros as u64) << 56) | value,
+        })
     }
 
     /// The number without any leading zeroes.

src/parser/mod.rs

@@ -86,7 +86,7 @@ pub fn parse_with<S: AsRef<str>>(
         national: NationalNumber::new(
             number.national.parse()?,
             number.national.chars().take_while(|&c| c == '0').count() as u8,
-        ),
+        )?,
 
         extension: number.extension.map(|s| Extension(s.into_owned())),
         carrier: number.carrier.map(|s| Carrier(s.into_owned())),
@@ -108,7 +108,7 @@ mod test {
                 source: country::Source::Default,
             },
 
-            national: NationalNumber::new(33316005, 0),
+            national: NationalNumber::new(33316005, 0).unwrap(),
 
             extension: None,
             carrier: None,
@@ -196,7 +196,7 @@ mod test {
                 source: country::Source::Number,
             },
 
-            national: NationalNumber::new(64123456, 0),
+            national: NationalNumber::new(64123456, 0).unwrap(),
 
             extension: None,
             carrier: None,
@@ -214,7 +214,7 @@ mod test {
                     source: country::Source::Default,
                 },
 
-                national: NationalNumber::new(30123456, 0),
+                national: NationalNumber::new(30123456, 0).unwrap(),
 
                 extension: None,
                 carrier: None,
@@ -229,7 +229,7 @@ mod test {
                     source: country::Source::Plus,
                 },
 
-                national: NationalNumber::new(2345, 0,),
+                national: NationalNumber::new(2345, 0,).unwrap(),
 
                 extension: None,
                 carrier: None,
@@ -244,7 +244,7 @@ mod test {
                     source: country::Source::Default,
                 },
 
-                national: NationalNumber::new(12, 0,),
+                national: NationalNumber::new(12, 0,).unwrap(),
 
                 extension: None,
                 carrier: None,
@@ -259,7 +259,7 @@ mod test {
                     source: country::Source::Default,
                 },
 
-                national: NationalNumber::new(3121286979, 0),
+                national: NationalNumber::new(3121286979, 0).unwrap(),
 
                 extension: None,
                 carrier: Some("12".into()),
@@ -279,4 +279,10 @@ mod test {
         let res = parser::parse(None, ".;phone-context=");
         assert!(res.is_err(), "{res:?}");
     }
+
+    #[test]
+    fn advisory_2() {
+        let res = parser::parse(None, "+dwPAA;phone-context=AA");
+        assert!(res.is_err(), "{res:?}");
+    }
 }

tests/prop.proptest-regressions

@@ -0,0 +1,8 @@
+# Seeds for failure cases proptest has generated in the past. It is
+# automatically read and these particular cases re-run before any
+# novel cases are generated.
+#
+# It is recommended to check this file in to source control so that
+# everyone who runs the test benefits from these saved cases.
+cc f4f1a19cf143c767508ab557480843e4f4093898768fbbea1034d19d4308257d # shrinks to tel_prefix = false, use_plus = true, s = "da", phone_context = Some("A0A0a")
+cc 4ea103e574793bd24b0267cc8a80962299ee50746d69332fbd0b85532fb707e2 # shrinks to tel_prefix = false, use_plus = false, s = "0", phone_context = Some("প")

tests/prop.rs

@@ -0,0 +1,34 @@
+use phonenumber::parse;
+use proptest::prelude::*;
+
+proptest! {
+    #[test]
+    fn rfc3966_crash_test(
+        tel_prefix: bool,
+        use_plus: bool,
+        s: String,
+        phone_context: Option<String>,
+    ) {
+        let context = if let Some(phone_context) = &phone_context { format!(";phone-context={phone_context}") } else { "".to_string() };
+        let tel_prefix = if tel_prefix { "tel:" } else { "" };
+        let plus = if use_plus { "+" } else { "" };
+        let s = format!("{}{}{}{}", tel_prefix, plus, s, context);
+        let _ = parse(None, &s);
+    }
+
+    #[test]
+    fn doesnt_crash(s in "\\PC*") {
+        let _ = parse(None, &s);
+    }
+
+    #[test]
+    fn doesnt_crash_2(s in "\\+\\PC*") {
+        let _ = parse(None, &s);
+    }
+
+    #[test]
+    fn parse_belgian_phonenumbers(s in "\\+32[0-9]{8,9}") {
+        let parsed = parse(None, &s).expect("valid Belgian number");
+        prop_assert_eq!(parsed.country().id(), phonenumber::country::BE.into());
+    }
+}