Skip to content

Add boxed Montgomery zeroizing support#708

Merged
tarcieri merged 1 commit into
RustCrypto:masterfrom
AaronFeickert:zeroize-boxed-montgomery
Dec 8, 2024
Merged

Add boxed Montgomery zeroizing support#708
tarcieri merged 1 commit into
RustCrypto:masterfrom
AaronFeickert:zeroize-boxed-montgomery

Conversation

@AaronFeickert

@AaronFeickert AaronFeickert commented Dec 3, 2024

Copy link
Copy Markdown
Contributor

As a follow-up to #706, this PR (mostly) adds boxed Montgomery Zeroize support. Specifically, it zeroizes the value for BoxedMontgomeryForm, but leaves the parameters alone since they're wrapped in an Arc. This caveat is carefully documented.

@tarcieri

tarcieri commented Dec 3, 2024

Copy link
Copy Markdown
Member

This seems like a potential footgun in that if someone does zeroize Arc<BoxedMontyParams> it would impact other BoxedMontyForm values

@AaronFeickert

Copy link
Copy Markdown
Contributor Author

This seems like a potential footgun in that if someone does zeroize Arc<BoxedMontyParams> it would impact other BoxedMontyForm values

This is a good point, but in theory applies to any use of a zeroizable type in a reference-counted wrapper, no?

@tarcieri

tarcieri commented Dec 3, 2024

Copy link
Copy Markdown
Member

Sure, but in this case the footgun seems pretty easy to stumble upon accidentally.

Are there actual practical use cases for heap-allocated secret moduli this change is motivated by?

@AaronFeickert

Copy link
Copy Markdown
Contributor Author

It could be relevant for use cases like the Paillier-related case referenced by @fjarri. While that case was specific to MontyParams, it seemed prudent to attempt to unify the behavior as feasible.

That being said, if the footgun isn't worth it, sticking to at least zeroizing the value of MontyForm seems beneficial.

@fjarri

fjarri commented Dec 3, 2024

Copy link
Copy Markdown
Contributor

Are there actual practical use cases for heap-allocated secret moduli

I imagine they're the same cases as for the stack-allocated ones, since the boxed and the stack form are supposed to be interchangeable.

In my case, I need to do some operations in the RNS with moduli p and q which are secret primes (namely, square root and fast sampling of non-quadratic residues), so I need to make sure the corresponding Montgomery parameters don't leak secret stuff.

Edit: I suppose all of it could be done entirely with the modulus p * q, but it would be significantly slower.

@tarcieri

tarcieri commented Dec 3, 2024

Copy link
Copy Markdown
Member

Note: I'm potentially okay with adding it, but...

since the boxed and the stack form are supposed to be interchangeable.

I'm not sure how they can be in this case, namely they'll need different zeroization patterns:

  • Stack allocated types need to be zeroized in-place to be effective, since trying to move the params out makes a copy
  • Heap-allocated types rely on obtaining an exclusive reference to the params first, then zeroizing it, which involves dropping all of the BoxedMontyForm values that reference it

I don't think there's going to be a way for generic code to be able to leverage it.

@fjarri

fjarri commented Dec 3, 2024

Copy link
Copy Markdown
Contributor

I'm not sure how they can be in this case, namely they'll need different zeroization patterns

They only need different zeroization patterns because of an optimization that is invisible to the user. From the outside perspective, there is no expectation of them being zeroized differently.

That said, I don't know exactly how to resolve this while keeping the Arc. In #704 I proposed an option of including a "zeroized on drop" flag in the params, but that would involve making another set of constructors.

@tarcieri

tarcieri commented Dec 4, 2024

Copy link
Copy Markdown
Member

That said, I don't know exactly how to resolve this while keeping the Arc. In #704 I proposed an option of including a "zeroized on drop" flag in the params, but that would involve making another set of constructors.

A flag to zeroize the params on drop might make sense for BoxedMontyParams due to the Arc. I'm not sure what else would even work, other than hanging on to a copy of the Arc and trying to drop everything else that references it, with a potential runtime error if you can't get exclusive access.

If you're looking for a way to make them consistent, I'd worry adding drop glue to MontyParams might impact performance, as right now they're constantly copied and dropped.

@AaronFeickert

Copy link
Copy Markdown
Contributor Author

For now, would it make the most sense to remove Zeroize from BoxedMontyParams and just merge the (partial) addition of it in BoxedMontyForm to handle only the value?

@AaronFeickert AaronFeickert force-pushed the zeroize-boxed-montgomery branch from 37a7724 to 6b45929 Compare December 8, 2024 00:43
Comment thread src/modular/boxed_monty_form.rs Outdated
@AaronFeickert AaronFeickert force-pushed the zeroize-boxed-montgomery branch from 6b45929 to 2155f6b Compare December 8, 2024 01:12
@tarcieri tarcieri merged commit 1310938 into RustCrypto:master Dec 8, 2024
@AaronFeickert AaronFeickert deleted the zeroize-boxed-montgomery branch December 8, 2024 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants