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

Update of BSI policy for BSI guidelines 2023 #3482

Merged
merged 1 commit into from
May 12, 2023

Conversation

FAlbertDev
Copy link
Collaborator

I've checked Botan's BSI policy against the newest 'BSI – Technical Guideline' document to see if the current policy is still up to date. This PR contains some adaptions. Since I'm still quite new to Botan and are not familiar with all algorithms, I hope that you can verify and double check my changes. In the following I give the reasoning for all changes I've done:

  1. Removed the XTS mode, since it does not appear in Table 2.2 of the Guideline.
  2. Added the PQC signature scheme Dilithium (Kyber/XMSS were already added). The BSI states the following:

The international standardisation of PQC mechanisms, in particular by the American National Institute of Standards and Technology (NIST), has meanwhile started and first mechanisms have been selected for standardisation. Corresponding standards are expected in the coming years. Introducing current, non-standardised mechanisms in new cryptographic systems is therefore always associated with the risk of creating systems that are incompatible with standards that are foreseeable for the near future. However, in applications that are intended to guarantee the confidentiality of information with a high value and a long-term need for protection, these problems weigh less heavily in the BSI’s view than the possibility of future attacks. In general, it is recommended to put great stress on cryptoagility in the design of new systems;

For me it sounds like PQC algorithms should be used with caution, but are basically allowed.
3. Added argon2_avx2 for password based KDFs. I see no reason why not (argon2_ssse3 is already enabled).
4. The BSI states the following:

If the use of a a cryptographic hardware token for password-based key derivation is not possible, the hash function Argon2id should be used. The security parameters of Argon2id and the requirements for the passwords depend on the application scenario and should be discussed with an expert.

For me it sounds like only Argon2 is recommended for PBKDFs. Therefore, i would forbid all other PBKDFs, i.e, pbkdf2, bcrypt_pbkdf, pgp_s2k, and scrypt. Also the password hashes passhash9 and bcrypt(already prohibited).
5. Added getentropy as entropy collection. I don't know much about there different entropy collection systems, but it seems that BSD's getentropy has high security claims. Also the BSI does not specify specific algorithms as far as I know.
6. Reenabled hkdf. I was told it is required for TLS 1.3.
7. Disabled the public key padding modules eme_raw. Should be sensible.
8. Disabled the hash module raw_hash. At first glance I could not find other modules using raw_hash. Otherwise, it should definitely be disabled.

@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.02 ⚠️

Comparison is base (51b06ca) 88.22% compared to head (a6d174c) 88.20%.

❗ Current head a6d174c differs from pull request most recent head e038309. Consider uploading reports for the commit e038309 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3482      +/-   ##
==========================================
- Coverage   88.22%   88.20%   -0.02%     
==========================================
  Files         617      617              
  Lines       70386    70385       -1     
  Branches     6818     6821       +3     
==========================================
- Hits        62095    62082      -13     
- Misses       5431     5443      +12     
  Partials     2860     2860              

see 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@reneme reneme left a comment

Choose a reason for hiding this comment

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

For Botan 3.0 we should just update the policy so that new things can be used. (e.g. by removing HKDF from the prohibited-list in favor of TLS 1.3). Additional prohibitions should be used scarcely. We might re-iterate that at a later stage.

src/build-data/policy/bsi.txt Outdated Show resolved Hide resolved
Comment on lines 148 to 153
# pbkdf
bcrypt_pbkdf
pbkdf2
pgp_s2k
scrypt

Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as BSI doesn't explicitly discourage their usage, we should not prohibit them. Just not enable by default. E.g. pgp_s2k: I could imagine that the PGP-related BSI project might even depend on it.

emsa_raw
emsa_x931
raw_hash
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a module to implement certain special cases using Botan. Shouldn't be enabled by default but also not prohibited.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This new module is from my understanding now used in e.g. ECDSA there before emsa_raw was used (as they required EMSA1(hash) before instead of the hash directly).

Don't know why we currently forbid emsa_raw, but we may want to keep it consistent an either forbid both or none.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll remove it for now.

@@ -153,8 +164,10 @@ sm2
# pk_pad
#eme_pkcs1 // needed for tls
#emsa_pkcs1 // needed for tls
eme_raw
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't prohibit, just not part of the default module set, please.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would like to create a comment for eme_raw, why it is not prohibited like some other paddings, or where it is used (like the lines above). Is it also used by TLS?

Copy link
Collaborator

Choose a reason for hiding this comment

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

From what I understand it is not strictly needed by any other module. Though, it allows applications to implement their own RSA encryption padding scheme. Basically, do the padding yourself and then encrypt with "EME Raw". No reason to prohibit that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree the needed is misleading here. I'm not aware of any module there eme_raw and raw_hash are needed.
It's just that we don't want to prohibit them in case a user needs them to implement a special (corner) case.

@@ -182,5 +195,6 @@ x919_mac

# passhash
bcrypt
passhash9
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do they explicitly prohibit PBKDF2? If no, let's just leave it to the user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The guideline does not explicitly prohibit anything as far as I know. They only recommend the most secure stuff. In this case they only recommend Argon2:

If the use of a a cryptographic hardware token for password-based key derivation is not possible,
the hash function Argon2id [14] should be used.

I would suggest the same as for the cipher modes: If there is no important dependency we should stay consistent and add or remove both.

Additional Note: Wikipedia quote:

One weakness of PBKDF2 is that while its number of iterations can be adjusted to make it take an arbitrarily large amount of computing time, it can be implemented with a small circuit and very little RAM, which makes brute-force attacks using application-specific integrated circuits or graphics processing units relatively cheap.

Not critical, but the reason to create a competition to create a better algorithm (which was Argon2).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now, I will remove the prohibition as suggested.

@FAlbertDev
Copy link
Collaborator Author

I've disabled the PQC algorithms kyber and dilithium for now, since they are not yet wanted in the current policy. Maybe these will be activated in the next iteration. (4fe8b03)

@FAlbertDev
Copy link
Collaborator Author

I would suggest that we leave the current changes for now. In the following weeks we are planning to agree with the BSI about the goals of the policy (including prohibition strategy, etc.). Afterward, we may need further adaptions.
Until then, these changes should (at least) enable the TLS 1.3 modules in this policy.

@randombit
Copy link
Owner

Please rebase and squash before merge

@FAlbertDev FAlbertDev force-pushed the bsi-policy-update branch from 569c9bb to 2686d91 Compare May 4, 2023 08:58
@coveralls
Copy link

coveralls commented May 4, 2023

Coverage Status

Coverage: 91.726%. Remained the same when pulling e038309 on FAlbertDev:bsi-policy-update into b37705f on randombit:master.

@FAlbertDev FAlbertDev force-pushed the bsi-policy-update branch from 2686d91 to e038309 Compare May 4, 2023 09:47
@reneme reneme merged commit ce428cb into randombit:master May 12, 2023
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.

7 participants