Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit and replace memcmp usage with s2n_constant_time_equals #3062

Closed
toidiu opened this issue Sep 21, 2021 · 1 comment
Closed

Audit and replace memcmp usage with s2n_constant_time_equals #3062

toidiu opened this issue Sep 21, 2021 · 1 comment

Comments

@toidiu
Copy link
Contributor

toidiu commented Sep 21, 2021

Security issue notifications

If you discover a potential security issue in s2n we ask that you notify
AWS Security via our vulnerability reporting page. Please do not create a public github issue.

Problem:

s2n codes should use s2n_constant_time_equals instead of memcmp.

Solution:

A description of the possible solution in terms of S2N architecture. Highlight and explain any potentially controversial design decisions taken.

  • Does this change what S2N sends over the wire? If yes, explain.
  • Does this change any public APIs? If yes, explain.
  • Which versions of TLS will this impact?

Requirements / Acceptance Criteria:

What must a solution address in order to solve the problem? How do we know the solution is complete?

  • RFC links: Links to relevant RFC(s)
  • Related Issues: Update PQ KEM branches to use constant time functions. #2590
  • Will the Usage Guide or other documentation need to be updated?
  • Testing: How will this change be tested? Call out new integration tests, functional tests, or particularly interesting/important unit tests.
    • Will this change trigger SAW changes? Changes to the state machine, the s2n_handshake_io code that controls state transitions, the DRBG, or the corking/uncorking logic could trigger SAW failures.
    • Should this change be fuzz tested? Will it handle untrusted input? Create a separate issue to track the fuzzing work.

Out of scope:

Is there anything the solution will intentionally NOT address?

@boquan-fang
Copy link
Contributor

#4709 has addressed this issue. There are three remaining memcmp s in the codebase. These were not removed because

  • They were used for comparison (not just equal or not equal).
  • Performance concerns about using s2n_constant_time_equals for large sizes of data.

For further discussion about these tradeoffs, see the linked PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants