Constant-time: Make signature declassifications explicit in verification #822
Conversation
mkannwischer
force-pushed
the
verify-cleaner-declassification
branch
from
19032f8 to
b863e4d
Compare
mldsa/src/sign.c
Outdated
| /* Constant-time: The signature is commonly considered public. However, we | ||
| * mark it as secret to make explicit which parts of the code take time | ||
| * depending on the signature. mld_unpack_sig uses conditional branches to | ||
| * unpack the hint vector h. */ | ||
| MLD_CT_TESTING_DECLASSIFY( | ||
| sig + MLDSA_CTILDEBYTES + MLDSA_L * MLDSA_POLYZ_PACKEDBYTES, | ||
| MLDSA_POLYVECH_PACKEDBYTES); |
There was a problem hiding this comment.
I'm not sure a public API should change the classification over user-provided data -- this can break or unexpectedly alter CT tests in integrations. Can we instead document that verify is not CT for the (relevant parts of) the signature, and leave all classification to the tests?
There was a problem hiding this comment.
The downside of your proposal is that it does not make it apparent which operations leak.
With the current approach, it is very clear which function leaks what (and it is checked in CI, so you cannot introduce additional leaks).
There was a problem hiding this comment.
But, yes, we can move the declassification of the h-component to the tests insetead.
I do want to keep testing with the remainder of the signature marked as secret though.
hanno-becker
left a comment
hanno-becker
left a comment
There was a problem hiding this comment.
Toplevel Q first before checking the details: I don't think the public API should change the classification of user-provided buffers, or am I overlooking something?
Previously, the signature would be declassified in the end of signing (which is the most common threat model). However, for rare use cases the signature may be secret in which case it is useful to know which part of the code has to be adapted. This commit removes the declassification from the end of signing, explicitly marks the signature as secret in the constant time tests (to also classify the previously declassified challenge c), and then adds necessary declassifications in verifiation. We may consider eliminating some of those declassifications later. In particular, the data dependencies in use_hint are not hard to eliminate (in fact, the native implementations are already constant time). The final comparison of the challenges can also easily be turned into a constant-time comparison. Signed-off-by: Matthias J. Kannwischer <matthias@kannwischer.eu>
mkannwischer
force-pushed
the
verify-cleaner-declassification
branch
from
b863e4d to
f72d82f
Compare
I have pushed an alternative that moves the declassification of h to the test harness. Not sure if this is necessarily easier to follow. |
| /* Constant-time: The signature is commonly considered public. However, we | ||
| * mark it as secret to make explicit which parts of the code take time | ||
| * depending on the signature. |
There was a problem hiding this comment.
This comment seems outdated?
| * mld_unpack_sig uses conditional branches to unpack the hint vector h. | ||
| * The h-component of the signature, hence, needs to be declassified. |
There was a problem hiding this comment.
| * mld_unpack_sig uses conditional branches to unpack the hint vector h. | |
| * The h-component of the signature, hence, needs to be declassified. | |
| * mld_unpack_sig uses conditional branches to unpack the hint vector h. | |
| * It is the caller's responsibility to declassify the h-component of the signature. |
| /* Constant-time: poly_challenge uses conditional branches depending on c. */ | ||
| MLD_CT_TESTING_DECLASSIFY(c, MLDSA_CTILDEBYTES); |
There was a problem hiding this comment.
In the current version, the implementation is inconsistent regarding what is declassified by the function vs. expected to be declassified by the call-site: The h-component is expected to be declassified by the call-site, but the c-component is declassified here.
There was a problem hiding this comment.
We're still lacking a consistent story for how (de)classification is handled.
We rely on the caller of sign to classify the signature. When we verify, we expect the caller to again have declassified some parts of it, while others are declassified ad-hoc.
Zooming out a little, I think we should strive for the following:
- For every
constinput to a top-level API, the classification status of the bytes is not changed by the implementation. This is natural if you viewconstas extending to the(value, secret_status)presentation of data in the CT model. It also helps to avoid surprises when integrating with CT tests of consuming libraries. - For every output to a top-level API, the API should say explicitly what its classification status will be.
At the moment, for example, keygen outputs a private key buffer parts of which are classified, and parts of which are declassified. This is not documented at the API-level, and it's not even obvious from the implementation. We should either say which parts are (de)classified and enforce it at the end of signing, or simply say that all of the private key buffer is secret, enforce it in the implementation, and leave it to the caller to alter that if they wish to.
For signing, it's similar: We should say explicitly whether the signature is classified as secret or public, enforce it in the implementation, and adjust the call-sites (for us, the tests) accordingly.
Note that extending const to the security-state does not imply that the implementation cannot declassify derived data from secret const inputs. Here, I'm not yet sure what to do. We can either a) declassify the relevant derived data, or b) where possible, expect the caller to have declassified the input data accordingly, and document that assumption. The latter is not always an option: For example, signing consumes the secret sk buffer, but safely declassifies derived data encountered during signature generation. For, for verification, we could document that the relevant parts of the signature must already have been declassified, and not do any ad-hoc declassification in the implementation.
Alternative to #822 that I hope to be less controversial. Currently the constant time tests for verification rely on the signature being declassified at the end of verification. This is not ideal. This commit moves this declassification to the constant-time test instead. As suggested in #822 (review), there is more work left to clean up the story around declassifications. This PR is a first step towards cleaning up that story to unblock #825 and #821, but there is more work left. Signed-off-by: Matthias J. Kannwischer <matthias@kannwischer.eu>
|
Thanks @hanno-becker for the review. |
Alternative to #822 that I hope to be less controversial. Currently the constant time tests for verification rely on the signature being declassified at the end of verification. This is not ideal. This commit moves this declassification to the constant-time test instead. As suggested in #822 (review), there is more work left to clean up the story around declassifications. This PR is a first step towards cleaning up that story to unblock #825 and #821, but there is more work left. Signed-off-by: Matthias J. Kannwischer <matthias@kannwischer.eu>
Previously, the signature would be declassified in the end of signing (which is the most common threat model).
However, for rare use cases the signature may be secret in which case it is useful to know which part of the code has to be adapted.
This commit removes the declassification from the end of signing, explicitly marks the signature as secret in the constant time tests (to also classify the previously declassified challenge c), and then adds necessary declassifications in verifiation.
We may consider eliminating some of those declassifications later. In particular, the data dependencies in use_hint are not hard to eliminate (in fact, the native implementations are already constant time). The final comparison of the challenges can also easily be turned into a constant-time comparison.