Skip to content

Add CMS AuthEnvelopedData with AES-GCM support #8024

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

Closed
wants to merge 1 commit into from

Conversation

bukka
Copy link
Contributor

@bukka bukka commented Jan 15, 2019

This PR adds support for CMS AuthEnvelopedData as defined in RFC 5083 with AES-GCM parameter as defined in RFC 5084.

I think it useful to support GCM as it allows the tag to be stored with the message. Especially if the cms is now recommended tool for AEAD and there was a decision not to introduce special format for apps enc to handle AEAD modes IIRC.

It works already with use of apps cms when selecting for example -aes-128-gcm cipher. It updates CMS_encrypt to automatically use AuthEnvelopedData for GCM modes so the application doesn't require new API. As there is now new special API, it means that there is currently no support for AAD but that's something that I would like to look in the future as part of another PR. Also CCM mode is not supported as it's a bit more tricky (mainly because the length needs to be known in advance) but I plan to look into it in the future too.

The documentation is partially updated which means that I updated CMS_encrypt and CMS_decrypt. The reason why I left the rest out is that I'm not sure if the new public functions should be really public. Currently I added them as a public for consistency. For example EVP_CIPHER_set_asn1_aead_params is an AEAD variant for EVP_CIPHER_set_asn1_iv so it might be useful to expose it possibly for engines use. Similarly ASN1_TYPE_set_octetstring_int is consistent with public ASN1_TYPE_set_int_octetstring (also not documented). In addition the new CMS_AuthEnvelopedData_create is like exposed CMS_EnvelopedData_create (also not documented) but for AuthEnvelopedData of course. Considering that all the existing mentioned functions are not documented, it might be better to leave the documentation for another PR and do it together with the existing once. I would be happy to do that later.

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

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jan 15, 2019
@bukka
Copy link
Contributor Author

bukka commented Jan 15, 2019

FYI I have sent ICLA this morning.

@bukka bukka closed this Jan 16, 2019
@bukka bukka reopened this Jan 16, 2019
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jan 16, 2019
@russhousley
Copy link

Thanks for all of this very valuable work. The S/MIME 4.0 specifications are in the final stages, and they make use of AuthEnvelopedData!

Ideally, in the future, AAD support for the authenticated attributes in AuthEnvelopedData would be the next step.

@bukka
Copy link
Contributor Author

bukka commented Jan 18, 2019

@russhousley Yes I plan to look on adding AAD support after this gets reviewed and hopefully merged. I'm still thinking about the best API for that but the implementation should be fairly easy. Although I think it will require some improvements in the CMS testing as it's quite limited atm. I would like to look on adding ChaCha20-Poly1305 (thank you for creating RFC 8103 btw.) as well and I also thought about CCM support but it will probably require loading the whole plaintext to the memory BIO to get the length before it's processed by cipher BIO. That needs a bit more thinking though!

@wolftobias
Copy link

@levitte Could you pls take a look at this? I also would need the support for 'AuthEnvelopData'.

@wolftobias
Copy link

@bukka @russhousley @FdaSilvaYY @mattcaswell Hi guys, I think here is a 2nd reviewer needed! Who can support us?

@wolftobias
Copy link

@bukka You developed this on the future openssl version 3.0.0, right?

[ "-decrypt", "-in", "test.cms", "-out", "smtst.txt", "-inform", "PEM",
"-secretkey", "000102030405060708090A0B0C0D0E0F",
"-secretkeyid", "C0FEE0" ]
],
Copy link
Member

Choose a reason for hiding this comment

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

I know your just following the way the other CMS tests are done, but really I'd like to see stronger tests that this. Rather than just testing things through the command line we should really be testing the API directly...e.g. with some negative tests too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree! I guess it might make more sense if I create a new with some tests for encryption and then just extend it in this PR. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Again I'd be delighted if you want to do that. I would accept more minimal testing which just tests the newly added functions in order to approve this PR. I'll leave it with you to decide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I plan to add those tests and already started looking into it. It's basically next thing that I want to do before I start looking to the CCM and ChaCha20-Poly1305 in CMS as it's currently quite a pain to debug issues and API tests would really help.

I'm just thinking that it would be helpful for me if it doesn't have to be requirement for this PR? The reason is that I'm spending quite a bit of time just on rebasing it and integrating changes into it (e.g. the GOST related one took a bit of time). If it doesn't need to wait for adding a new way of testing in CMS, that would be great as it would save me some time!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just extended the cmsapitest.c to at least cover the gcm mode. It's not a great test but hope it should do for this PR.

I was thinking about improvements in CMS testing and I think that the main issue it that the actual ASN.1 elements are not verified in any way. It's basically just encrypted envelope can be decrypted and the result is the same. But if there was some structural bug in both, then it could be easily missed. Maybe some kind of match for asn1parse as part of cli test..? Anyway that's more for a different PR but thoughts on this would be appreciated.

@bukka bukka force-pushed the cms-auth-env-gcm branch from 10778e5 to b74eca6 Compare June 16, 2019 18:23
@bukka
Copy link
Contributor Author

bukka commented Jun 16, 2019

@bukka You developed this on the future openssl version 3.0.0, right?

@wolftobias Yes it targets 3.0.0. I just rebased it on latest master.

@wolftobias
Copy link

I need this new feature on the current v1.1.1xxx, maybe I have to integrate it manually in my local build, version 1.1.1 has long time support till 2023 and version 3 has not target release date yet. Do you or anybody else see a chance to make this available for v1.1.1?

@mattcaswell
Copy link
Member

Do you or anybody else see a chance to make this available for v1.1.1?

Unfortunately not. 1.1.1 is a stable release. We don't add new features to stable releases (and this is counted as a new feature).

@wolftobias
Copy link

Do you or anybody else see a chance to make this available for v1.1.1?

Unfortunately not. 1.1.1 is a stable release. We don't add new features to stable releases (and this is counted as a new feature).

Yes I understand your point. Is there already an idea about the target release date of v3?

@mattcaswell
Copy link
Member

Yes I understand your point. Is there already an idea about the target release date of v3?

We hope to be code complete by the end of this year, i.e. beta release around that point. With a possible release around early Q2 2020 (FIPS validation would come some time later).

@wolftobias
Copy link

@bukka @mattcaswell What's the current status?

@mattcaswell
Copy link
Member

Oh...sorry, it looks like @bukka previously responded to earlier review comments and is waiting on me to respond to some questions. I'll try and answer those now.

@mattcaswell
Copy link
Member

I've responded to the various comments above...over to @bukka. Looks like this PR needs a rebase due to conflicts with master

@bukka
Copy link
Contributor Author

bukka commented Jul 21, 2019

I have been bit busy lately. Will try to address all the outstanding bits when I have got a bit more free time.

@wolftobias
Copy link

I have been bit busy lately. Will try to address all the outstanding bits when I have got a bit more free time.

time is always against us! But it would be great if it would be possible for you to finish the open tasks, otherwise we're risking not to go in the next release and that will be sad. Also the reviewers took there time to work on this.

@wolftobias
Copy link

@bukka I tried to test your unauthenticated attribute feature you developed, but the result is the error below. Could you please help me further?

C:\download\openssl-bukka\win64\bin>openssl cms -encrypt -in test.txt -out test.txt.enc -aes-128-gcm -secretkey 000102030405060708090A0B0C0D0E0F -secretkeyid C0FEE0
35672:error:060CD0E4:digital envelope routines:EVP_CIPHER_param_to_asn1:reason(228):crypto/evp/evp_lib.c:46:
35672:error:2E078066:CMS routines:cms_EncryptedContent_init_bio:cipher parameter initialisation error:crypto/cms/cms_enc.c:142:
35672:error:2E07F068:CMS routines:CMS_final:cms lib:crypto/cms/cms_smime.c:764:

@bukka
Copy link
Contributor Author

bukka commented Sep 7, 2020

@slontis Addressed the last 2 comments.

@slontis
Copy link
Member

slontis commented Sep 7, 2020

Unless you really need to rebase could you please start using commit fixups..
It makes it a lot harder to review your changes on something large if you do it via force pushing..

To do this you do

git log
To view your commits and get the hash of the commit you want to add a fixup to..
git commit --fixup hashid
git push

Note you can also do this with the generated files (e.g libcrypto.num)..

@slontis
Copy link
Member

slontis commented Sep 7, 2020

Have a look at something simple like
https://github.com/openssl/openssl/pull/12795/commits
And you be able to see the change fixups..
At the moment you are doing monolithic blobs - which means I have to scan everything each time (not just what you changed).

@slontis
Copy link
Member

slontis commented Sep 7, 2020

NOTE: when we merge the committers can easily squash these fixups as part of the process.

Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

Approved assuming the CI loop passes.

I wish we had something to interop against though - your code is likely to become the thing that is interoped against in the future :)

@slontis slontis added approval: review pending This pull request needs review by a committer and removed approval: otc review pending labels Sep 7, 2020
@bukka
Copy link
Contributor Author

bukka commented Sep 7, 2020

@slontis Thanks for the approval.

The reason why I kept it in a single commit is because I needed to rebase it quite a few times and there were lots of conflicts. If I had lots of commits, it would be really a pain to rebase it. Although I guess I could just merge master in if it's something that you are fine with? I can certainly do that next time. It works for me as soon as I don't need to rebase multiple commits...

@slontis
Copy link
Member

slontis commented Sep 7, 2020

The reason why I kept it in a single commit is because I needed to rebase it quite a few times and there were lots of

I understand.. What I do is collapse it when it is ready to start getting reviews.. Then once people start reviewing just do the fixups to make it easy to review afte the initial review.. If it is going to sit there for a long time and cause you lots of conflicts later then collapse it again and start the fixups process again when it starts getting reviewed again.

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Sep 7, 2020
@t8m
Copy link
Member

t8m commented Sep 7, 2020

Approved, if the CI passes.

@bukka
Copy link
Contributor Author

bukka commented Sep 7, 2020

@slontis @t8m @mattcaswell The CI is green so hopefully it's good to be merged.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

Add the AuthEnvelopedData as defined in RFC 5083 with AES-GCM
parameter as defined in RFC 5084.
@t8m t8m force-pushed the cms-auth-env-gcm branch from 7ecbfcc to a9eac52 Compare September 8, 2020 13:42
openssl-machine pushed a commit that referenced this pull request Sep 8, 2020
Add the AuthEnvelopedData as defined in RFC 5083 with AES-GCM
parameter as defined in RFC 5084.

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #8024)
@t8m
Copy link
Member

t8m commented Sep 8, 2020

In the force push I've rebased and resolved trivial conflict in libcrypto.num.
Merged to master.
Thank you for the contributed PR and for the patience with the reviews and merging.

@t8m t8m closed this Sep 8, 2020
@elear
Copy link

elear commented Sep 15, 2020

@bukka does this require a PR for PHP?

@bukka
Copy link
Contributor Author

bukka commented Sep 17, 2020

@elear No, it should just work.

@lukaaash
Copy link

@bukka I think that a bug was introduced with this PR, in the following cms_asn1.c code:

/* Defined in RFC 5083 - Section 2.1. AuthEnvelopedData Type */
ASN1_NDEF_SEQUENCE(CMS_AuthEnvelopedData) = {
        ASN1_EMBED(CMS_AuthEnvelopedData, version, INT32),
        ASN1_IMP_OPT(CMS_AuthEnvelopedData, originatorInfo, CMS_OriginatorInfo, 0),
        ASN1_SET_OF(CMS_AuthEnvelopedData, recipientInfos, CMS_RecipientInfo),
        ASN1_SIMPLE(CMS_AuthEnvelopedData, authEncryptedContentInfo, CMS_EncryptedContentInfo),
        ASN1_IMP_SET_OF_OPT(CMS_AuthEnvelopedData, authAttrs, X509_ALGOR, 2),
        ASN1_SIMPLE(CMS_AuthEnvelopedData, mac, ASN1_OCTET_STRING),
        ASN1_IMP_SET_OF_OPT(CMS_AuthEnvelopedData, unauthAttrs, X509_ALGOR, 3)
} ASN1_NDEF_SEQUENCE_END(CMS_AuthEnvelopedData)

I believe that the two lines with authAttrs and unauthAttrs should actually look like this:

        ASN1_IMP_SET_OF_OPT(CMS_AuthEnvelopedData, authAttrs, X509_ATTRIBUTE, 1),
        ...
        ASN1_IMP_SET_OF_OPT(CMS_AuthEnvelopedData, unauthAttrs, X509_ATTRIBUTE, 2)

(It looks like the implicit IDs were copy&pasted from CMS_AuthenticatedData but not changed according to RFC 5083. Additionally, it seems that even the attributes in CMS_AuthenticatedData should use X509_ATTRIBUTE instead of X509_ALGOR.)

Ironically, it's quite fortunate that this bug exists - it causes OpenSSL to fail when it tries parsing AuthEnvelopedData that actually has authAttrs. Otherwise, it would likely fail on decryption because authAttrs is not used as AAD input for AES-GCM, as required by the RFC.

@bukka
Copy link
Contributor Author

bukka commented Sep 10, 2024

@lukaaash Apology for the dealy (was busy at that time and then forgot). I just checked it and it is really a bug in the tag number as well as the type (I agree it should be X509_ATTRIBUTE for both). You are also right that this sort of failure is probably better than a bit cryptic failure in decryption that would likely happen due to logic in ossl_cms_EncryptedContent_init_bio which does not set AAD.

There's currently no API for setting authAttrs and unauthAttrs so this might be a bit harder to test. It would be good to add some point too. I have been recently doing some work on BouncyCastle and the integration testing was mostly fine but haven't checked properly so will try to produce PEM there that we could use in the test. I think as a fix, it should just add veryfication for authAttr (setting it as AAD) when decrypting and fix those tags. In the future it would be good to also add support for attrs as well as making sure that authAttrs are set as AAD in encryption but that would be more a feature.

I will try to give a try in the coming weeks see if I can also produce a fix. I will also try to create a proper issue for this as this is closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.