Skip to content

Commit c4f143d

Browse files
Santiago Cingolaniinorick
Santiago Cingolani
authored andcommittedOct 25, 2024·
Refactor unsafe code in raw_jobs.rs for better memory safety
A potential issue was present where the check to the pointed to memory was done independently to the cast to the Rust type. The check was meant to protect from casting memory in an invalid state, but did not protect from the memory being modified in between the check and the cast. This new version creates a copy on the stack _first_. This way, we can safely validate it and trust our own memory to still be valid when we call `assume_init()`.
1 parent e3a6fd1 commit c4f143d

File tree

1 file changed

+37
-7
lines changed

1 file changed

+37
-7
lines changed
 

‎heimlig/src/integration/raw_jobs.rs

+37-7
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use crate::common::jobs::{HashAlgorithm, Request, Response};
22
use crate::hsm::keystore::{Curve, KeyId};
33
use crate::integration::raw_errors::JobErrorRaw;
4-
use core::mem::offset_of;
4+
use core::mem::{offset_of, MaybeUninit};
55
use core::slice;
66
use strum::EnumCount;
77

@@ -475,16 +475,31 @@ impl RequestRaw {
475475
/// valid `RequestRaw` instance.
476476
///
477477
pub unsafe fn from_raw(ptr: *const u8) -> Result<Self, ValidationError> {
478+
// We create a copy of the untrusted request on the stack to prevent a potential
479+
// point-of-check/point-of-use issue
480+
let mut request: MaybeUninit<RequestRaw> = MaybeUninit::uninit();
481+
478482
// SAFETY: Pointer and size must be checked by integrator
479-
let tag: u8 = unsafe { *(ptr.add(offset_of!(RequestRaw, data))) };
483+
unsafe {
484+
core::ptr::copy(ptr.cast(), request.as_mut_ptr(), 1);
485+
}
486+
487+
// SAFETY: request is of type RequestRaw, so adding the offset of a field of such a struct
488+
// still falls under the object behind the pointer
489+
let tag: u8 = unsafe {
490+
*(request
491+
.as_ptr()
492+
.cast::<u8>()
493+
.add(offset_of!(RequestRaw, data)))
494+
};
480495

481496
// Validate tag value. Invalid tag values cause UB when transmuted into an enum.
482497
if tag >= RequestDataRaw::COUNT as u8 {
483498
return Err(ValidationError::InvalidTagValue);
484499
}
485500

486-
// SAFETY: All members of RequestRaw are valid for all possible values found in memory
487-
Ok(*ptr.cast::<RequestRaw>())
501+
// SAFETY: Besides the tag, which we checked, all other members are ok with any value
502+
Ok(unsafe { request.assume_init() })
488503
}
489504

490505
pub fn verify<'data>(
@@ -1701,16 +1716,31 @@ impl ResponseRaw {
17011716
/// valid `ResponseRaw` instance.
17021717
///
17031718
pub unsafe fn from_raw(ptr: *const u8) -> Result<Self, ValidationError> {
1719+
// We create a copy of the untrusted response on the stack to prevent a potential
1720+
// point-of-check/point-of-use issue
1721+
let mut response: MaybeUninit<ResponseRaw> = MaybeUninit::uninit();
1722+
17041723
// SAFETY: Pointer and size must be checked by integrator
1705-
let tag: u8 = unsafe { *(ptr.add(offset_of!(RequestRaw, data))) };
1724+
unsafe {
1725+
core::ptr::copy(ptr.cast(), response.as_mut_ptr(), 1);
1726+
}
1727+
1728+
// SAFETY: response is of type ResponseRaw, so adding the offset of a field of such a struct
1729+
// still falls under the object behind the pointer
1730+
let tag: u8 = unsafe {
1731+
*(response
1732+
.as_ptr()
1733+
.cast::<u8>()
1734+
.add(offset_of!(ResponseRaw, data)))
1735+
};
17061736

17071737
// Validate tag value. Invalid tag values cause UB when transmuted into an enum.
17081738
if tag >= ResponseDataRaw::COUNT as u8 {
17091739
return Err(ValidationError::InvalidTagValue);
17101740
}
17111741

1712-
// SAFETY: All members of RequestRaw are valid for all possible values found in memory
1713-
Ok(*ptr.cast::<ResponseRaw>())
1742+
// SAFETY: Besides the tag, which we checked, all other members are ok with any value
1743+
Ok(unsafe { response.assume_init() })
17141744
}
17151745
}
17161746

0 commit comments

Comments
 (0)
Please sign in to comment.