MDC checking

[kaie]

If an encrypted message doesn't have MDC, then RNP will decrypt it without warning.
Function rnp_op_verify_execute returns zero (good status).

Lack of MDC is considered a security issue, and an email agent should usually reject messages that don't use MDC.

Does RNP offer a mechanism to learn about the lack of MDC in a decrypted message?

If not, how about adding an API with a rnp_op_verify_t parameter, that can be called after the operation, and which will return details of the decryption operation?

[kaie]

This issue was originally reported by Patrick Brunschwig at https://bugzilla.mozilla.org/show_bug.cgi?id=1638645 which also has a test message attached.

The message is encrypted for Alice's key from https://gitlab.com/openpgp-wg/openpgp-samples

[ni4]

@kaie Thanks for reporting. I think we should also return some error code (like RNP_ERROR_LOW_SECURITY) instead of RNP_SUCCESS for such operation.

[kaie]

Maybe like this?
kaie@7a2ccc9

[kaie]

The encryption flag was added because of issue 1107.

[ni4]

Maybe like this?
kaie@7a2ccc9

I'd prefer slightly change this:

• do not change src_finish() params to transfer such specific information (since src_finish() is much lower-level object and should be used with all of the streams)
• use pgp_processing_ctx_t to fill flags information from encrypted_src, and propagate it to the upper level (maybe, via rnp_ctx_t).
• use single but extensible FFI interface function rnp_op_verify_get_protection_mode(rnp_op_verify_t op, char **mode, bool **valid), where mode may become 'none', 'mdc', 'aead-eax', 'aead-ocb', and all possible future values.

I can do PR with all these changes but slightly later, once finished with other tasks in queue.
Also, right now you may use packet dumping function to check whether packet was non-MDC:
rnp --list-packets --json ~/Downloads/example.msg

[
  {
    "header":{
      "offset":0,
      "tag":1,
      "tag.str":"Public-Key Encrypted Session Key",
      "raw":"85020c",
      "length":524,
      "partial":false,
      "indeterminate":false
    },
    "version":3,
    "keyid":"5f5fdf400616a9cf",
    "algorithm":1,
    "algorithm.str":"RSA (Encrypt or Sign)",
    "material":{
      "m.bits":4093
    }
  },
  {
    "header":{
      "offset":527,
      "tag":1,
      "tag.str":"Public-Key Encrypted Session Key",
      "raw":"845e",
      "length":94,
      "partial":false,
      "indeterminate":false
    },
    "version":3,
    "keyid":"4766f6b9d5f21eb6",
    "algorithm":18,
    "algorithm.str":"ECDH",
    "material":{
      "p.bits":263,
      "m.bytes":48
    }
  },
  {
    "header":{
      "offset":623,
      "tag":9,
      "tag.str":"Symmetrically Encrypted Data",
      "raw":"c97d",
      "length":125,
      "partial":false,
      "indeterminate":false
    }
  }
]

[kaie]

Your rnp_op_verify_get_protection_mode API suggestion seems fine. I'd prefer to avoid having to interpret the raw package dump.

[ni4]

@kaie Got it. Will make a PR as soon as I'll get to this.

[kaie]

I wonder how additional attributes about the encryption could get obtained.

I think we should have a way query which ciphersuites were used to encrypt the message (public key and symmetric).

Should we use the same API to also provide this information, and use an integer as the output parameter, not a bool?

It could be used to query
"does have MDC" -> bool 0/1
"does have AEAD" -> bool 0/1
"is encrypted" -> bool 0/1
"public key algo used" -> integer
"symmetric key algo used" -> integer

The application might want to have a whitelist of algorithms that are considered "sufficiently secure", and only show security indicators for messages that use an algorithm from the whitelist.

[kaie]

Another argument for not parsing the packet dump ourselves is, the JSON output format might not be guaranteed to be stable over time. A defined API seems more future safe.

[kaie]

Another idea to be even more flexible.

Two input parameters:

• string - the value being queried
• integer - an optional index being queried (e.g. recipient 5)

Two output parameters:

• integer (bool or number)
• string

Which output parameter is filled is decided by the implementation, based on the value being queried.

I don't know if this makes sense, or if we're rebuilding the flexibility of JSON.
The alternative could be a JSON output, which you guarantee to be stable.

[kaie]

Actually, given that the tag numbers and algorithms identifiers are officially defined, maybe your original suggestion to look at the package dump makes sense after all. I'll think about it a bit more.

[ni4]

@kaie We definitely should provide some easy interface to obtain this information, JSON output was proposed as the temporary solution/or solution for cases where more detailed information is needed. Also idea is to keep JSON output format the same, together with FFI interface. I.e. use rule 'new things could be added, old things should not be changed'.

What do you think about the following approach with 3+ functions:

• rnp_op_verify_get_protection_mode(rnp_op_verify_t op, char **mode, bool **valid), where mode may become 'none', 'mdc', 'aead-eax', 'aead-ocb', and all possible future values.
• rnp_op_verify_get_protection_cipher(rnp_op_verify_t op, char **cipher), where cipher may become 'none', 'AES128', etc.
• rnp_op_verify_get_recipient_count(rnp_op_verify_t op, size_t *count), rnp_op_verify_get_recipient(rnp_op_verify_t op, rnp_recipient_t *recipient, size_t idx)
• bunch of functions to query recipient's information: whether it is public key or password (not sure whether password encryption is used in email protection, but would be usable by other implementations), recipient's keyid, which algorithm was used to encrypt the message encryption key (say, while message is encrypted with strong AES-256, but encryption key is encrypted with DES, then we have questions).

[kaie]

* `rnp_op_verify_get_protection_mode(rnp_op_verify_t op, char **mode, bool **valid)`, where mode may become 'none', 'mdc', 'aead-eax', 'aead-ocb', and all possible future values.

If these are mutually exclusive, would a string return value better? It would avoid having to call the function multiple times.

* `rnp_op_verify_get_protection_cipher(rnp_op_verify_t op, char **cipher)`, where cipher may become 'none', 'AES128', etc.

'none' would indicate that no encryption layer was present?

Does OpenPGP define a way to use an encryption layer with a null cipher for testing? If yes, how would we distinguish that?

* `rnp_op_verify_get_recipient_count(rnp_op_verify_t op, size_t *count)`, `rnp_op_verify_get_recipient(rnp_op_verify_t op, rnp_recipient_t *recipient, size_t idx)`

seems good overall

[ni4]

If these are mutually exclusive, would a string return value better? It would avoid having to call the function multiple times.

All FFI calls return RNP_SUCCESS or error code, so this one should follow the same rule. And why would you need to call it multiple times? It would allocate memory itself (on success), like any other function, and there is only single possible result.

Does OpenPGP define a way to use an encryption layer with a null cipher for testing? If yes, how would we distinguish that?

There is no null cipher in OpenPGP, but anyway we would be able to use string 'null' for that, for instance.

[kaie]

All FFI calls return RNP_SUCCESS or error code, so this one should follow the same rule. And why would you need to call it multiple times?

Sorry, I had missed that you defined rnp_op_verify_get_protection_mode uses two output parameters. (I had falsely assumed its one input and one output.)

[lambdafu]

Just an inconsequential side note: OpenPGP does specify a null cipher (see section 9.2 id 0 - "Plaintext or unencrypted data"), although I have never seen it supported in practice. If you try to decrypt such a packet with gpg, you get an error "Invalid cipher algorithm".

[ni4]

@lambdafu Thanks for the clarification. If that would ever required we may return 'null' instead of 'none' so such case may be checked by caller.

[ni4]

@kaie Could you please check the function interface from PR #1170, whether it will give you enough information? It doesn't add function to retrieve recipient list and their properties - this would be added as a separate PR.

[kaie]

@ni4 Would it be possible to add another output parameter to rnp_op_verify_get_protection_info, in order to obtain the rnp_key_handle_t of the local secret key that was used to decrypt the message? (This would be a more reliable and direct way than the key unlock callback.) This could be used to learn which encryption mechanism was used to to transport the message key.

[kaie]

Alternatively, it could be a separate API. If you don't like the suggestion, or if want to think more about it, then in the meantime I can keep using the unlock callback to get that information.

[kaie]

IIUC, the "mode" parameter tells us, which mechanism the message intended to use. And parameter "valid" tells us, if that mechanism was successfully used, or if the related checks failed. Because there aren't any checks for mechanism "cfb", you want to always return "false" for cfb.

That seems fine.

Now I have an additional idea. Instead of returning the key handle for the related secret key, you could simply return the algorithm of the related public/private key (or decryption operation). So the output parameters of rnp_op_verify_get_protection_info would inform about both the symmetric cipher, and about the public/private key cipher that was used for decrypting the message.

[ni4]

@kaie

Would it be possible to add another output parameter to rnp_op_verify_get_protection_info, in order to obtain the rnp_key_handle_t of the local secret key that was used to decrypt the message?

This functionality is planned in the next PR I mentioned - something like get_recipient_count()/get_recipient_at()/get_used_recipient()/get_recipient_protection_info().

Returning only the information about the key, used during decryption, is not enough from the security perspective. Say, message can be encrypted to 4096-bit RSA key, but it is also encrypted to the 512-bit old RSA key, or with password with 16 hashing iterations. I.e. encryption strength is the lowest one from the recipient list.

[kaie]

Ok, makes sense.
So let's postpone that for now.
The suggested API in PR 1170 seems fine to me.
Thanks