Skip to content

Commit b7ef62d

Browse files
authored
Security enhancements (#20)
This PR establishes compliance with: - No unwrap/expect/panic - All operations return proper Result types - Automatic secret zeroization - Keys cleared from memory when dropped - Constant-time operations - Prevents timing-based side-channel attacks - Comprehensive error handling - All edge cases properly managed ## Copilot Integration This PR introduces comprehensive GitHub Copilot instructions that automatically guide AI-assisted development toward secure, panic-free Rust code. ## Enhanced CI Pipeline - Security audit job with general security lints - cargo deny integration for dependency policy enforcement - Semgrep Rust security pattern scanning - Optimized caching strategy ## Cryptographic Improvements SecureKey<N> wrapper implementing Zeroize and ZeroizeOnDrop Constant-time MAC verification via verify_mac_constant_time() Enhanced MAC traits with secure key handling Comprehensive test coverage for security features
1 parent c9cd581 commit b7ef62d

File tree

19 files changed

+373
-64
lines changed

19 files changed

+373
-64
lines changed

.github/copilot-instructions.md

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
## Pull Request Review Checklist
2+
3+
- [ ] Code is completely panic-free (no unwrap/expect/panic/indexing)
4+
- [ ] All fallible operations return Result or Option
5+
- [ ] Integer operations use checked/saturating/wrapping methods where needed
6+
- [ ] Array/slice access uses get() or pattern matching, not direct indexing
7+
- [ ] Error cases are well documented and handled appropriately
8+
- [ ] Tests verify error handling paths, not just happy paths
9+
- [ ] Unsafe code blocks are documented with safety comments
10+
- [ ] Hardware register access uses proper volatile operations
11+
- [ ] Cryptographic operations use constant-time implementations where applicable
12+
13+
## Quick Reference: Forbidden Patterns
14+
15+
| Forbidden Pattern | Required Alternative |
16+
|-------------------|----------------------|
17+
| `value.unwrap()` | `match value { Some(v) => v, None => return Err(...) }` |
18+
| `result.expect("msg")` | `match result { Ok(v) => v, Err(e) => return Err(e.into()) }` |
19+
| `collection[index]` | `collection.get(index).ok_or(Error::OutOfBounds)?` |
20+
| `a + b` (integers) | `a.checked_add(b).ok_or(Error::Overflow)?` |
21+
| `ptr.read()` | `ptr.read_volatile()` (for MMIO) |
22+
23+
## Security-Specific Guidelines
24+
25+
- **Timing attacks**: Use constant-time comparisons for secrets (subtle crate)
26+
- **Zeroization**: Use `zeroize` crate for sensitive data cleanup (keys, passwords, etc.)
27+
- **Memory safety**: Ensure sensitive data is properly zeroized after use
28+
- **Hardware abstraction**: All register access must go through HAL traits
29+
- **Error information**: Don't leak sensitive data in error messages

.github/workflows/ci.yml

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,45 @@ jobs:
8989

9090
- name: Install Rust toolchain
9191
uses: dtolnay/rust-toolchain@stable
92+
with:
93+
components: clippy
94+
95+
- name: Cache cargo registry
96+
uses: actions/cache@v4
97+
with:
98+
path: |
99+
~/.cargo/registry
100+
~/.cargo/git
101+
target
102+
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
92103

93104
- name: Install cargo-audit
94-
run: cargo install cargo-audit
105+
run: cargo install cargo-audit --locked
106+
107+
- name: Install cargo-deny
108+
run: cargo install cargo-deny --version 0.18.3 --locked
95109

96110
- name: Run security audit
97111
run: cargo audit
112+
113+
- name: Run cargo deny checks
114+
run: cargo xtask deny
115+
116+
- name: Run security-focused clippy lints
117+
run: |
118+
cargo clippy --all-targets --all-features -- \
119+
-D warnings \
120+
-W clippy::arithmetic_side_effects \
121+
-W clippy::float_arithmetic \
122+
-W clippy::indexing_slicing \
123+
-W clippy::unwrap_used \
124+
-W clippy::expect_used \
125+
-W clippy::panic \
126+
-W clippy::mem_forget \
127+
-W clippy::multiple_unsafe_ops_per_block \
128+
-W clippy::undocumented_unsafe_blocks
129+
130+
- name: Run semgrep security scan
131+
uses: returntocorp/semgrep-action@v1
132+
with:
133+
config: p/rust

.semgrepignore

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
# Licensed under the Apache-2.0 license
2+
3+
# Ignore xtask development tools - not production code
4+
xtask/
5+
6+
# Ignore build artifacts
7+
target/
8+
9+
# Ignore external dependencies and examples
10+
external/

Cargo.lock

Lines changed: 28 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,5 +17,12 @@ members = [
1717
]
1818
resolver = "2"
1919

20+
[workspace.package]
21+
license = "Apache-2.0"
22+
repository = "https://github.com/rusty1968/openprot"
23+
edition = "2021"
24+
2025
[workspace.dependencies]
2126
zerocopy = { version = "0.8.25", features = ["derive"] }
27+
zeroize = { version = "1.7", features = ["derive"] }
28+
subtle = "2.5"

deny.toml

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
# Licensed under the Apache-2.0 license
2+
3+
# cargo-deny configuration file
4+
# Minimal configuration for OpenPRoT security checks
5+
6+
[advisories]
7+
version = 2
8+
ignore = []
9+
10+
[licenses]
11+
version = 2
12+
allow = [
13+
"MIT",
14+
"Apache-2.0",
15+
"Apache-2.0 WITH LLVM-exception",
16+
"BSD-2-Clause",
17+
"BSD-3-Clause",
18+
"ISC",
19+
"Unicode-DFS-2016",
20+
"Unicode-3.0",
21+
"CC0-1.0",
22+
]
23+
24+
[bans]
25+
multiple-versions = "warn"
26+
wildcards = "allow"
27+
deny = []
28+
29+
[sources]
30+
unknown-registry = "warn"
31+
unknown-git = "warn"
32+
allow-registry = ["https://github.com/rust-lang/crates.io-index"]

hal/blocking/Cargo.toml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,4 +9,6 @@ license = "Apache-2.0"
99

1010
[dependencies]
1111
embedded-hal = "1.0"
12-
zerocopy = { workspace = true }
12+
zerocopy = { workspace = true }
13+
zeroize = { workspace = true }
14+
subtle = { workspace = true }

hal/blocking/src/mac.rs

Lines changed: 109 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,87 @@
22

33
use crate::digest::Digest;
44
use core::fmt::Debug;
5+
use subtle::ConstantTimeEq;
56
use zerocopy::IntoBytes;
7+
use zeroize::{Zeroize, ZeroizeOnDrop};
68

7-
/// Common error kinds for MAC operations (reused from digest operations).
9+
/// Secure wrapper for MAC keys that automatically zeros on drop.
10+
///
11+
/// This wrapper ensures that cryptographic keys are securely erased from memory
12+
/// when no longer needed, preventing key material from remaining in memory.
13+
#[derive(Clone, Zeroize, ZeroizeOnDrop)]
14+
pub struct SecureKey<const N: usize> {
15+
/// The actual key bytes, zeroized on drop
16+
bytes: [u8; N],
17+
}
18+
19+
impl<const N: usize> SecureKey<N> {
20+
/// Create a new secure key from a byte array.
21+
///
22+
/// # Security
23+
/// The input array will be zeroized after copying to prevent key material
24+
/// from remaining in multiple memory locations.
25+
pub fn new(mut key_bytes: [u8; N]) -> Self {
26+
let key = Self { bytes: key_bytes };
27+
key_bytes.zeroize();
28+
key
29+
}
30+
31+
/// Create a new secure key from a byte slice.
32+
///
33+
/// # Returns
34+
/// - `Ok(SecureKey)` if the slice length matches the key size
35+
/// - `Err(ErrorKind::InvalidInputLength)` if the slice is the wrong size
36+
pub fn from_slice(key_slice: &[u8]) -> Result<Self, ErrorKind> {
37+
if key_slice.len() != N {
38+
return Err(ErrorKind::InvalidInputLength);
39+
}
40+
41+
let mut key_bytes = [0u8; N];
42+
key_bytes.copy_from_slice(key_slice);
43+
Ok(Self::new(key_bytes))
44+
}
45+
46+
/// Get a reference to the key bytes.
47+
///
48+
/// # Security
49+
/// Use this sparingly and ensure the returned reference doesn't outlive
50+
/// the SecureKey instance.
51+
pub fn as_bytes(&self) -> &[u8; N] {
52+
&self.bytes
53+
}
54+
55+
/// Verify a MAC tag using constant-time comparison.
56+
///
57+
/// # Security
58+
/// This function uses constant-time comparison to prevent timing attacks
59+
/// that could reveal information about the expected MAC value.
60+
pub fn verify_mac(&self, computed_mac: &[u8], expected_mac: &[u8]) -> bool {
61+
if computed_mac.len() != expected_mac.len() {
62+
return false;
63+
}
64+
computed_mac.ct_eq(expected_mac).into()
65+
}
66+
}
67+
68+
impl<const N: usize> Debug for SecureKey<N> {
69+
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
70+
f.debug_struct("SecureKey")
71+
.field("len", &N)
72+
.field("bytes", &"[REDACTED]")
73+
.finish()
74+
}
75+
}
76+
77+
impl<const N: usize> PartialEq for SecureKey<N> {
78+
fn eq(&self, other: &Self) -> bool {
79+
self.bytes.ct_eq(&other.bytes).into()
80+
}
81+
}
82+
83+
impl<const N: usize> Eq for SecureKey<N> {}
84+
85+
/// Common error kinds for MAC operations.
886
#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
987
#[non_exhaustive]
1088
pub enum ErrorKind {
@@ -30,6 +108,8 @@ pub enum ErrorKind {
30108
PermissionDenied,
31109
/// The MAC computation context has not been initialized.
32110
NotInitialized,
111+
/// MAC verification failed - computed MAC does not match expected value.
112+
VerificationFailed,
33113
}
34114

35115
/// Trait for converting implementation-specific errors into a common error kind.
@@ -116,6 +196,31 @@ pub trait MacOp: ErrorType {
116196
fn finalize(self) -> Result<Self::Output, Self::Error>;
117197
}
118198

199+
/// Utility function for constant-time MAC verification.
200+
///
201+
/// This function provides a secure way to verify MAC values using constant-time
202+
/// comparison to prevent timing attacks.
203+
///
204+
/// # Parameters
205+
///
206+
/// - `computed_mac`: The computed MAC bytes
207+
/// - `expected_mac`: The expected MAC bytes to verify against
208+
///
209+
/// # Returns
210+
///
211+
/// `true` if the MACs match, `false` otherwise
212+
///
213+
/// # Security
214+
///
215+
/// This function uses constant-time comparison to prevent timing attacks
216+
/// that could reveal information about the expected MAC value.
217+
pub fn verify_mac_constant_time(computed_mac: &[u8], expected_mac: &[u8]) -> bool {
218+
if computed_mac.len() != expected_mac.len() {
219+
return false;
220+
}
221+
computed_mac.ct_eq(expected_mac).into()
222+
}
223+
119224
// =============================================================================
120225
// MAC Algorithm Marker Types
121226
// =============================================================================
@@ -133,7 +238,7 @@ pub struct HmacSha2_256;
133238
impl MacAlgorithm for HmacSha2_256 {
134239
const OUTPUT_BITS: usize = 256;
135240
type MacOutput = Digest<{ Self::OUTPUT_BITS / 32 }>;
136-
type Key = [u8; 32];
241+
type Key = SecureKey<32>;
137242
}
138243

139244
/// HMAC-SHA-384 MAC algorithm marker type.
@@ -149,7 +254,7 @@ pub struct HmacSha2_384;
149254
impl MacAlgorithm for HmacSha2_384 {
150255
const OUTPUT_BITS: usize = 384;
151256
type MacOutput = Digest<{ Self::OUTPUT_BITS / 32 }>;
152-
type Key = [u8; 48];
257+
type Key = SecureKey<48>;
153258
}
154259

155260
/// HMAC-SHA-512 MAC algorithm marker type.
@@ -165,5 +270,5 @@ pub struct HmacSha2_512;
165270
impl MacAlgorithm for HmacSha2_512 {
166271
const OUTPUT_BITS: usize = 512;
167272
type MacOutput = Digest<{ Self::OUTPUT_BITS / 32 }>;
168-
type Key = [u8; 64];
273+
type Key = SecureKey<64>;
169274
}

openprot/Cargo.toml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@
33
[package]
44
name = "openprot"
55
version = "0.1.0"
6-
edition = "2021"
6+
edition.workspace = true
7+
license.workspace = true
8+
repository.workspace = true
79

810
[[bin]]
911
name = "openprot"

openprot/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Licensed under the Apache-2.0 license
22

33
pub fn greet(name: &str) -> String {
4-
format!("Hello, {}!", name)
4+
format!("Hello, {name}!")
55
}
66

77
#[cfg(test)]

0 commit comments

Comments
 (0)