Fix CMS encryption with key agreement crashing when originator set

[bukka]

This is a fix for the segfault that I noticed sometime ago. OpenSSL currently does not support explicitly setting originator private and pub key (from cert). The cms command actually documents that originator can be used only for decryption but it still allows setting for encryption which results in segmentation fault (the test actually segfault without this change). The API actually does not specify such restriction and would just segfault. This is a minimal fix to just prevent the segfault so it can be cherry-picked to lower branches (haven't checked if it's clean or there is conflict - there might be in number of tests possibly). I can create possibly PR's for other branches. Just let me know which ones.

After this gets merged I would like to look to allowing the originator to be set. This is something that BouncyCastle support and it is quite useful. Such change is just for master though so I did this first. The thing is that it requires quite a few places to be changed and some changes to the structs as well. And a bit more testing of course.

Checklist

• tests are added or updated

[bukka]

Ok that leak (address_ub_sanitizer failure) seems like something that was probably there but my test triggers. Will look into it next week.

[t8m]

Is it ready for review now?

[bukka]

I will create a new PR for that originator leak with a different test. I realised that there is actually no test for valid originator usage in decryption so it would be good to have that first. Will do it this week.

[bukka]

@t8m So it is actually ready for review. I was just checking if I could add test separately for that leak (cms decryption usage only) and it's not actually that easy because OpenSSL support only encryption without originator which means that the empheral key is created. So this option is useful only for interoperability with other libs where setting originator is possible - for example I have got this interop example test for BouncyCastle - https://github.com/bukka/jcrypto/blob/6978ebb8e7b162380785315e3f6ee3c904c0caf4/examples/cms/cms-enveloped-key-agree-test.sh#L45-L47 . Here it can be used for a message encrypted using BouncyCastle (through my toy app jcrypto).

I plan to look to the originator encryption support which would make it work in line with BouncyCastle where this is already possible. But that will be just for master as a new feature.

[bukka]

I just fixed the pem new line and swap the commits - they are independent so it makes sense to not use a single commit for the PR but instead cherry-pick them independently (in case it doesn't apply to lower branches, let me know and I will create a separate PR). The leak fix should go first as it is prereq for the second commit to be green.

[DDvO]

The cms command actually documents that originator can be used only for decryption but it still allows setting for encryption which results in segmentation fault

Where do you see in the openssl-cms man page that setting the originator cert is not allowed for encryption?
It just says that it is necessary in some case but does not say it is forbidden/impossible/unsupported in others:

A certificate of the originator of the encrypted message. Necessary for decryption when Key
Agreement is in use for a shared key.

Please add to the doc there that for encryption with key agreement, setting a static originator is so far not supported.

Your fix prevents a crash in this case by returning instead an error at API level, but when CLI users thus get

408F92F201000000:error:17000071:CMS routines:ossl_cms_RecipientInfo_kari_encrypt:error getting public key:crypto/cms/cms_kari.c:510:
408F92F201000000:error:17000074:CMS routines:cms_EnvelopedData_Encryption_init_bio:error setting recipientinfo:crypto/cms/cms_env.c:1185:
408F92F201000000:error:17000068:CMS routines:CMS_final:cms lib:crypto/cms/cms_smime.c:906:

most of them will not understand what happened.
So please add a check also in apps/cms.c with a telling user-level error, saying that this is not supported.

[DDvO]

Please also simplify the new test as suggested.

[bukka]

Where do you see in the openssl-cms man page that setting the originator cert is not allowed for encryption?

The docs say:

A certificate of the originator of the encrypted message

So it means that it's just for decryption because obviously you will not use encryption for encrpyted message...

[bukka]

@DDvO Ok so I fixed the test as suggested. I'm not so sure about the cms app changes because currently originator is ignored for non KARI encryption so theoretically it could be a minor BC break if someone has got it there by mistake for non kari as their script still works - checking that's it's kari before encryption might be a bit hacky... Also if I change it, the test will no longer test the API change which is the main point here. So I would need and API test which is much more involved (current implementation is very limited) and doing that for something that I want to change in the near future seems a bit like overkill to me. Also I don't think there are too many users hitting this - it's been segfaulting for ages and no one ever complaint - I found it just because I did some full interop test. My main aim was really just to fix the segfault and then address it properly in master. So do you still want me to change the app?

[bukka]

I just realised that I can actually output more explaining error in cms app after the error by checking the code and origin so it should do and will address the comment. I will also add extra sentence to the docs and reword the test comment. Most likely next week as I'm past my short weekly OpenSSL slot. :)

[bukka]

@DDvO Ok I thought about changing the error before but wasn't sure if it's acceptable for backports (this is mainly meant as a backport really). Anyway I changed the error now as it seems you are both ok with it. But not sure if it's really more explaining for users so I kept the extra info in the app but if you don't want it there, happy to remove it. In addition, I needed to change the test back to checking the error code as it was actually passing with the current master code (segfault results in the false as well so it could not differentiate segfault from the normal failure - the error code needs to be checked for that).

Let me know, if there's anything else. Once you are both happy, I can create PR for lower branches (let me know which ones) if it doesn't apply cleanly.

[openssl-machine]

This pull request is ready to merge

[t8m]

Merged to all the active branches. Thank you for your contribution.