ring icon indicating copy to clipboard operation
ring copied to clipboard

Refactor fe_isnonzero to fe_isnonzero_vartime

Open tisonkun opened this issue 2 years ago • 8 comments

This refers to #1827.

An intuitive solution according to @briansmith's comment. But I know little about crypto so perhaps some invariants are broken.

tisonkun avatar Jan 11 '24 15:01 tisonkun

Codecov Report

Attention: Patch coverage is 88.88889% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 96.30%. Comparing base (9cb93e0) to head (407599a). Report is 31 commits behind head on main.

Files Patch % Lines
crypto/curve25519/curve25519.c 88.88% 0 Missing and 1 partial :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1893   +/-   ##
=======================================
  Coverage   96.29%   96.30%           
=======================================
  Files         135      135           
  Lines       20663    20667    +4     
  Branches      226      228    +2     
=======================================
+ Hits        19898    19903    +5     
  Misses        730      730           
+ Partials       35       34    -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 13 '24 01:01 codecov[bot]

Thanks for your review @briansmith!

Do you think it's better to exclude changes around src/constant_time.rs in this PR and move forward, or we can wait for https://github.com/briansmith/ring/pull/1899 merged?

tisonkun avatar Jan 13 '24 02:01 tisonkun

Yes, I think we should limit this to the curve25519.c change and mem.h deletion.

briansmith avatar Jan 13 '24 02:01 briansmith

Updated. PTAL @briansmith.

tisonkun avatar Jan 13 '24 10:01 tisonkun

@briansmith ping as a reminder :D

tisonkun avatar Jan 22 '24 17:01 tisonkun

@briansmith ping as a reminder :D

tisonkun avatar Mar 09 '24 03:03 tisonkun

@briansmith I suppose the codecov regression is false positive..

tisonkun avatar Apr 09 '24 04:04 tisonkun

@briansmith is there other blocker to this PR?

tisonkun avatar Jul 06 '24 22:07 tisonkun