Skip to content

DNM: force jitter#25929

Closed
xnox wants to merge 1 commit into
openssl:openssl-3.4from
xnox:force-jitter
Closed

DNM: force jitter#25929
xnox wants to merge 1 commit into
openssl:openssl-3.4from
xnox:force-jitter

Conversation

@xnox
Copy link
Copy Markdown
Contributor

@xnox xnox commented Nov 11, 2024

  • Force use jitter entropy in the FIPS 3.0.9 provider callback
    FIPS 3.0.9 provider does not honor runtime seed configuration, thus if
    one desires to use JITTER entropy source with FIPS 3.0.9 provider
    something like this needs to be applied to the core (libcrypto) build.

    Not sure if this is at all suitable for upstream.

This is backport of #25930 to codebase pre fips-jitter config option

@xnox xnox changed the base branch from master to openssl-3.4 November 11, 2024 14:44
@xnox xnox marked this pull request as ready for review November 11, 2024 14:44
@xnox xnox marked this pull request as draft November 11, 2024 14:45
Copy link
Copy Markdown
Contributor

@Sashan Sashan left a comment

Choose a reason for hiding this comment

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

Thank you for PR and detailed analysis. To be honest I think @paulidale can provide better insight here. I could spot just few nits. But change seems to make sense to me.

OSSL_PARAM params[1] = { OSSL_PARAM_END };
PROV_JITTER *s = jitter_new(NULL, NULL, NULL);
if (s == NULL)
return ret;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for PR. I wonder if should raise some error like we do in similar situation in ossl_rand_get_entropy().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True.

@Sashan Sashan requested a review from paulidale November 11, 2024 17:09
FIPS 3.0.9 provider does not honor runtime seed configuration, thus if
one desires to use JITTER entropy source with FIPS 3.0.9 provider
something like this needs to be applied to the core (libcrypto) build.

Not sure if this is at all suitable for upstream.
@paulidale
Copy link
Copy Markdown
Contributor

This is an improvement, but we don't back port features which this is.

OTC: back port or not?

@paulidale paulidale added the hold: discussion The community needs to establish a consensus how to move forward with the issue or PR label Nov 11, 2024
@Sashan
Copy link
Copy Markdown
Contributor

Sashan commented Nov 13, 2024

I gave a try this pull request for 3.4 branch. It looks like some work is needed here, because the relevant changes delivered here are not being compiled. The compile time option which enables jitrer entroy is controlled by enable-fips-jitter compile time option. The option is adhered in master branch only, it is not recognized as compile time flag in 3.4 branch. So the code which delivers desired functionality is dead. see my test diff here:

diff --git a/crypto/provider_core.c b/crypto/provider_core.c
index e5e40d5e82..f4fc4104e8 100644
--- a/crypto/provider_core.c
+++ b/crypto/provider_core.c
@@ -2141,6 +2141,7 @@ static size_t rand_get_entropy(const OSSL_CORE_HANDLE *handle,
                                unsigned char **pout, int entropy,
                                size_t min_len, size_t max_len)
 {
+#error "We have some jitter"
     return ossl_rand_jitter_get_seed(pout, entropy, min_len, max_len);
 }
 # endif

compilation of 3.4 branch just succeeds with change above in.

Comment thread crypto/provider_core.c
}

# ifdef OPENSSL_NO_JITTER
static size_t rand_get_entropy(const OSSL_CORE_HANDLE *handle,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Sashan the conditional in this PR is OPENSSL_NO_JITTER

The conditional in the #25930 PR targetting master/3.5 is OPENSSL_NO_FIPS_JITTER

Are you reviewing matching code with matching PRs as they are indeed different and use different conditionals. Depending on the version of the core being compiled and the configure settings of the core.

@xnox
Copy link
Copy Markdown
Contributor Author

xnox commented Nov 13, 2024

I gave a try this pull request for 3.4 branch. It looks like some work is needed here, because the relevant changes delivered here are not being compiled. The compile time option which enables jitrer entroy is controlled by enable-fips-jitter compile time option. The option is adhered in master branch only, it is not recognized as compile time flag in 3.4 branch. So the code which delivers desired functionality is dead. see my test diff here:

diff --git a/crypto/provider_core.c b/crypto/provider_core.c
index e5e40d5e82..f4fc4104e8 100644
--- a/crypto/provider_core.c
+++ b/crypto/provider_core.c
@@ -2141,6 +2141,7 @@ static size_t rand_get_entropy(const OSSL_CORE_HANDLE *handle,
                                unsigned char **pout, int entropy,
                                size_t min_len, size_t max_len)
 {
+#error "We have some jitter"
     return ossl_rand_jitter_get_seed(pout, entropy, min_len, max_len);
 }
 # endif

compilation of 3.4 branch just succeeds with change above in.

Please see https://github.com/openssl/openssl/pull/25929/files#r1840610093

The exact test cases I use to test this functionality is gdb batch scripts that break on syscall_random function and jitter ones. Like so

  pipeline:
    - name: Verify only jitter entropy source is in use
      runs: |
        # Possibly python gdb would be easier to read
        cat <<EOF >openssl.gdb
        set pagination off
        set logging file gdb.log
        set logging on
        set width 0
        set height 0
        set verbose off
        set breakpoint pending on
        break get_jitter_random_value
        commands 1
          continue
        end
        break syscall_random
        commands 2
          continue
        end
        run rand -hex 8
        run genrsa -out /dev/null
        run ecparam -name prime256v1 -genkey
        run genpkey -algorithm ed25519
        EOF
        gdb --batch --command ./openssl.gdb openssl
        # Assert that jitter entropy is used
        grep -q 'Breakpoint 1,' gdb.log || exit 1
        # Assert that getrandom syscall wrapper is not used
        grep -q 'Breakpoint 2,' gdb.log && exit 1

@levitte
Copy link
Copy Markdown
Member

levitte commented Nov 26, 2024

OTC: no backport

@levitte levitte removed the hold: discussion The community needs to establish a consensus how to move forward with the issue or PR label Nov 26, 2024
@t8m t8m added triaged: feature The issue/pr requests/adds a feature branch: 3.4 Applies to openssl-3.4 labels Nov 26, 2024
@t8m t8m closed this Nov 28, 2024
@xnox xnox changed the title force jitter DNM: force jitter Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment