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

Make s2n_init idempotent or return error for multi-s2n_init invocations #3446

Closed
franklee26 opened this issue Aug 12, 2022 · 2 comments · Fixed by #3512
Closed

Make s2n_init idempotent or return error for multi-s2n_init invocations #3446

franklee26 opened this issue Aug 12, 2022 · 2 comments · Fixed by #3512

Comments

@franklee26
Copy link
Contributor

Problem:

Customer is facing issues due to a double-call to s2n_init due to dependencies re-calling s2n_init. Directly from the customer:

Specifically, our application depends on multiple libraries that all individually depend on S2N. They all have a need to call s2n_init and expect it to succeed, which is critical to the functionality of their library. However, as currently implemented, s2n_init is not idempotent, nor it returns an error code that indicates it has already been initialized. As result, only the first calling library works, and others fail. It makes it hard for application like ours that directly/indirectly depend on S2N via multiple dependency paths.

Solution:

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: Link any relevant issues
  • 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?

@graebm
Copy link
Contributor

graebm commented Aug 18, 2022

It is NOT a good idea to make it idempotent (do nothing if already initialized). Imagine this scenario:

  • Application starts up and calls s2n_init()
  • later on, application dynamically loads some Library, which also calls s2n_init()
  • later on, application unloads that dynamic Library, which calls s2n_cleanup()
  • Application continues trying to use S2N, but S2N is no longer initialized, so everything is broken

It would be better to go the route of returning some ALREADY_INITIALIZED error, then the Library could know that it shouldn't call s2n_cleanup()

@graebm
Copy link
Contributor

graebm commented Aug 18, 2022

Another idea: Do reference counting with s2n_init() and s2n_cleanup() calls.

Very similar to being idempotent (the only thing that changes on a 2nd call to s2n_init() is the reference count).
But if s2n_init() is called 3 times by an application, then s2n_cleanup() won't ACTUALLY clean up until the reference count hits 0.

jeking3 added a commit to jeking3/s2n-tls that referenced this issue Sep 26, 2022
jeking3 added a commit to jeking3/s2n-tls that referenced this issue Sep 26, 2022
jeking3 added a commit to jeking3/s2n-tls that referenced this issue Sep 28, 2022
jeking3 added a commit to jeking3/s2n-tls that referenced this issue Sep 28, 2022
jeking3 added a commit to jeking3/s2n-tls that referenced this issue Sep 28, 2022
jeking3 added a commit to jeking3/s2n-tls that referenced this issue Sep 30, 2022
jeking3 added a commit to jeking3/s2n-tls that referenced this issue Oct 1, 2022
jeking3 added a commit to jeking3/s2n-tls that referenced this issue Oct 3, 2022
camshaft pushed a commit that referenced this issue Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants