Merging Quant-TheodoreFelix's improvement to ct::Condition<u64>. Closes #7

Author: ounsworth

.github/workflows/publish_doc_benches_to_ghpages.yaml

@@ -37,21 +37,22 @@ jobs:
         with:
           name: code_quality_stats
           path: ./code_stats.txt
-  run_benches:
-    runs-on: ubuntu-latest
-    if: github.ref == 'refs/heads/main'
-    steps:
-      - name: Checkout code
-        uses: actions/checkout@v4
-      - run: cargo bench --all
-      - name: Save artifacts
-        uses: actions/upload-artifact@v4
-        with:
-          name: bc-rust-benches
-          path: ./target/criterion
+  # the benches run crazy slow on the github agent. So there's really no point because it's not useful data.
+  # run_benches:
+  #   runs-on: ubuntu-latest
+  #   if: github.ref == 'refs/heads/main'
+  #   steps:
+  #     - name: Checkout code
+  #       uses: actions/checkout@v4
+  #     - run: cargo bench --all
+  #     - name: Save artifacts
+  #       uses: actions/upload-artifact@v4
+  #       with:
+  #         name: bc-rust-benches
+  #         path: ./target/criterion
   collect_ghpages:
     if: github.ref == 'refs/heads/main'
-    needs: [build_docs, code_stats, run_benches]
+    needs: [build_docs, code_stats]
     runs-on: ubuntu-latest
     steps:
       - run: mkdir ./gh-pages
@@ -65,11 +66,11 @@ jobs:
         with:
           name: code_quality_stats
           path: ./gh-pages/
-      - name: Get benches from previous job
-        uses: actions/download-artifact@v4
-        with:
-          name: bc-rust-benches
-          path: ./gh-pages/benches
+      # - name: Get benches from previous job
+      #   uses: actions/download-artifact@v4
+      #   with:
+      #     name: bc-rust-benches
+      #     path: ./gh-pages/benches
       - name: Archive Compatibility Matrix For Download
         uses:  actions/upload-pages-artifact@v3
         with:

CONTRIBUTING.md

@@ -20,6 +20,23 @@ before posting anything public. See [Security Policy](SECURITY.md).
 
 If a related discussion or issue doesn't exist, and the issue is not security related, you can [open a new issue](https://github.com/bcgit/bc-java/issues/new). An issue can be converted into a discussion if regarded as one.
 
+## Coding philosophy
+
+> Slow is smooth, smooth is fast.
+
+There is a time and a place for "Move fast and break things", but the source code of a crypto library is not one of them.
+
+This project takes the philosophy that taking the time to do things right pays off in the long run, both in terms of 
+the runtime and memory footprint of the code, and it terms of the time required for a future maintainer to get up to speed with the code
+and avoid introducing bugs due to the code being hard to understand.
+
+Some specifics:
+
+* Respect that the innovative process sometimes requires exploring several dead-ends before you find the most elegant solution.
+* Public APIs of a library should be both ergonomic and expressive. When defining a new trait or public function, ask yourself whether a programmer who is new to cryptography is likely to use this in a way that will get them into trouble.
+* Variables should be well-named, well-structured, and well-commented (a comment-to-code ration of 1:1 is a goal to be strived for!). Think about memory footprint and, where possible, use unnamed scopes to allow the compiler to pop intermediate value variables off the stack as soon as they are no longer needed.
+* Always run your code through `cargo mutants` and get the issue count as low as your can. As a first pass, this forces you to write thorough unit tests. As a second pass, this draws your attention to bits of your code that cannot be tested from the outside. Often this means that the code can be simplified without affecting functionality (as defined by your set of unit tests) -- "simpler code" usually means faster runtime and easier future maintenance.
+
 ## Contribute to the code
 
 For substantial, non-trivial contributions, you may be asked to sign a contributor assignment agreement. Optionally, you can also have your name and contact information listed in [Contributors](https://www.bouncycastle.org/contributors.html). 
@@ -56,5 +73,5 @@ Don't forget to self-review. Please follow these simple guidelines:
 
 #### Your pull request is merged
 
-For acceptance, pull requests need to meet specific quality criteria, including tests for anything substantial. Someone on the Bouncy Castle core team will review the pull request when there is time, and let you know if something is missing or suggest improvements. If it is a useful and generic feature it will be integrated in Bouncy Castle to be available in a later release.
+Someone on the Bouncy Castle core team will review the pull request when there is time, and let you know if something is missing or suggest improvements. If it is a useful and generic feature it will be integrated in Bouncy Castle to be available in a later release.
 

crypto/core/src/errors.rs

@@ -145,3 +145,4 @@ impl From<KeyMaterialError> for SignatureError {
 impl From<RNGError> for SignatureError {
     fn from(e: RNGError) -> SignatureError { Self::RNGError(e) }
 }
+

crypto/core/src/key_material.rs

@@ -39,7 +39,7 @@
 
 use crate::errors::KeyMaterialError;
 use crate::traits::{RNG, SecurityStrength, Secret};
-use bouncycastle_utils::{ct, max, min};
+use bouncycastle_utils::{ct, min};
 
 use core::cmp::{Ordering, PartialOrd};
 use core::fmt;
@@ -569,8 +569,8 @@ impl<const KEY_LEN: usize> KeyMaterialTrait for KeyMaterial<KEY_LEN> {
         }
         self.buf[self.key_len..new_key_len].copy_from_slice(other.ref_to_bytes());
         self.key_len += other.key_len();
-        self.key_type = max(&self.key_type, &other.key_type()).clone();
-        self.security_strength = max(&self.security_strength, &other.security_strength()).clone();
+        self.key_type = min(&self.key_type, &other.key_type()).clone();
+        self.security_strength = min(&self.security_strength, &other.security_strength()).clone();
         Ok(self.key_len())
     }
 

crypto/core/tests/key_material_tests.rs

@@ -620,8 +620,9 @@ mod test_key_material {
         assert_eq!(zeroized_key.key_len(), 8);
         zeroized_key.concatenate(&key2).unwrap();
         assert_eq!(zeroized_key.key_len(), 24);
-        // should take max(Zeroized, BytesLowEntropy)
-        assert_eq!(zeroized_key.key_type(), KeyType::BytesLowEntropy);
+        // The result takes the lesser (min) of the two key types: min(Zeroized, BytesLowEntropy).
+        // Folding in zeroized (uninitialized) bytes taints the whole buffer as Zeroized.
+        assert_eq!(zeroized_key.key_type(), KeyType::Zeroized);
         assert_eq!(zeroized_key.security_strength(), SecurityStrength::None);
 
         // This should be symmetric, so test it in the other direction too.
@@ -634,8 +635,8 @@ mod test_key_material {
         let mut key2 = KeyMaterial256::from_bytes(&[1u8; 16]).unwrap();
         key2.concatenate(&zeroized_key).unwrap();
         assert_eq!(key2.key_len(), 24);
-        // should take max(Zeroized, BytesLowEntropy)
-        assert_eq!(key2.key_type(), KeyType::BytesLowEntropy);
+        // The result takes the lesser (min) of the two key types: min(BytesLowEntropy, Zeroized).
+        assert_eq!(key2.key_type(), KeyType::Zeroized);
         assert_eq!(key2.security_strength(), SecurityStrength::None);
 
         // now try it with keys of different key types
@@ -644,33 +645,34 @@ mod test_key_material {
         let full_entropy_key =
             KeyMaterial256::from_bytes_as_type(&[2u8; 16], KeyType::BytesFullEntropy).unwrap();
         low_entropy_key.concatenate(&full_entropy_key).unwrap();
-        // should take max(BytesLowEntropy, BytesFullEntropy)
-        assert_eq!(low_entropy_key.key_type(), KeyType::BytesFullEntropy);
-        // should take max(None, _128Bit)
-        assert_eq!(low_entropy_key.security_strength(), SecurityStrength::_128bit);
+        // Conservative model: concatenating a full-entropy key with a low-entropy key yields a
+        // low-entropy key. min(BytesLowEntropy, BytesFullEntropy) == BytesLowEntropy.
+        assert_eq!(low_entropy_key.key_type(), KeyType::BytesLowEntropy);
+        // min(None, _128bit) == None (and BytesLowEntropy keys must have strength None anyway).
+        assert_eq!(low_entropy_key.security_strength(), SecurityStrength::None);
 
         // and in the other direction too
         let low_entropy_key =
             KeyMaterial256::from_bytes_as_type(&[1u8; 16], KeyType::BytesLowEntropy).unwrap();
         let mut full_entropy_key =
             KeyMaterial256::from_bytes_as_type(&[2u8; 16], KeyType::BytesFullEntropy).unwrap();
         full_entropy_key.concatenate(&low_entropy_key).unwrap();
-        // should take max(BytesLowEntropy, BytesFullEntropy)
-        assert_eq!(full_entropy_key.key_type(), KeyType::BytesFullEntropy);
-        // should take max(None, _128Bit)
-        assert_eq!(full_entropy_key.security_strength(), SecurityStrength::_128bit);
+        // min(BytesFullEntropy, BytesLowEntropy) == BytesLowEntropy.
+        assert_eq!(full_entropy_key.key_type(), KeyType::BytesLowEntropy);
+        // min(_128bit, None) == None.
+        assert_eq!(full_entropy_key.security_strength(), SecurityStrength::None);
 
         // now with full entropy keys at different security levels
-        let mut low_entropy_key =
+        let mut full_entropy_key_112 =
             KeyMaterial512::from_bytes_as_type(&[1u8; 16], KeyType::BytesFullEntropy).unwrap();
         // Now we're gonna explictly tag it at the 112bit security level -- does not require allow_hazardous_operations().
-        low_entropy_key.set_security_strength(SecurityStrength::_112bit).unwrap();
+        full_entropy_key_112.set_security_strength(SecurityStrength::_112bit).unwrap();
         let full_entropy_key =
             KeyMaterial256::from_bytes_as_type(&[2u8; 32], KeyType::BytesFullEntropy).unwrap();
-        low_entropy_key.concatenate(&full_entropy_key).unwrap();
-        assert_eq!(low_entropy_key.key_type(), KeyType::BytesFullEntropy);
-        // should take max(_112Bit, _256Bit)
-        assert_eq!(low_entropy_key.security_strength(), SecurityStrength::_256bit);
+        full_entropy_key_112.concatenate(&full_entropy_key).unwrap();
+        assert_eq!(full_entropy_key_112.key_type(), KeyType::BytesFullEntropy);
+        // The combined key keeps the lower of the two security strengths: min(_112bit, _256bit).
+        assert_eq!(full_entropy_key_112.security_strength(), SecurityStrength::_112bit);
     }
 
     #[test]

crypto/mldsa_lowmemory/src/lib.rs

@@ -45,7 +45,7 @@
 //!
 //! We also get a surprising amount of memory-savings by good coding hygiene:
 //! Using un-named scopes to tell the compiler when an intermediate variable is no longer needed and
-//! con be popped off the stack. This sometimes requires re-ordering the steps of the algorithms given in
+//! can be popped off the stack. This sometimes requires re-ordering the steps of the algorithms given in
 //! FIPS 204 so that variables can be created, used, and released in a self-contained block.
 //! Sometimes this is not possible and we have to make a choice between keeping the variable around
 //! or releasing it and re-deriving it later.

crypto/utils/src/ct.rs

@@ -38,7 +38,9 @@ impl Condition<i64> {
     // MikeO: TODO: there are a bunch of impls in here that seem to be generic and not related to i64,
     // MikeO: TODO: could those be moved to a generic impl<T> for Condition<T> ?
 
-    pub const TRUE: Self = Self(1);
+    /// TRUE is the bit vector of all 1's
+    pub const TRUE: Self = Self(-1);
+    /// FALSE is the bit vector of all 0's
     pub const FALSE: Self = Self(0);
 
     pub const fn from_bool<const VALUE: bool>() -> Self {
@@ -111,13 +113,31 @@ impl Condition<i64> {
         *dst = self.select(src, *dst);
     }
 
-    // MikeO: TODO: I have no idea what this does, .negate(-1) seems to give -3 ?? Is that a bug?
     /// Conditionally negate the value.
+    ///
+    /// negate(-1) gives -3
+    ///
+    /// `value` is `-1` (i.e., all bits are `1`, `...1111`)
+    ///
+    /// Condition `self.0` is 1 (`...0001`) (assuming `TRUE`)
+    ///
+    /// XOR operation was executed as `value ^ self.0`
+    ///
+    /// Then `...1111 XOR ...0001 = ...1110` (i.e., `-2`)
+    ///
+    /// Subtraction operation is `wrapping_sub(self.0)`
+    ///
+    /// Then `-2 - 1 = -3`
+    ///
+    /// As a result, `1`, which is the negation of `-1`, should be returned, but `-3` is output.
+    ///
+    /// Therefore, if the [Self::TRUE] constant value of the i64 [Condition] implementation is changed to `-1`,
+    /// the test also runs normally.
     pub const fn negate(self, value: i64) -> i64 {
         (value ^ self.0).wrapping_sub(self.0)
     }
 
-    const fn or_halves(value: i64) -> i64 {
+    pub const fn or_halves(value: i64) -> i64 {
         (value | (value >> 32)) & 0xFFFFFFFF
     }
 
@@ -136,93 +156,36 @@ impl Condition<i64> {
     }
 }
 
-// TODO: ... this doesn't ... work. We should get this working and then then do u8.
+// TODO: We should do Condition<u8>.
 // TODO: then and change Hex and Base64 to use this.
 // TODO: (there's probably no noticeable performance difference u8 and u64 bit ops on a 64-bit machine,
 // TODO:  but there would be on a 8, 16, or 32-bit machine.)
-// impl Condition<u64> {
-//     pub const TRUE: Self = Self(1);
-//     pub const FALSE: Self = Self(0);
-// 
-//     pub const fn new<const VALUE: bool>() -> Self {
-//         Self((VALUE as u64).wrapping_neg())
-//     }
-// 
-//     pub const fn from_bool(value: bool) -> Self {
-//         Self((value as u64).wrapping_neg())
-//     }
-// 
-//     pub const fn is_bit_set(value: u64, bit: u64) -> Self {
-//         Self(((value >> bit) & 1).wrapping_neg())
-//     }
-// 
-//     // MikeO: TODO ?? What does "negative" mean for an unsigned value?
-//     pub const fn is_negative(value: u64) -> Self {
-//         Self(((value as i64) >> 63) as u64)
-//     }
-// 
-//     pub const fn is_not_zero(value: u64) -> Self {
-//         Self::is_negative(Self::or_halves(value).wrapping_neg())
-//     }
-// 
-//     pub const fn is_zero(value: u64) -> Self {
-//         Self::is_negative(Self::or_halves(value).wrapping_sub(1))
-//     }
-// 
-//     // MikeO: TODO: I borrowed this formula from Botan, but rust complains about u64 subtraction overflow if x < y, so this works in C but won't work in rust.
-//     // MikeO: TODO: I played with u64.wrapping_sub(y) but that doesn't work either.
-//     pub const fn is_lt(x: u64, y: u64) -> Self {
-//         Self::is_zero(x ^ ((x ^ y) | (x.wrapping_sub(y)) ^ x))
-//     }
-// 
-//     // Note: haven't found a clever way to make this const, since it either needs a (non-const) not (!) or a boolean OR is_zero.
-//     // pub fn is_lte(x: i64, y: i64) -> Self { !Self::is_gt(x, y) }
-// 
-//     // pub const fn is_gt(x: i64, y: i64) -> Self { Self::is_lt(y, x) }
-// 
-//     // Note: haven't found a clever way to make this const, since it either needs a (non-const) not (!) or a boolean OR is_zero.
-//     // pub fn is_gte(x: i64, y: i64) -> Self { !Self::is_lt(x, y) }
-// 
-//     pub fn is_in_list(value: u64, list: &[u64]) -> Self {
-//         // Research question: is this actually constant-time?
-//         // A clever compiler might turn this into a short-circuiting loop.
-//         // A quick google search shows that rust doesn't have the ability to annotate specific code blocks
-//         // as no-optimize; the only option is to insert direct assembly.
-// 
-//         let mut c = Self::FALSE;
-//         for i in 0..list.len() {
-//             let diff = value ^ list[i];
-//             c |= Condition::<u64>::is_zero(diff);
-//         }
-// 
-//         c
-//     }
-// 
-//     pub fn mov(self, src: u64, dst: &mut u64) {
-//         *dst = self.select(src, *dst);
-//     }
-// 
-//     // MikeO: TODO: This needs a docstring because I have no idea what this does.
-//     pub const fn negate(self, value: u64) -> u64 {
-//         (value ^ self.0).wrapping_sub(self.0)
-//     }
-// 
-//     const fn or_halves(value: u64) -> u64 {
-//         (value & 0xFFFFFFFF) | (value >> 32)
-//     }
-// 
-//     pub const fn select(self, true_value: u64, false_value: u64) -> u64 {
-//         (true_value & self.0) | (false_value & !self.0)
-//     }
-// 
-//     pub const fn swap(self, lhs: u64, rhs: u64) -> (u64, u64) {
-//         (self.select(rhs, lhs), self.select(lhs, rhs))
-//     }
-// 
-//     pub const fn to_bool_var(self) -> bool {
-//         self.0 != 0
-//     }
-// }
+impl Condition<u64> {
+    /// TRUE is the bit vector of all 1's
+    pub const TRUE: Self = Self(u64::MAX);
+    /// FALSE is the bit vector of all 0's
+    pub const FALSE: Self = Self(0);
+
+    // this is the core logic for constant-time mask generation for unsigned integers
+    //   Unlike signed integers where we can rely on Two's Complement via negation `-(v as i64)`,
+    //   for u64 we must use wrapping subtraction to achieve the all-ones bit pattern (u64::MAX) for true
+    pub const fn from_bool<const VALUE: bool>() -> Self {
+        // If VALUE is true (1) -> 0 - 1 = u64::MAX (All 1s)
+        // If VALUE is false (0) -> 0 - 0 = 0 (All 0s)
+        Self(0u64.wrapping_sub(VALUE as u64))
+    }
+
+    // the select function manually for u64
+    //    although a fully generic impl<T> would be the ultimate long-term goal
+    pub fn select(self, a: u64, b: u64) -> u64 {
+        let mask = self.0;
+        (a & mask) | (b & !mask)
+    }
+
+    pub fn is_true(&self) -> bool {
+        self.0 != 0
+    }
+}
 
 impl<T> BitAnd for Condition<T>
 where
@@ -327,22 +290,22 @@ pub fn conditional_copy_bytes<const LEN: usize>(
     a: &[u8; LEN],
     b: &[u8; LEN],
     out: &mut [u8; LEN],
-    take_a: bool) {
-    
-    // we want the behaviour of 
+    take_a: bool,
+) {
+    // we want the behaviour of
     //  if take_a { 0xFF } else { 0x00 }
     // but without using any branches that could leak timing signals
-    let mask: u8 = (take_a as u8) |
-        (take_a as u8) <<1 |
-        (take_a as u8) <<2 |
-        (take_a as u8) <<3 |
-        (take_a as u8) <<4 |
-        (take_a as u8) <<5 |
-        (take_a as u8) <<6 |
-        (take_a as u8) <<7;
-    
+    let mask: u8 = (take_a as u8)
+        | (take_a as u8) << 1
+        | (take_a as u8) << 2
+        | (take_a as u8) << 3
+        | (take_a as u8) << 4
+        | (take_a as u8) << 5
+        | (take_a as u8) << 6
+        | (take_a as u8) << 7;
+
     debug_assert_eq!(mask, if take_a { 0xFF } else { 0x00 });
-    
+
     for i in 0..LEN {
         out[i] = std::hint::black_box(a[i] & mask) | std::hint::black_box(b[i] & !mask);
     }