Enable the --ct-verif switch to toggle the CT_VERIF define in
bltest and fbectest. Add Valgrind instrumentation for
constant-timeness checks to bltest and fbectest.
Details
Details
- Reviewers
mt ckerschb - Group Reviewers
nss-reviewers - Bugzilla Bug ID
- 1741849
Diff Detail
Diff Detail
- Repository
- rNSS nss
- Branch
- default
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Just a quick note; I didn't spend the time necessary to determine that the coverage here is adequate. That's probably the hardest part of reviewing any change of this sort.
| cmd/bltest/blapitest.c | ||
|---|---|---|
| 28–30 | I think that it would be a good idea to define a new macro (CT_POISON(b, l) say) here. When CT_VERIF is on it can use the valgrind macro; otherwise it can be a noop. That will avoid the need for #ifdef statements throughout the code. | |
Comment Actions
I have implemented the suggested CT_POISON macro. Btw. there are already mpi_taint and mpi_untaint functions defined and implemented under CT_VERIF in mpi.c, so maybe this can get unified.
However, those functions are only used in one unittest, which is disabled by default.
W.r.t coverage, feel free to let me know where to look for more primitives to include, I am quite new to NSS.
Comment Actions
Hello! Thanks for your patch.
Could I ask you to write a small README section about what you have done in this patch? For example, in the macros.
Thanks :)
Comment Actions
We, the crypto engineering team, are in the process of improving our review and landing queue, basically patches that are tagged with 'nss-reviewers'. Part of this process is to clean out 'stalled' patches. For whatever reason this changeset has not seen any movement for a long time and we consider it 'stalled'. If you feel this is a mistake or incorrect, please follow up, e.g. by rebasing the patch and/or by resetting the tag 'nss-reviewers' and drive this patch forward!
Thank you for your contributions!