Skip to content

Commit 9d3729a

Browse files
committed
fix(stack-encrypt)!: wipe the parts an owned split has not yet handed over
Encryption::split ran the first part's description while the other part sat in a local, so a panic in that description (a field declaration's selector, say) dropped the later sealed fields of an owned struct record unwiped. split now takes a wipe for the waiting part and holds it in a guard that calls it during the unwind; the part leaves the guard only once the first description has returned. The derive writes each wipe to zeroize the sealed fields still waiting and leave the passthrough ones. A sealed field's plaintext is already Zeroize, as every EncryptTarget source is, so no bound is added, and passthrough field types take none. BREAKING CHANGE: Encryption::split takes a fourth argument, wipe: fn(&mut U).
1 parent b40cf93 commit 9d3729a

7 files changed

Lines changed: 166 additions & 11 deletions

File tree

‎conformance/stack-encrypt/v1/lifecycle-profiles.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,9 @@
182182
],
183183
"owned-records": [
184184
"the_owned_record_wipes_sealed_fields_and_moves_passthrough_fields",
185-
"the_owned_record_wipes_sealed_fields_when_the_context_is_refused"
185+
"the_owned_record_wipes_sealed_fields_when_the_context_is_refused",
186+
"a_panicking_field_declaration_wipes_the_sealed_fields_after_it",
187+
"a_declaration_that_panics_while_constructed_wipes_the_plaintext"
186188
],
187189
"guest-abi": [
188190
"buffers::tests::wipe_all_leaves_no_live_buffer",

‎packages/stack-encrypt-derive/docs/attributes.md‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,9 +102,11 @@ carrying it through: the owned conversion destructures the struct, and a
102102
field it does neither would be dropped unwiped, so it is refused at the
103103
`struct = ..` path ("pattern does not mention field"). For the same reason
104104
the plaintext struct cannot implement `Drop` (so not `ZeroizeOnDrop`
105-
either): a type with a destructor cannot be destructured. On decryption a
106-
field opened before a later field is refused is wiped, not dropped as it
107-
is.
105+
either): a type with a destructor cannot be destructured. A field's
106+
declaration that panics wipes the sealed fields not yet handed to their
107+
own declarations, and a record's declaration that panics while it is
108+
constructed wipes the whole plaintext. On decryption a field opened before
109+
a later field is refused is wiped, not dropped as it is.
108110

109111
A `plaintext` record's fields are all handed the one context the record
110112
requires, so a term beside an `AeadContext`-only record is refused at the

‎packages/stack-encrypt-derive/src/owned_encrypt.rs‎

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -204,15 +204,20 @@ fn struct_derive(input: &DeriveInput, record: &Record) -> TokenStream {
204204
[first, rest @ ..] => {
205205
// `rest` as one description over the nested tuple of its parts,
206206
// divided by identity; then the struct divided into `first` and
207-
// that tuple.
207+
// that tuple. Each `split` is told how to wipe the parts still
208+
// waiting for their descriptions, should an earlier field's
209+
// description panic.
208210
let mut tail = {
209211
let last = rest[rest.len() - 1];
210212
quote!(#last)
211213
};
212-
for local in rest[..rest.len() - 1].iter().rev() {
213-
tail = quote!(#local.split(#tail, |__parts| __parts));
214+
for index in (1..locals.len() - 1).rev() {
215+
let local = locals[index];
216+
let wipe = wipe_waiting(krate, &fields[index + 1..]);
217+
tail = quote!(#local.split(#tail, |__parts| __parts, #wipe));
214218
}
215-
quote!(#first.split(#tail, #divide))
219+
let wipe = wipe_waiting(krate, &fields[1..]);
220+
quote!(#first.split(#tail, #divide, #wipe))
216221
}
217222
[] => unreachable!("a record has a derived field"),
218223
};
@@ -251,6 +256,37 @@ fn struct_derive(input: &DeriveInput, record: &Record) -> TokenStream {
251256
quote!(#encrypt_target #bridge)
252257
}
253258

259+
/// The `wipe` a `split` is handed for the parts still waiting, the nested
260+
/// tuple of `waiting`: every sealed field in it wiped, every passthrough
261+
/// field left as it is. A sealed field's plaintext is `Zeroize` already, as
262+
/// every `EncryptTarget` source is, so this adds no bound; a passthrough
263+
/// field is stored in the clear and takes none.
264+
fn wipe_waiting(krate: &syn::Path, waiting: &[&Field]) -> TokenStream {
265+
let mut wipes = Vec::new();
266+
let mut base = quote!((*__parts));
267+
for (index, field) in waiting.iter().enumerate() {
268+
let last = index == waiting.len() - 1;
269+
let place = if last {
270+
base.clone()
271+
} else {
272+
let first = syn::Index::from(0);
273+
quote!(#base.#first)
274+
};
275+
if field.is_derived() {
276+
wipes.push(quote!(#krate::target::Zeroize::zeroize(&mut #place);));
277+
}
278+
if !last {
279+
let rest = syn::Index::from(1);
280+
base = quote!(#base.#rest);
281+
}
282+
}
283+
if wipes.is_empty() {
284+
quote!(|_| {})
285+
} else {
286+
quote!(|__parts| { #(#wipes)* })
287+
}
288+
}
289+
254290
/// The nested tuple `(a, (b, (c, d)))` of the locals: how `split` divides
255291
/// a struct, one part per step.
256292
fn nested_tuple(locals: &[&Ident]) -> TokenStream {

‎packages/stack-encrypt/CHANGELOG.md‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
6666
the pending is returned; `EncryptInto` and the owned column run it. A
6767
record overrides it to divide the struct between its fields with the new
6868
`Encryption::split` (two descriptions over two parts of one value, one
69-
context) and `Encryption::project_owned`. `indexes` is what a plan field
69+
context, and a `wipe` for the part that waits while the first part's
70+
description runs, called if that description panics) and
71+
`Encryption::project_owned`. `EncryptInto` guards the plaintext until the
72+
declaration and its context have been constructed, so a declaration that
73+
panics while it is constructed wipes it. `indexes` is what a plan field
7074
typed with the target answers queries by; the terms, `Encrypted<Terms>`
7175
and `plaintext` records report theirs.
7276
- **The core leaves implement the owned declarations**: `StackCipherText`

‎packages/stack-encrypt/docs/adr/0012-owned-record-declarations.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,20 @@ function pointer that destructures the struct: it captures nothing, so like
5454
the accepted plaintext-ownership rule made mechanical: the conversion wipes
5555
what it seals and transfers what the caller chose to keep in the clear.
5656

57+
Each `split` runs its first part's description while the other part waits,
58+
and it is handed a `wipe` for that waiting part. If the first description
59+
panics (a declaration's selector, say), the waiting part goes to `wipe`
60+
during the unwind rather than being dropped as it is, as an owned column's
61+
unvisited elements are. The derive writes each `wipe` to zeroize the sealed
62+
fields still waiting and to leave the passthrough ones. A sealed field's
63+
plaintext is `Zeroize` already, as every `EncryptTarget` source is, so this
64+
adds no bound, and a passthrough field, stored in the clear, takes none. A
65+
`Zeroize` bound on `split`'s waiting part would have done the same work,
66+
but it would have fallen on passthrough field types too. `EncryptInto`
67+
holds the whole plaintext in a `Protected` guard until the declaration and
68+
its context exist, so a declaration that panics while it is constructed
69+
wipes the plaintext too.
70+
5771
The destructure is exhaustive. The owned form replaces the guard over the
5872
whole value, so a plaintext field the record neither seals nor carries
5973
would be dropped unwiped; it is a compile error at the `struct = ..` path

‎packages/stack-encrypt/src/target/operations.rs‎

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -416,23 +416,65 @@ impl<'s, S: 's, T: 'static, K: 'static, Ctx: 's> Encryption<'s, S, T, K, Ctx, Ow
416416
/// nesting a `split` per field, so a passthrough field is moved into
417417
/// the output and never copied, and a sealed field is guarded by its own
418418
/// target's description. Nothing is cloned, so `P` need not be `Clone`.
419+
///
420+
/// `self` runs first while `other`'s part waits, and `wipe` is what
421+
/// keeps that part safe meanwhile: if `self`'s description panics
422+
/// before `other` has received its part, the part is handed to `wipe`
423+
/// during the unwind instead of being dropped as it is. `wipe` wipes
424+
/// what `other` seals and leaves what it stores in the clear, so a part
425+
/// that is all passthrough takes `|_| {}`, and no `Zeroize` bound falls
426+
/// on a part that is not sealed. Once `other` has its part, wiping it is
427+
/// `other`'s responsibility, as it is for any owned source.
419428
pub fn split<P: 's, U: 's, V: 'static>(
420429
self,
421430
other: Encryption<'s, U, V, K, Ctx, Owned>,
422431
divide: fn(P) -> (S, U),
432+
wipe: fn(&mut U),
423433
) -> Encryption<'s, P, (T, V), K, Ctx, Owned>
424434
where
425435
Ctx: Clone,
426436
{
427437
Encryption {
428-
build: Box::new(move |source, cipher, cx| {
438+
build: Box::new(move |source, cipher, cx: Ctx| {
439+
let first_cx = cx.clone();
429440
let (mine, theirs) = divide(source);
430-
(self.build)(mine, cipher, cx.clone()).zip((other.build)(theirs, cipher, cx))
441+
let waiting = Waiting {
442+
part: Some(theirs),
443+
wipe,
444+
};
445+
let first = (self.build)(mine, cipher, first_cx);
446+
first.zip((other.build)(waiting.hand_over(), cipher, cx))
431447
}),
432448
}
433449
}
434450
}
435451

452+
/// The part of a [`split`](Encryption::split) value that waits while the
453+
/// other part's description runs. Dropped before
454+
/// [`hand_over`](Self::hand_over), by an unwind out of that description, it
455+
/// hands the part to `wipe`.
456+
struct Waiting<U> {
457+
part: Option<U>,
458+
wipe: fn(&mut U),
459+
}
460+
461+
impl<U> Waiting<U> {
462+
/// The description before it returned: the part goes to its own.
463+
fn hand_over(mut self) -> U {
464+
self.part
465+
.take()
466+
.unwrap_or_else(|| unreachable!("a waiting part is handed over once"))
467+
}
468+
}
469+
470+
impl<U> Drop for Waiting<U> {
471+
fn drop(&mut self) {
472+
if let Some(part) = self.part.as_mut() {
473+
(self.wipe)(part);
474+
}
475+
}
476+
}
477+
436478
/// A description whose operation is chosen by the value it is handed, at
437479
/// the moment it runs: `choose` sees the source, the keyset cipher and the
438480
/// context, and returns the pending of whichever typed operation applies.

‎packages/stack-encrypt/tests/owned_records.rs‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,6 +371,61 @@ impl Decryptable for FlaggedCiphertext {
371371
const DECRYPTABLE: bool = false;
372372
}
373373

374+
#[derive(Zeroize)]
375+
struct Trio {
376+
first: Flagged,
377+
#[zeroize(skip)]
378+
handle: u64,
379+
third: Flagged,
380+
fourth: Flagged,
381+
}
382+
// Only encrypted, and never completed: its fields are never read.
383+
#[allow(dead_code)]
384+
#[derive(EncryptTarget)]
385+
#[stash(struct = Trio, context = "trios")]
386+
struct EncryptedTrio {
387+
first: FlaggedCiphertext,
388+
#[stash(passthrough)]
389+
handle: u64,
390+
third: FlaggedCiphertext,
391+
fourth: FlaggedCiphertext,
392+
}
393+
394+
#[tokio::test]
395+
async fn a_panicking_field_declaration_wipes_the_sealed_fields_after_it() {
396+
let cipher = common::stack_cipher().await;
397+
let keyset = cipher.default_keyset();
398+
// The outer `split` (the first field panics, the rest wait as one part)
399+
// and a nested one (the third panics, the fourth waits on its own).
400+
for panicking in ["first", "third"] {
401+
let value = |field: &'static str| if field == panicking { "panics" } else { field };
402+
let (first, first_wiped) = flagged(value("first"));
403+
let (third, third_wiped) = flagged(value("third"));
404+
let (fourth, fourth_wiped) = flagged(value("fourth"));
405+
let trio = Trio {
406+
first,
407+
handle: 7,
408+
third,
409+
fourth,
410+
};
411+
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
412+
drop(trio.encrypt_into::<EncryptedTrio, _>(&keyset));
413+
}));
414+
assert!(
415+
result.is_err(),
416+
"the {panicking} field's declaration panicked"
417+
);
418+
assert!(
419+
first_wiped.load(Ordering::SeqCst) && third_wiped.load(Ordering::SeqCst),
420+
"the fields handed over are wiped by their own guards ({panicking} panicked)"
421+
);
422+
assert!(
423+
fourth_wiped.load(Ordering::SeqCst),
424+
"the sealed fields not yet handed over are wiped during the unwind ({panicking} panicked)"
425+
);
426+
}
427+
}
428+
374429
/// Seals a `Flagged` through a declaration whose construction panics: what
375430
/// `encrypt_into` is holding then is the plaintext alone.
376431
struct UnconstructedCiphertext(#[allow(dead_code)] Ciphertext);

0 commit comments

Comments
 (0)