Skip to content

Commit

Permalink
Add //SAFETY comments
Browse files Browse the repository at this point in the history
  • Loading branch information
efer-ms committed Nov 26, 2024
1 parent 0d729cc commit d5ac1dd
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 42 deletions.
64 changes: 34 additions & 30 deletions wincrypto/src/cert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,51 +10,51 @@ use windows::{
},
};

/// Certificate wraps the CERT_CONTEXT pointer, so that it can be destroyed
/// when it is no longer used. Because it is tracked, it is important that
/// Certificate does NOT implement Clone/Copy, otherwise we could destroy the
/// Certificate too early. It is also why access to the certificate pointer
/// should remain hidden.
#[derive(Debug)]
pub struct Certificate(pub(crate) *const CERT_CONTEXT);
// SAFETY: CERT_CONTEXT pointers are safe to send between threads.
unsafe impl Send for Certificate {}
// SAFETY: CERT_CONTEXT pointers are safe to send between threads.
unsafe impl Sync for Certificate {}

impl Certificate {
pub fn new_self_signed(subject: &str) -> Result<Self, WinCryptoError> {
let subject = HSTRING::from(subject);
let mut subject_blob_buffer = vec![0u8; 256];
let mut subject_blob = CRYPT_INTEGER_BLOB {
cbData: subject_blob_buffer.len() as u32,
pbData: subject_blob_buffer.as_mut_ptr(),
};

// Use RSA-SHA256 for the signature, since SHA1 is deprecated.
let signature_algorithm = CRYPT_ALGORITHM_IDENTIFIER {
pszObjId: PSTR::from_raw(szOID_RSA_SHA256RSA.as_ptr() as *mut u8),
Parameters: CRYPT_INTEGER_BLOB::default(),
};

// SAFETY: The Windows APIs accept references, so normal borrow checker
// behaviors work for those uses. The name_blob has a pointer to the buffer
// which must exist for the duration of the unsafe block.
unsafe {
let subject = HSTRING::from(subject);
let mut name_blob = CRYPT_INTEGER_BLOB::default();

// Ask size needed to store Name Blob.
CertStrToNameW(
X509_ASN_ENCODING,
&subject,
CERT_OID_NAME_STR,
None,
None,
&mut name_blob.cbData,
Some(subject_blob.pbData),
&mut subject_blob.cbData,
None,
)?;

// Create buffer for the name blob, and get it filled in.
let mut name_buffer = vec![0u8; name_blob.cbData as usize];
name_blob.pbData = name_buffer.as_mut_ptr();
CertStrToNameW(
X509_ASN_ENCODING,
&subject,
CERT_OID_NAME_STR,
None,
Some(name_blob.pbData),
&mut name_blob.cbData,
None,
)?;

// Use RSA-SHA256 for the signature, since SHA1 is deprecated.
let signature_algorithm = CRYPT_ALGORITHM_IDENTIFIER {
pszObjId: PSTR::from_raw(szOID_RSA_SHA256RSA.as_ptr() as *mut u8),
Parameters: CRYPT_INTEGER_BLOB::default(),
};

// Generate the self-signed cert.
let cert_context = CertCreateSelfSignCertificate(
HCRYPTPROV_OR_NCRYPT_KEY_HANDLE(0),
&name_blob,
&subject_blob,
CERT_CREATE_SELFSIGN_FLAGS(0),
None,
Some(&signature_algorithm),
Expand All @@ -74,9 +74,13 @@ impl Certificate {
}

pub fn sha256_fingerprint(&self) -> Result<[u8; 32], WinCryptoError> {
let mut hash = [0u8; 32];
let mut hash_handle = BCRYPT_HASH_HANDLE::default();

// SAFETY: The Windows APIs accept references, so normal borrow checker
// behaviors work for those uses.
unsafe {
// Create the hash instance.
let mut hash_handle = BCRYPT_HASH_HANDLE::default();
if let Err(e) = WinCryptoError::from_ntstatus(BCryptCreateHash(
BCRYPT_SHA256_ALG_HANDLE,
&mut hash_handle,
Expand All @@ -101,14 +105,12 @@ impl Certificate {
}

// Grab the result of the hash.
let mut hash = [0u8; 32];
WinCryptoError::from_ntstatus(BCryptFinishHash(hash_handle, &mut hash, 0))?;

// Destroy the allocated hash.
WinCryptoError::from_ntstatus(BCryptDestroyHash(hash_handle))?;

Ok(hash)
}
Ok(hash)
}
}

Expand All @@ -120,6 +122,8 @@ impl From<*const CERT_CONTEXT> for Certificate {

impl Drop for Certificate {
fn drop(&mut self) {
// SAFETY: The Certificate is no longer usable, so it's safe to pass the pointer
// to Windows for release.
unsafe {
_ = CertFreeCertificateContext(Some(self.0));
}
Expand Down
43 changes: 38 additions & 5 deletions wincrypto/src/dtls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ impl Dtls {
// These are the outputs of AcquireCredentialsHandleA
let mut cred_handle = SecHandle::default();
let mut creds_expiry: i64 = 0;

// SAFETY: The references passed are all borrow checked, the exception to
// this is the pointer to the cert_context in schannel_cred, which
// is kept alive by the `self.cert`.
unsafe {
AcquireCredentialsHandleW(
None,
Expand Down Expand Up @@ -258,7 +262,12 @@ impl Dtls {
pBuffers: &sec_buffers[0] as *const _ as *mut _,
};

// SAFETY: The references passed are all borrow checked. However,
// it is important to note that `sec_buffer_desc` holds a pointer to
// a sequence of SecBuffers. Those SecBuffers must exist for the
// duration of the unsafe block.
let status = unsafe { EncryptMessage(security_ctx, 0, &sec_buffer_desc, 0) };

match status {
SEC_E_OK => {
self.output_datagrams.push_back(output);
Expand Down Expand Up @@ -337,6 +346,11 @@ impl Dtls {
};

let mut attrs = 0;

// SAFETY: The references passed are all borrow checked. However,
// it is important to note that `in_buffer_desc` and `out_buffer_desc`
// hold pointers to sequence of SecBuffers. Those SecBuffers must exist
// for the duration of the unsafe block.
let status = unsafe {
if is_client {
// Client
Expand Down Expand Up @@ -380,6 +394,7 @@ impl Dtls {
)
}
};

debug!("DTLS Handshake status: {status}");
self.security_ctx = Some(new_ctx_handle);
if out_buffers[0].cbBuffer > 0 {
Expand Down Expand Up @@ -416,6 +431,7 @@ impl Dtls {
return Err(WinCryptoError("Security context missing".to_string()));
};

// SAFETY: The references used in the unsafe block are all borrow checked.
unsafe {
QueryContextAttributesW(
security_ctx as *const _,
Expand Down Expand Up @@ -444,6 +460,8 @@ impl Dtls {
};
let mut keying_material = SecPkgContext_KeyingMaterial::default();

// SAFETY: The references used in the unsafe block are all borrow checked. The
// only pointers used in those structs are to statically defined values.
let srtp_keying_material = unsafe {
SetContextAttributesW(
security_ctx as *const _,
Expand All @@ -468,20 +486,24 @@ impl Dtls {
))
})?;

std::slice::from_raw_parts(
// Copy the returned keying material to a Vec.
let keying_material_vec = std::slice::from_raw_parts(
keying_material.pbKeyingMaterial,
keying_material.cbKeyingMaterial as usize,
)
.to_vec()
};
.to_vec();

unsafe {
// Now that we copied the key to the Rust heap, we can free the buffer.
FreeContextBuffer(keying_material.pbKeyingMaterial as *mut _ as *mut std::ffi::c_void)
.map_err(|e| {
WinCryptoError(format!("FreeContextBuffer Keying Material: {:?}", e))
})?;
}

keying_material_vec
};

// SAFETY: All the passed in values are borrow checked. The raw CERT_CONTEXT
// pointer does not escpae the block, to avoid leaking the unwrapped pointer.
let peer_certificate: Certificate = unsafe {
let mut peer_cert_context: *mut CERT_CONTEXT = std::ptr::null_mut();
QueryContextAttributesW(
Expand All @@ -492,6 +514,7 @@ impl Dtls {
.map_err(|e| WinCryptoError(format!("QueryContextAttributesW: {:?}", e)))?;
(peer_cert_context as *const CERT_CONTEXT).into()
};

let peer_fingerprint = peer_certificate.sha256_fingerprint()?;

self.state = EstablishmentState::Established;
Expand Down Expand Up @@ -551,6 +574,9 @@ impl Dtls {
pBuffers: &sec_buffers[0] as *const _ as *mut _,
};

// SAFETY: All the passed in values are borrow checked. The `sec_buffer_desc`
// however holds pointers to a SecBuffer list. It's important that those buffers
// exist through this unsafe block.
let status = unsafe { DecryptMessage(security_ctx, &sec_buffer_desc, 0, None) };
match status {
SEC_E_OK => {
Expand All @@ -577,6 +603,11 @@ impl Dtls {
self.state = EstablishmentState::Handshaking;
let data = token_buffer.pvBuffer as *mut u8;
let len = token_buffer.cbBuffer as usize;

// SAFETY: We don't want to copy the data, so we will create a slice
// from the pointer and length, and pass it on. The pointer is required
// to actually be part of all of the buffers passed in, so the slice
// is valid so long as `sec_buffers` is.
self.handshake(Some(unsafe { std::slice::from_raw_parts(data, len) }))
} else {
Err(WinCryptoError("Renegotiate didn't include a token".to_string()).into())
Expand All @@ -593,6 +624,8 @@ impl Dtls {

impl Drop for Dtls {
fn drop(&mut self) {
// SAFETY: The handles here are no longer needed and cannot be accessed outside
// this struct, so it's safe to Delete/Free them here.
unsafe {
if let Some(ctx_handle) = self.security_ctx {
if let Err(e) = DeleteSecurityContext(&ctx_handle) {
Expand Down
2 changes: 2 additions & 0 deletions wincrypto/src/sha1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ use windows::Win32::Security::Cryptography::{
};

pub fn sha1_hmac(key: &[u8], payloads: &[&[u8]]) -> Result<[u8; 20], WinCryptoError> {
// SAFETY: The Windows APIs accept references, so normal borrow checker
// behaviors work for these uses.
unsafe {
// Create hash.
let mut hash_handle = BCRYPT_HASH_HANDLE::default();
Expand Down
39 changes: 32 additions & 7 deletions wincrypto/src/srtp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,15 @@ use windows::Win32::Security::Cryptography::{
const MAX_BUFFER_SIZE: usize = 2048;
const AEAD_AES_GCM_TAG_LEN: usize = 16;

/// SRTP Key wraps the CNG key, so that it can be destroyed when it is
/// no longer used. Because it is tracked, it is important that StrpKey
/// does NOT implement Clone/Copy, otherwise we could destroy the key
/// too early. It is also why access to the key handle should remain
/// hidden.
pub struct SrtpKey(BCRYPT_KEY_HANDLE);
// SAFETY: BCRYPT_KEY_HANDLEs are safe to send between threads.
unsafe impl Send for SrtpKey {}
// SAFETY: BCRYPT_KEY_HANDLEs are safe to send between threads.
unsafe impl Sync for SrtpKey {}

impl SrtpKey {
Expand All @@ -24,6 +31,7 @@ impl SrtpKey {
/// Creates a key from the given data for operating AES in ECB mode.
pub fn create_aes_ecb_key(key: &[u8]) -> Result<Self, WinCryptoError> {
let mut key_handle = BCRYPT_KEY_HANDLE::default();
// SAFETY: The key and key_handle will exist before and after this call.
unsafe {
WinCryptoError::from_ntstatus(BCryptGenerateSymmetricKey(
BCRYPT_AES_ECB_ALG_HANDLE,
Expand All @@ -39,6 +47,7 @@ impl SrtpKey {
/// Creates a key from the given data for operating AES in GCM mode.
pub fn create_aes_gcm_key(key: &[u8]) -> Result<Self, WinCryptoError> {
let mut key_handle = BCRYPT_KEY_HANDLE::default();
// SAFETY: The key and key_handle will exist before and after this call.
unsafe {
WinCryptoError::from_ntstatus(BCryptGenerateSymmetricKey(
BCRYPT_AES_GCM_ALG_HANDLE,
Expand All @@ -54,6 +63,8 @@ impl SrtpKey {

impl Drop for SrtpKey {
fn drop(&mut self) {
// SAFETY: The SrtpKey is being dropped it is safe to copy the handle
// because the handle will no longer be accessible after this.
unsafe {
if let Err(e) = WinCryptoError::from_ntstatus(BCryptDestroyKey(self.0)) {
error!("Failed to destory crypto key: {}", e);
Expand All @@ -68,8 +79,10 @@ pub fn srtp_aes_128_ecb_round(
input: &[u8],
output: &mut [u8],
) -> Result<usize, WinCryptoError> {
let mut count = 0;
// SAFETY: The Windows API accepts references, so normal borrow checker
// behaviors work.
unsafe {
let mut count = 0;
WinCryptoError::from_ntstatus(BCryptEncrypt(
key.0,
Some(input),
Expand All @@ -79,8 +92,8 @@ pub fn srtp_aes_128_ecb_round(
&mut count,
BCRYPT_BLOCK_PADDING,
))?;
Ok(count as usize)
}
Ok(count as usize)
}

/// Run the given input through the AES-128-CM using the given AES CTR/CM key.
Expand All @@ -90,8 +103,8 @@ pub fn srtp_aes_128_cm(
input: &[u8],
output: &mut [u8],
) -> Result<usize, WinCryptoError> {
// First, we'll make a copy of the IV with a countered as many times as needed into a new
// countered_iv.
// First, we'll make a copy of the IV with a countered as many times as
// needed into a new countered_iv.
let mut iv = iv.to_vec();
let mut countered_iv = [0u8; MAX_BUFFER_SIZE];
let mut offset = 0;
Expand All @@ -113,10 +126,14 @@ pub fn srtp_aes_128_cm(
}

let mut count = 0;
// SAFETY: The Windows API accepts references, so normal borrow checker
// behaviors work. The spare reference used to pass a mutable and immutable
// reference into the BCryptEncypt method relies on `countered_iv` which exists
// beyond the unsafe block.
unsafe {
// Now, we'll encrypt the countered IV. CNG can do this in-place, so we'll need a separate
// reference to the slice, but fool the borrow-checker, otherwise it won't like us passing
// the immutable and mutable reference to BCryptEncrypt.
// Now, we'll encrypt the countered IV. CNG can do this in-place, so we'll need
// a separate reference to the slice, but fool the borrow-checker, otherwise it
// won't like us passing the immutable and mutable reference to BCryptEncrypt.
let encrypted_countered_iv =
std::slice::from_raw_parts_mut(countered_iv.as_mut_ptr(), countered_iv.len());
WinCryptoError::from_ntstatus(BCryptEncrypt(
Expand Down Expand Up @@ -171,6 +188,10 @@ pub fn srtp_aead_aes_128_gcm_encrypt(
};

let mut count = 0;
// SAFETY: The Windows API accepts references, so normal borrow checker
// behaviors work for those. The `auth_cipher_mode_info` however, contains
// pointers. It is important that the data pointed to there (`additional_auth_data`,
// `cipher_text` and `iv`) exists for the duration of the unsafe block.
unsafe {
WinCryptoError::from_ntstatus(BCryptEncrypt(
key.0,
Expand Down Expand Up @@ -225,6 +246,10 @@ pub fn srtp_aead_aes_128_gcm_decrypt(
};

let mut count = 0;
// SAFETY: The Windows API accepts references, so normal borrow checker
// behaviors work for those. The `auth_cipher_mode_info` however, contains
// pointers. It is important that the data pointed to there (`additional_auth_data`,
// `cipher_text` and `iv`) exists for the duration of the unsafe block.
unsafe {
WinCryptoError::from_ntstatus(BCryptDecrypt(
key.0,
Expand Down

0 comments on commit d5ac1dd

Please sign in to comment.