Check for weak hash algorithms

[antonsviridenko]

Partially addresses #1281. Other possible weak algorithms (symmetric encryption, public key, etc) should be added later. This PR adds new API function rnp_is_weak_hash() and prevents CLI from creating new keys, signatures, encrypted messages having weak hashes unless new option --allow-weak is explicitly passed.

[ni4]

@antonsviridenko Instead of making such function for each hash/symmetric algo/curve/whatever else we could use single and extensible one, as it is done in rnp_supports_feature/rnp_supported_features. For instance, later on we may add features for RSA/DSA key sizes (min-max), and so on. I.e. something like rnp_weak_feature(type, bool)/rnp_weak_features(type, string) (better naming ideas are welcome!). Also we have --force option which may be reused instead of adding new --allow-weak.

[antonsviridenko]

Ok, what other algorithms can be added as weak now? Or keep only hash for now?

I.e. something like rnp_weak_feature(type, bool)/rnp_weak_features(type, string)

rnp_weak_algorithm()/rnp_weak_algorithms()?

Also we have --force option which may be reused instead of adding new --allow-weak

I think force has bit different semantics

[ni4]

Ok, what other algorithms can be added as weak now? Or keep only hash for now?

Let's mark IDEA and 3DES as weak for now.
@ronaldtse Does this sound good for you?

Also we have --force option which may be reused instead of adding new --allow-weak I think force has bit different semantics

Why? It is used to complete an action which normally would not be completed due to some logic, and already available. Another thing is that having too much switches would make usage of RNP CLI too complicated (see, for instance man gpg).

[antonsviridenko]

Why? It is used to complete an action which normally would not be completed due to some logic, and already available. Another thing is that having too much switches would make usage of RNP CLI too complicated (see, for instance man gpg).

@ni4 for example users can already use --force flag for some other purpose, like overwriting destination file and weak algorithms can be unintentionally enabled this way, so I think having separate flag is a better idea for security reasons

[ni4]

@ni4 for example users can already use --force flag for some other purpose, like overwriting destination file and weak algorithms can be unintentionally enabled this way, so I think having separate flag is a better idea for security reasons

Makes sense, let's have a separate option for this. However, what do you think about renaming it to the --force-weak, so it will be aligned with --force and future --force-somethings.

[antonsviridenko]

@ni4 ok, renamed to --force-weak

[ni4]

I.e. something like rnp_weak_feature(type, bool)/rnp_weak_features(type, string)

rnp_weak_algorithm()/rnp_weak_algorithms()?

@antonsviridenko Sorry, mislooked this question. Basically I proposed feature semantics since it can go beyond of just algorithms in a future - it may be curve, RSA key size, MDC method, number of iterations in S2K and so on.

[antonsviridenko]

@antonsviridenko Sorry, mislooked this question. Basically I proposed feature semantics since it can go beyond of just algorithms in a future - it may be curve, RSA key size, MDC method, number of iterations in S2K and so on.

But in this case the function needs at least one more parameter, to pass key size argument, numer of iterations and so on. For example, RSA is considered as a secure algorithm, but key sizes less than 1024 (or 2048 now) are weak.

[antonsviridenko]

so rnp_weak_feature() is too generic, maybe it's better to have something like rnp_weak_key_size(type,size, bool is_weak)

[ni4]

@antonsviridenko Sorry, mislooked this question. Basically I proposed feature semantics since it can go beyond of just algorithms in a future - it may be curve, RSA key size, MDC method, number of iterations in S2K and so on.

But in this case the function needs at least one more parameter, to pass key size argument, numer of iterations and so on. For example, RSA is considered as a secure algorithm, but key sizes less than 1024 (or 2048 now) are weak.

It may be checked by attaching key size to the algorithm in a documented way, like "RSA-1024". The same can be done with rnp_supported_features(), returning "RSA-1024-16384".

so rnp_weak_feature() is too generic, maybe it's better to have something like rnp_weak_key_size(type,size, bool is_weak)

That would also require us to add functions rnp_supported_key_sizes() and rnp_supports_key_size() so API would look consistent. It think we should not bloat the API in this way, when functionality may be expressed via slightly more complicated string parameter.

[antonsviridenko]

ping @ni4 @rrrooommmaaa @ronaldtse

[ronaldtse]

I think there are concepts of "weak algorithms" and "weak algorithm usage" and maybe also "weak algorithm combinations".

We should settle on a data model that allows someone to specify the user's preference: what is good, what is acceptable but weak, what is unacceptable. We can call this a "cryptographic preference profile".

e.g. for RSA

{
  "rsa" : {
    "keysize_min": [
      /* anything less will score "unacceptable" */
      {
        "value": 1024,
        "score": "acceptable"
      },
      {
        "value": 2048,
        "score": "preferred"
      },
    ]
  }
}

We should be able to take different parameters for different algorithms.

This way we can have two of these profiles:

• RNP global: represents RNP's recommended profile, potentially we can modify them by time, just like how AWS changes their TLS profile by time (while keeping older profiles available).
• User specified: whatever the user decides

[antonsviridenko]

What are we going to do with this PR?

[ronaldtse]

@antonsviridenko how about we merge this first and implement https://github.com/rnpgp/rnp/pull/1524#issuecomment-869193318 ?

[ronaldtse]

Had a discussion with @ribose-jeffreylau and propose we move ahead with this scheme as follows.

The user defines a set of security levels (or whatever they wish to call them).

• e.g. 0-3 with 0 unacceptable, 1 deprecated, 2 safe, 3 possibly future-proof for X years

Then the user associates the following objects each with a certain security level:

• symmetric encryption algorithm
• asymmetric encryption algorithm
• hash algorithm

For each algorithm they can specify:

• validity period as date-time ranges (from, to, from+to) and what levels they lead to
• what key length/size leads to what level

Using JSON to illustrate here: https://gist.github.com/ronaldtse/e7569de55a4ddb476d9881b7e5267e3c

What do you think?

UPDATED: The JSON has been edited and is good to go for review -- @ni4 @antonsviridenko @dewyatt can you have a look at it? Thanks!

[dkg]

I added comments on the gist, but i'll repeat them here, since i don't know whether the gist will go away:

---

I'm not sure i understand why the user is defining different levels? Will the level also be passed into the library when it is used, e.g. by fiddling with the rnp_ffi_t object?

If so, why not just specify a single policy on the rnp_ffi_t object, then if the user wants different levels, they can just instantiate multiple rnp_ffi_t objects, each with different policies.

Or are you imagining that some operations might return some complex semantics like "this would have worked if you were in level 3, but since you're in level 4 it fails"?

It seems simpler and cleaner to to define a single policy than multiple ones, and avoiding complex return semantics also seems like a usability win.

---

I also like having the date cutoffs, but the way these are specified seems like they could be potentially ambiguous/overlapping. It's also not entirely clear to me what they mean. For example, when you say (for example) a given hash is valid "from" a certain time, does that mean that when verifying a signature, the date in the signature is checked? or the current time is checked? does it mean "stop making signatures with this hash at this date"? What does it mean for a date limit to be placed on an encryption algorithm?

By comparison, the sop draft specifies that it's looking at signature dates, not the current time (but that of course isn't bound to a specific algorithm, and it's only relevant for signature verification). What's proposed here is much more complex, and therefore needs clearer specification, i think.