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

Add fips indicator requirements doc #23609

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

slontis
Copy link
Member

@slontis slontis commented Feb 16, 2024

Checklist
  • documentation is added or updated
  • tests are added or updated

@slontis
Copy link
Member Author

slontis commented Feb 16, 2024

@paulidale FYI

Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

Just a first round of quick comments.

doc/designs/fips_indicator.md Outdated Show resolved Hide resolved
doc/designs/fips_indicator.md Outdated Show resolved Hide resolved
doc/designs/fips_indicator.md Outdated Show resolved Hide resolved
doc/designs/fips_indicator.md Outdated Show resolved Hide resolved


Note that in order for a service to be ‘fips approved’ the following requirments would need to be met.
- Any algorithms are fetched using “fips=yes”
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO. this isn't required. They must, however, be fetched from a FIPS approved provider.

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case the ones that have fips=no need an indicator.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was my understanding that the FIPS provider (i.e. fips.so) defined the module boundary. Therefore any algorithms outside the FIPS provider would simply not be part of the FIPS module (and thus not listed in the Security Policy). Does the "fips=yes" property not enforce algorithms to be fetched through the FIPS provider?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is the definition of an implicit indicator.
I want the indicator to apply to any provider i.e. retrieving the getter on any ctx will return either yes - its FIPS approved OR no its not.

Copy link
Member

Choose a reason for hiding this comment

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

Well... there could be helper functions but if implemented as OSSL_PARAM, it should be that a param is set to true if FIPS approved/allowed, otherwise - not present or false -> not approved.

I think it would be bad to make this unclear - i.e. the param value not returned means something undefined.

doc/designs/fips_indicator.md Outdated Show resolved Hide resolved
doc/designs/fips_indicator.md Outdated Show resolved Hide resolved
#### Proposed Solution (Using an indicator everywhere)

Add a OSSL_PARAM getter to each provider algorithm.
By default if the getter is not handled then it would return not approved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Easy to detect with the helper functions.


#### Backwards Compatability..

Previous providers do not support this operation, so they will return not approved if they are not handled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe????

Copy link
Member Author

Choose a reason for hiding this comment

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

The thinking being that it wont handle the OSSL_PARAM so it is not approved.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the FIPS provider has many more algorithms which are approved rather than non-approved. Would it not be easier to have an OSSL_PARAM indicating the non-approved status? If this parameter is present and set to true, the service would be non-approved. The absence of the parameter, or (if it is present) a value of false would mean the service is approved. That way you don't have to add the parameter to every single API, but only the ones where non-approvedness is possible.

On the other hand, this becomes more difficult to explain in the Security Policy.

Copy link
Member Author

@slontis slontis Feb 21, 2024

Choose a reason for hiding this comment

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

I was thinking this applies to all providers, and in that case undefined is better to mean not approved, especially if we apply this to any provider that doesnt understand this. The set of approved algorithms will always be a small subset of all algorithms.

Copy link
Contributor

Choose a reason for hiding this comment

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

From that point of view that's correct – however keep in mind that only the FIPS provider is certified anyway. So from a certification point of view, it doesn't matter what values other providers do or don't return for this param. If you want to be FIPS compliant, you already have to make sure that you are using the FIPS provider.

Red Hat went for the "if it doesn't have an indicator and is in the FIPS provider, it's approved" default.


### Digest Checks

Any algorithms that use a digest need to make sure that the CAVP certificate lists all supported FIPS digests otherwise an indicator is required.
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently Hash and HMAC DRBGs are the only impacted algorithms.
Nonetheless, this is a concern going forwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree. The DRBG's are the only algorithms that are doing the right thing currently because you added code to check what digests are allowed. Unless the FIPS cert lists all the digests that matches all the FIPS digest algorithms then we are not currently doing it correctly for a FIPS 140-3 validation.

Copy link
Contributor

Choose a reason for hiding this comment

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

The 140-3 should list everything. I've not seen the actual submission so I don't know if this is correct or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

I find it hard to believe it was done for absolutely everything.. (including things like SSKDF, and HKDF)

Copy link
Member

Choose a reason for hiding this comment

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

It does not make sense to not test&validate things that are possible to be tested and validated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure I understand your point, look at the 3.0.8 validation for example and you will see that the digests tested vary wildly. This could be because there was not even an option to test them at that point in time, I dont know since I wasnt part of that process.

Copy link
Member Author

Choose a reason for hiding this comment

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

As @jvdsn has pointed out there are cases where subsets are only allowed (such as TLS1.3 PRF)

Copy link
Contributor

Choose a reason for hiding this comment

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

The commit message in https://gitlab.com/redhat/centos-stream/rpms/openssl/-/blob/c9s/0078-KDF-Add-FIPS-indicators.patch?ref_type=heads has a nice overview of which combinations can be tested, and which ones we were told to consider unapproved (and thus marked with an indicator saying so).

@slontis
Copy link
Member Author

slontis commented Feb 19, 2024

Removed all the doc-nits

@paulidale
Copy link
Contributor

Do we need a version has part of the indicator? I.e. to distinguish between 140-3 and the eventual 140-4.
This should be planned for now to avoid problems later. 140-2 doesn't need indicators, so we don't such differentiation yet but it's certain we will eventually.

Various other countries have their own standards, we should support some kind of indicator for them too. Or at least we should plan to do so now rather than later.

@slontis
Copy link
Member Author

slontis commented Feb 20, 2024

For each submission something is either approved or NOT. This should just be different testing requirements based on the provider version. So I dont see the need for this.
If we are using OSSL_PARAM getters then add another entry.


- HMAC Which applies to all algorithms that use HMAC also.
- CMAC

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you are missing KMAC here.

KDFs also need a key size >= 112 bits per Section 8 of SP 800-131Ar2, including HKDF, SSKDF, KBKDF.

Copy link
Member Author

Choose a reason for hiding this comment

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

KMAC is only checking > 4 currently so you are correct (I was thinking it was sufficient that it uses the KECCAK_KMAC algorithm - but it is not).
HKDF, SSKDF and KBKDF fall under the category of (HMAC which applies to all algorithms that use HMAC also)

- DES_EDE3_ECB. Disallowed for encryption, allowed for legacy decryption
- DSA. Keygen and Signing are no longer approved, not sure if verify is still approved.
- RSA Signing using PKCSV15. Rsa self test for sign may need to change as the default pad_mode is PKCSV15. Check if saltlen needs a indicator. Padding mode updates required in rsa_check_padding(). Check if sha1 is allowed?
- ECDSA B & K curves are deprecated It looks like these are still allowed. Are the approved?
Copy link
Contributor

Choose a reason for hiding this comment

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

Per FIPS 140-3 IG C.K, Resolution 4:

There is currently no scheduled transition away from elliptic curves over binary fields (i.e., K-233, B-233,
K-283, B-283, K-409, B-409, K-571, B-571). However, these curves are now deprecated, and it is
strongly recommended to use the SP-800-186-defined prime curves (i.e., P-224, P-256, P-384, P-521) for
the generation of the ECDSA signatures. Despite their deprecation status, these curves are still considered
approved and therefore will be included in the CAVP FIPS 186-5 testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was only updated recently. Before the update this was not true :)
There is no requirement that they are supported. Validations such as Redhat have stripped these out ages ago, so we could chose to do this. (I imagine they will be removed at some point.)

- ED25519/ED448 is now approved.
- X25519/X448 is not approved currently. keygen would need an indicator if we allow it?
- RSA Key transport Padding need to be changed to either not allow padding modes other than OEAP or use an indicator.
- RSA - It looks like SP800-131Ar2 specifies that RSA >= 2048 is approved. Can we check with the labs what to do for the legacy verification cases since we allow >=1024. Would this now be unapproved? This would apply to rsa_kem also? rsa_keygen_pairwise_test() may need to change to do a sign/verify PCT?
Copy link
Contributor

Choose a reason for hiding this comment

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

SP 800-131Ar2 Table 2 specifies that 1024 ≤ len(n) < 2048 is allowed for "legacy use" for signature verification purposes, so that could have an approved indicator.

For RSA KAS (RSASVE) and KTS (OAEP), SP 800-131Ar2 Table 5 does not have a "legacy use" provision. There, the minimum is 2048 bits for both encryption/encapsulation and decryption/decapsulation.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, I must of misread something. The FIPS 140-3 IG C.F makes this clear, note that this also allows FIPS-186-2 verification also...

Copy link
Contributor

Choose a reason for hiding this comment

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

For FIPS 186-2 verification, strictly speaking only "1024+256*s, where s is a nonnegative integer" would be allowed. So the approved modulus sizes would go something like: 1024, 1280, 1536, 1792, and then 2048 and above. I don't like that it's so awkward, instead of just allowing all modulus sizes in 1024-2048, but that's what they did.

Any algorithms that use a digest need to make sure that the CAVP certificate lists all supported FIPS digests otherwise an indicator is required.
This applies to the following algorithms:

- SSKDF
Copy link
Contributor

@jvdsn jvdsn Feb 20, 2024

Choose a reason for hiding this comment

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

In OpenSSL 3.2.0, it seems like SSKDF supports using SHAKE128, SHAKE256, KECCAK-KMAC128, and KECCAK-KMAC256 as digests. Those are not valid auxiliary functions according to Section 4.2 of SP 800-56Cr2, so they should probably be removed (I do not know why someone would actually use them).

Copy link
Member Author

Choose a reason for hiding this comment

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

The general rule is the KECCAK ones are only for KMAC.
We will have to figure out which algorithms will allow SHAKE (i.e. even if they are allowed we can optionally chose to support them).

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now, I can't think of any KDF that allows SHAKE. Only recently has NIST been allowing SHAKE in higher-level algorithms. FIPS 140-3 IG C.C still states "The SHAKE128 and SHAKE256 extendable-output functions may only be used as the standalone algorithm", but that's obviously outdated now with SP 800-208 (LMS) and FIPS 186-5 allowing SHAKE for RSA/ECDSA/EdDSA signatures.

-------------

- AES-GCM Security Policy must list AES GCM IV generation scenarios
- TEST_RAND is not approved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now it has FIPS_UNAPPROVED_PROPERTIES in fipsprov.c, would that not be sufficient?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we are going to use indicators everywhere then it is probably better to not mix implicit and explicit indicators. Querying anything in the default provider for example should return not approved. (i.e.- keep the mechanism simple),.


- See the "security checks" Section. Anywhere using ossl_securitycheck_enabled() may need an indicator

Other Changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Some other items I can think of:

  • SP 800-56Ar3 Section 5.7.1.2 specifically defines the EC Cofactor Diffie-Hellman primitive. Therefore, it seems like OSSL_PKEY_PARAM_USE_COFACTOR_ECDH must be set to 1 for an approved EC key exchange. This doesn't impact the P curves (as their cofactor is always 1), but it does impact K and B curves.
  • FISP 186-5 Section 5.4, bullet (g) has some restrictions on the salt length for RSA-PSS signatures. Are those enforced? In the case of a 1024-bit modulus, FIPS 186-4 Section 5.5, bullet (e) has an additional restriction.
  • X9.31 padding would need to be removed or marked non-approved for signature generation, as it was removed by FIPS 186-5
  • It seems like RSA OAEP allows SHAKE128, SHAKE256, KECCAK-KMAC128, and KECCAK-KMAC256 to be used as the md. Is that well-defined?
  • KMAC128 and KMAC256 currently allow the output of very short tag lengths, less than 32 bits. According to Section 8.4.2 of SP 800-185:
When used as a MAC, applications of this Recommendation shall not select an output length L
that is less than 32 bits, and shall only select an output length less than 64 bits after a careful risk
analysis is performed

(I do not know what "a careful risk analysis" would entail for a general-purpose cryptographic library like OpenSSL)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the input...
I would be fine with K & B curves being removed altogether - but if we keep them then what you say applies.
I do mention salt length in the Algorithm Transitions - it will need to be checked. I think it needs an indicator.

  • RSA OEAP digests should be determined by what we can actually test.
    -Looks like KMAC needs an indicator on the lower bound.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to the specification (Table 12), ACVP testing for OAEP is restricted to SHA-1, SHA-2, or SHA-3. Can also be confirmed by looking at the ACVP server source code

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is sufficent to say that if we cant select as a tick box on the submission paperwork then its not supported

@slontis
Copy link
Member Author

slontis commented Feb 21, 2024

@jvdsn I really appreciate your input.
Will update sections based on your feedback.

@t8m
Copy link
Member

t8m commented Feb 21, 2024

IMO we should still strive for keeping as much as possible non-approved functionality out of the FIPS module. The explicit indicators support is to me just a bad excuse if we fail to do that. BTW if we do all these checks where the indicator is set or unset we could as well have an implicit indicator mode -> have a config option that makes all these checks that unset the approved/allowed indicator to fail the operation.

@beldmit
Copy link
Member

beldmit commented Feb 28, 2024

I have probably a side discussion regarding indicators.
If I create a key using (deprecated) the EVP_PKEY_set1_* API, after export-import dance it may or may not be FIPS-compliant (e.g. too short or using specific RSA_METHOD) key. I would like to distinguish these (valid and invalid) cases using indicators. And probably, I'd consider to un-deprecate the EVP_PKEY_set1_* API in that case.

@paulidale
Copy link
Contributor

I'm beginning to think that some explicit indicators are inevitable.

Take ECDSA, for example, where NIST has disallowed passing a hash. Rather the digest has to be computed inside the sign/verify call. This is nuts and we ought to allow either by default with the former setting the not FIPS flag. I.e. an explicit indicator.

Nonetheless, most of the new restrictions are reasonable to enforce by default and have a way for the user to disable the enforcement. I.e. implicit indicator with an option for explicit.

@jvdsn
Copy link
Contributor

jvdsn commented May 1, 2024

Take ECDSA, for example, where NIST has disallowed passing a hash. Rather the digest has to be computed inside the sign/verify call. This is nuts and we ought to allow either by default with the former setting the not FIPS flag. I.e. an explicit indicator.

In that specific case, it should be sufficient to disallow the EVP_PKEY_sign/verify APIs, no? I believe the EVP_DigestSign/Verify APIs compute the hash internally

@paulidale
Copy link
Contributor

paulidale commented May 1, 2024

In that specific case, it should be sufficient to disallow the EVP_PKEY_sign/verify APIs, no? I believe the EVP_DigestSign/Verify APIs compute the hash internally

We could disallow these but such usage is very common and we would break a lot of people.
Making this use an explicit indicator would meet the NIST requirements without breaking anybody.

Thus the suggestion is that we should support both:

  • blocked by default but can be enabled in various ways with an indicator saying it's no longer FIPS approved; and
  • allowed by default with an explicit indicator saying it's naughty.

Rarely is FIPS 140 black and white and many of the rules are confusing, arbitrary, silly and contradictory. Plus agencies always have the option to not follow the strict rules with sufficient documentation (risk analyses и так далее).

@slontis
Copy link
Member Author

slontis commented May 2, 2024

Just to clarify it was the ECDSA sig verify case only....

@paulidale paulidale removed the approval: otc review pending This pull request needs review by an OTC member label May 14, 2024
@paulidale
Copy link
Contributor

Ping @openssl/otc this has been stalled for way too long and needs to be progressed...
Ping @openssl/committers as well

Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Overall this looks ok to me. Just some comments on some details.

doc/designs/fips_indicator.md Outdated Show resolved Hide resolved
doc/designs/fips_indicator.md Show resolved Hide resolved
doc/designs/fips_indicator.md Outdated Show resolved Hide resolved
doc/designs/fips_indicator.md Show resolved Hide resolved
Copy link
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.

the callback function needs to receive cbarg

```c
int OSSL_INDICATOR_callback(OSSL_LIB_CTX *libctx, const char *algtype,
const char *algdesc)
```
Copy link
Contributor

Choose a reason for hiding this comment

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

do I understand right the algtype string also has a reference to key size? I'm thinking of RSA with keysize smaller than 2048. May be I would pass a pointer to structure with algtype and algdesc members. we can add bits member where needed/makes sense.

Also cbarg seems to be missing in callback arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that this is just a very generic system for logging strings, not for passing complex context specific types back..
For an example see dsa_sig.c dsa_init_check_approved()
https://github.com/openssl/openssl/pull/23890/files#diff-0c2b4e0a207614d420d8f376c476115e33d5ad7733790a40b1c238a2a79e40c1

In this case the callback may be triggered twice..
Once because dsa signing is not allowed & a second time if the security check fails
Once with "DSA", "Sign Init"
and the second time with "DSA", "Key Size".

Copy link
Member Author

Choose a reason for hiding this comment

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

The cbarg is set via OSSL_INDICATOR_set_callback

Copy link
Member Author

Choose a reason for hiding this comment

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

OSSL_INDICATOR_callback() is actually internal.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated the doc a bit

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for a link to draft-pr looking at the code all is clear now (at least around callback).

@slontis
Copy link
Member Author

slontis commented May 15, 2024

updated doc based on feedback.
Also added a section called 'Issues with setting OSSL_ALG_PARAM_STRICT_CHECKS'

```c
int strict = 0;
params[0] = OSSL_PARAM_construct_int(OSSL_ALG_PARAM_STRICT_CHECKS, strict);
EVP_DigestSignInit_ex(ctx, &pctx, name, libctx, NULL, pkey, params);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest: params[1] = OSSL_PARAM_construct_end();
so the code doesn't walk memory when copy/pasted.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for illustration purposes only. I do not declare params[2] = { END, END };

Copy link
Contributor

Choose a reason for hiding this comment

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

It will, nonetheless, be blindly copied...
The documentation should be exemplary.

doc/designs/fips_indicator.md Show resolved Hide resolved
callback may be called

```c
int ossl_INDICATOR_callback(OSSL_LIB_CTX *libctx, const char *algtype,
Copy link
Contributor

@paulidale paulidale May 16, 2024

Choose a reason for hiding this comment

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

The libctx argument here is meaningless. The FIPS provider's internal libctx is the only one available and it's unusable outside. I'd suggest PROV_CTX * here with a wrapper around the actual callback that converts it: const OSSL_CORE_HANDLE *handle = CORE_HANDLE(prov_ctx);

@paulidale
Copy link
Contributor

If wrappers are added to do the type conversions, it opens up a new possibility.

The FIPS provider only needs to know about the call back which gets called when something unFIPS is about to occur. On the libcrypto side, the wrapper can:

  1. call the user supplied function if it's been given;
  2. check the configuration for an override and
  3. check if the strict flag has been set on the EVP context (not the provider side one).

What this buys us is an almost complete dissociation of the exception mechanism from the FIPS provider. Keeping the FIPS provider clean is good because it's painful to modify. Having the checks in libcrypto saves passing lots of configuration values to the FIPS provider. It also means that none of the exception mechanisms are inside the boundary which will reduce queries.

@jvdsn
Copy link
Contributor

jvdsn commented May 16, 2024

What this buys us is an almost complete dissociation of the exception mechanism from the FIPS provider. Keeping the FIPS provider clean is good because it's painful to modify. Having the checks in libcrypto saves passing lots of configuration values to the FIPS provider. It also means that none of the exception mechanisms are inside the boundary which will reduce queries.

However, the FIPS validation only stops at the FIPS provider boundary, doesn't it? So you'd still have to make sure the callback mechanism in the FIPS provider meets all the requirements regarding indicators.

@paulidale
Copy link
Contributor

The callback is explicitly about logging and allowing non-FIPS operations. There is no issue with this happening outside the FIPS boundary. The checks that trigger it need to be inside but only them.

@slontis
Copy link
Member Author

slontis commented May 16, 2024

If wrappers are added to do the type conversions, it opens up a new possibility.

The FIPS provider only needs to know about the call back which gets called when something unFIPS is about to occur. On the libcrypto side, the wrapper can:

1. call the user supplied function if it's been given;

2. check the configuration for an override and

3. check if the strict flag has been set on the EVP context (not the provider side one).

What this buys us is an almost complete dissociation of the exception mechanism from the FIPS provider. Keeping the FIPS provider clean is good because it's painful to modify. Having the checks in libcrypto saves passing lots of configuration values to the FIPS provider. It also means that none of the exception mechanisms are inside the boundary which will reduce queries.

I am not seeing why this is any cleaner. Why would you want to parse the config again and handle every different ctx type externally.

@paulidale
Copy link
Contributor

I am not seeing why this is any cleaner. Why would you want to parse the config again and handle every different ctx type externally.

It avoid polluting the FIPS provider with all manner of mess.

  • It avoids passing (potentially) lots of Booleans to the FIPS provider via individual params: they will be available from the already parsed config on libcrypto's side.
  • We're going to have to handle every different ctx type regardless. We're going to need two flags: one to say if non-FIPS is allowed and the indicator. Where these are located isn't going to make a difference.
  • Reducing the perimeter of the FIPS provider is good and keeping logic in libcrypto achieves this. The FIPS provider gets exactly one callback added to cover all the exception logic.

@slontis
Copy link
Member Author

slontis commented May 16, 2024

The params will be passed to the provider regardless unless we explicitly black list individual flags.
Are you talking about using the callback to control the logic again? Where exactly does this code sit in the core (please look at the DSA sample and tell me exactly where this code would now sit, given that there is 2 seperate init checks for signing and key size)

@paulidale
Copy link
Contributor

I'm suggesting using the callback to make the yes/no decision and leaving this logic out of the provider. The callback being implemented by EVP and it potentially calls a user installed callback in addition to checking config etc.
All of the configuration settings can be kept outside of the provider -- it doesn't need to know or care about these as EVP will handle them. Likewise, we don't need to add anything to fipsinstall to cover the new options.

The OSSL_FIPS_INDICATOR_DECLARE macro and underlying ossl_FIPS_INDICATOR structure would be split. The strict part being in the EVP structure and the approved part in the provider's. When something non-FIPS is detected while doing an operation, the provider will clear the approved flag and call the callback. It will continue the operation or error out based on the callback's result.

The init checks would stay where they are on the provider's side & basically be unchanged. All such checks have to be internal to the FIPS boundary or we cannot claim indicators.

I'm mostly suggesting moving the logic from OSSL_FIPS_INDICATOR_DEFINE_NOT_APPROVED to the libcrypto side.

I think, but am not certain, that doing indicators this way might allow different versions of libcrypto and the FIPS provider to interoperate better moving forwards.

@slontis
Copy link
Member Author

slontis commented May 16, 2024

@paulidale After looking at your suggestion I dont think it will work. There was a deliberate decision in the providers for them to not know anything at all about the core ctx they came from (and the core retrieves a generic implementation blob back from the provider, that it also knows not much about). When a callback is triggered inside the fips provider algorithm, if it tries to run a core callback first it has no knowledge of the core ctx it was called from - so it would not know where to get the strict value from (unless it came from the provider)

@paulidale
Copy link
Contributor

We can pass an OSSL_CORE_HANDLE from the provider to the core. From this the owning OSSL_LIB_CTX can be obtained and passed to the user call back.

@slontis
Copy link
Member Author

slontis commented May 16, 2024

I am not talking about the libctx..

@paulidale
Copy link
Contributor

Which core ctx then?

@slontis
Copy link
Member Author

slontis commented May 16, 2024

Where do you think the strict flag is going to be retrieved from? The algorithm evp ctx.

@paulidale
Copy link
Contributor

Oh, that ctx...

@paulidale
Copy link
Contributor

It's possible if we change the provctx argument to point to the EVP_x_CTX instead of the provider. A bit more invasive and will have backward compatibility concerns....

The provctx should have pointed to the EVP_x_CTX originally 😞

@slontis
Copy link
Member Author

slontis commented May 16, 2024

Completely disagree, and changing the ptr is not feasible. It was done deliberately so that hacks could not be put into the provider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: review pending This pull request needs review by a committer tests: exempted The PR is exempt from requirements for testing triaged: design The issue/pr deals with a design document
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Finalize the FIPS Indicator design
10 participants