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

Implementation of fork generation number API #3191

Merged
merged 54 commits into from
Mar 17, 2022

Conversation

torben-hansen
Copy link
Contributor

@torben-hansen torben-hansen commented Feb 3, 2022

Resolved issues:

See CryptoAlg-892 and #3107

Description of changes:

This PR is the first out of two PRs that will implement the solution outlined in #3107:

  • Change 1: implement fork generation number API <-- this PR
  • Change 2: Integrate fgn into s2n randomness layer, replacing the current fork detection code.

This change implements a fork generation number (fgn). A fgn has the following properties:

  1. Unsigned 64-bit integer.
  2. Strictly-monotonic increasing.
  3. If returned in a process, a subsequent return in a forked child process will result in a strictly greater value.

To manage and expose this number to higher layers (and hide the implementation details to the rest of the code base) the fgn API s2n_get_fork_generation_number(*generation_number) is implemented. This function has the following properties:

  1. Returns the fork generation number, in the current process, in *generation_number.
  2. If called in an address space and then again in a subsequently forked child process, the forked address space will observe a greater value.
  3. Returns S2N_SUCCESS if fork detection is supported, S2N_FAILURE otherwise.

To implement the second property, two independent fork detection mechanisms are re-implemented:

  • pthread_atfork
  • MADV_WIPEONFORK

The implementation of these follow the current implementation but fixes its issues. There is one fundamental difference compared to the existing implementation: the former method is required to be functional while the latter method is initialised with best-effort (on supported systems...).

Call-outs:

The reason for having MADV_WIPEONFORK best-effort is documented in the code, but is repeated here for completeness:

Some system don't define MADV_WIPEONFORK in sys/mman.h but the kernel still supports the mechanism (AL2 being a prime example). Likely because glibc on the system is old. We might be able to include kernel header files directly, that define MADV_WIPEONFORK, conditioning on specific OS's. But it is a mess. A more reliable method is to probe the system, at run-time, whether madvise supports the MADV_WIPEONFORK advice. However, the method to probe for this feature is equivalent to actually attempting to initialise the MADV_WIPEONFORK fork detection. Compare FOR_TESTING_probe_madv_wipeonfork_support() (used for testing) with initialise_wipeonfork_or_inherit_zero(). Instead, we apply best-effort to initialise the MADV_WIPEONFORK fork detection and otherwise always require pthread_atfork to be initialised. We also currently always apply prediction resistance. So, this should be a safe default.

Given this, it might be useful, in the future, to add a public API that consuming application can use to assert whether MADV_WIPEONFORK has been enabled.

In theory the state variable current_fork_generation_number can overflow. But it has ~2^64 possible values, which should be plenty to outlive even wildest long-lived process and forking.

Testing:

A new test suite has been written containing 4 different type of tests running over a combination of active fork detection methods. The 4 different types of tests are:

  • Basic unit tests (e.g. calling the function returns succesfully).
  • Verifying idempotence of s2n_get_fork_generation_number() after lazy initialisation (and no fork events, or after a fork event). I also call this "stability".
  • Verifying property two of s2n_get_fork_generation_number() under a fork().
  • Verifying property two of s2n_get_fork_generation_number() under a clone() (only if MADV_WIPEONFORK or MIN_INHERIT_ZERO is expected to be supported).

Darwin/OSX

Verified locally:

uname -rsv
Darwin 19.6.0 Darwin Kernel Version 19.6.0: Sun Nov 14 19:58:51 PST 2021; root:xnu-6153.141.50~1/RELEASE_X86_64

./build/bin/s2n_fork_generation_number_test
Running ../tests/unit/s2n_fork_generation_number_test.c    ... 
FGN test case: Only madv_wipeonfork fork detection mechanism.
Test case not supported. Skipping.

FGN test case: Only map_inheret_zero fork detection mechanism.
Test case not supported. Skipping.
PASSED         16 tests

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dougch
Copy link
Contributor

dougch commented Feb 3, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 3, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 3, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 3, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 3, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 3, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 3, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 3, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 4, 2022

Benchmark report
No change in performance detected.

@torben-hansen torben-hansen marked this pull request as draft February 4, 2022 06:26
@dougch
Copy link
Contributor

dougch commented Feb 4, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 4, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 4, 2022

Benchmark report
No change in performance detected.

@dougch
Copy link
Contributor

dougch commented Feb 4, 2022

Benchmark report
No change in performance detected.

@torben-hansen torben-hansen marked this pull request as ready for review February 4, 2022 20:13
Copy link
Contributor

@camshaft camshaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will review more; just a couple of things that stood out initially.

utils/s2n_safety_macros.h Outdated Show resolved Hide resolved
utils/s2n_fork_detection.h Outdated Show resolved Hide resolved
utils/s2n_fork_detection.c Outdated Show resolved Hide resolved
@dougch
Copy link
Contributor

dougch commented Feb 12, 2022

Benchmark report
No change in performance detected.

Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not done reviewing the full change, but here's some initial comments mostly about the codebase conventions.

crypto/s2n_drbg.c Outdated Show resolved Hide resolved
error/s2n_errno.c Outdated Show resolved Hide resolved
utils/s2n_fork_detection.h Outdated Show resolved Hide resolved
utils/s2n_fork_detection.c Outdated Show resolved Hide resolved
utils/s2n_fork_detection.c Outdated Show resolved Hide resolved
utils/s2n_fork_detection.c Outdated Show resolved Hide resolved
utils/s2n_fork_detection.c Outdated Show resolved Hide resolved
utils/s2n_fork_detection.c Outdated Show resolved Hide resolved
utils/s2n_fork_detection.c Outdated Show resolved Hide resolved
@torben-hansen torben-hansen enabled auto-merge (squash) March 17, 2022 18:21
@torben-hansen torben-hansen merged commit 31d99d2 into aws:main Mar 17, 2022
@torben-hansen torben-hansen deleted the impl_fgn branch March 17, 2022 20:08
dougch pushed a commit to dougch/s2n-tls that referenced this pull request Mar 17, 2022
This change implements a fork generation number (fgn). A fgn has the following properties:

* Unsigned 64-bit integer.
* Strictly-monotonic increasing.
* If returned in a process, a subsequent return in a forked child process will result in a strictly greater value.
dougch pushed a commit to dougch/s2n-tls that referenced this pull request Mar 18, 2022
This change implements a fork generation number (fgn). A fgn has the following properties:

* Unsigned 64-bit integer.
* Strictly-monotonic increasing.
* If returned in a process, a subsequent return in a forked child process will result in a strictly greater value.
torben-hansen added a commit that referenced this pull request Apr 6, 2022
Turns out that osx/apple defines both madvise() and minherit(). This invalidates an assumption I made in #3191.

To remedy, remove the bad assumption and fix another osx/apple-specific thing: __WALL is not defined.
torben-hansen added a commit that referenced this pull request Jun 14, 2022
This change takes the fork detection backend implemented in #3191 and wires up the randomness generation frontend to it. In turn, replacing the existing fork detection mechanism.

In addition, it implement two test cases that verifies more fork detection methods in isolation.
dougch pushed a commit to dougch/s2n-tls that referenced this pull request Jun 27, 2022
This change takes the fork detection backend implemented in aws#3191 and wires up the randomness generation frontend to it. In turn, replacing the existing fork detection mechanism.

In addition, it implement two test cases that verifies more fork detection methods in isolation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants