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

Add SM4 XTS implementation to providers #19619

Closed
wants to merge 4 commits into from

Conversation

xu-yi-zhou
Copy link
Contributor

@xu-yi-zhou xu-yi-zhou commented Nov 7, 2022

Add SM4 XTS implementation to providers

XTS mode has two implementations, one is standardized in IEEE Std. 1619-2007 and has been widely used (e.g., XTS AES), the other is proposed recently (GB/T 17964-2021 implemented in May 2022) and is currently only used in SM4.

The main difference between them is the multiplication by the primitive element α to calculate the tweak values. The IEEE Std 1619-2007 noted that the multiplication "is a left shift of each byte by one bit with carry propagating from one byte to the next one", which means that in each byte, the leftmost bit is the most significant bit. But in GB/T 17964-2021, the rightmost bit is the most significant bit, thus the multiplication becomes a right shift of each byte by one bit with carry propagating from one byte to the next one.

The default value is 0, XTS mode of the SM4 algorithm is specified by GB/T 17964-2021. Set the parameter to 1 to use the IEEE Std. 1619-2007 variant of XTS-SM4.

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

Add the following OID:

SM4-XTS: 1.2.156.10197.1.104.10
util/libcrypto.num Outdated Show resolved Hide resolved
include/openssl/modes.h Outdated Show resolved Hide resolved
@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Nov 7, 2022
@hlandau hlandau added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Nov 7, 2022
include/crypto/modes.h Outdated Show resolved Hide resolved
util/missingcrypto.txt Outdated Show resolved Hide resolved
include/crypto/modes.h Outdated Show resolved Hide resolved
doc/man3/EVP_EncryptInit.pod Outdated Show resolved Hide resolved
doc/man3/EVP_EncryptInit.pod Outdated Show resolved Hide resolved
@t8m t8m added the tests: present The PR has suitable tests present label Nov 10, 2022
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Nov 11, 2022
@xu-yi-zhou
Copy link
Contributor Author

xu-yi-zhou commented Nov 16, 2022

Ping for second approval @hlandau @paulidale @mattcaswell

Copy link
Member

@hlandau hlandau left a comment

Choose a reason for hiding this comment

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

One nit, otherwise looks OK.

crypto/modes/xts128gb.c Outdated Show resolved Hide resolved

Title = SM4 XTS test vectors, while the XTS mode is standardized in IEEE Std 1619-2007

Cipher = SM4-XTS
Copy link
Member

Choose a reason for hiding this comment

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

Where does this test vector come from? IEEE?

Copy link
Member

Choose a reason for hiding this comment

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

It seems IEEE has no XTS test vector defined for SM4...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Ciphertext in this test vector is not from any standard.

We encrypted the test vector using GmSSL and got this Ciphertext.

@InfoHunter
Copy link
Member

From my understandings, without considering the compatibility with other SM4-XTS implementations (say, GmSSL for instance), OpenSSL doesn't need to support IEEE version of SM4 XTS at all, since the only standard specifying SM4-XTS is GB 17964.

But before this GB 17964 has been published in May, some IEEE version fo SM4 XTS encrypted data are somehow already out there, so OpenSSL needs to have the ability to decrypt that bundle of data as well. This is root cause of why we need the parameter and set it to 'gb' by default, correct me if I understand wrong...

@xu-yi-zhou
Copy link
Contributor Author

Hi @t8m, I've changed the xts_standard parameter to an utf8 string value, please review it.

providers/implementations/ciphers/cipher_sm4_xts.c Outdated Show resolved Hide resolved
providers/implementations/ciphers/cipher_sm4_xts.c Outdated Show resolved Hide resolved
@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 Nov 28, 2022
@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

@t8m
Copy link
Member

t8m commented Nov 29, 2022

Merged to master branch. Thank you for your contribution.

@t8m t8m closed this Nov 29, 2022
@t8m t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Nov 29, 2022
openssl-machine pushed a commit that referenced this pull request Nov 29, 2022
Add the following OID:

SM4-XTS: 1.2.156.10197.1.104.10

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19619)
openssl-machine pushed a commit that referenced this pull request Nov 29, 2022
Signed-off-by: Xu Yizhou <xuyizhou1@huawei.com>

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19619)
openssl-machine pushed a commit that referenced this pull request Nov 29, 2022
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19619)
openssl-machine pushed a commit that referenced this pull request Nov 29, 2022
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19619)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Add the following OID:

SM4-XTS: 1.2.156.10197.1.104.10

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19619)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Signed-off-by: Xu Yizhou <xuyizhou1@huawei.com>

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19619)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19619)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19619)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants