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

crypto: add test case for AES key wrapping #20587

Closed
wants to merge 1 commit into from

Conversation

yhwang
Copy link
Member

@yhwang yhwang commented May 8, 2018

Add test cases for AES key wrapping and only detect output length in
cipher case. The reason being is the returned output length is
insufficient in AES key unwrapping case.

Signed-off-by: Yihong Wang yh.wang@ibm.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Add test cases for AES key wrapping and only detect output length in
cipher case. The reason being is the returned output length is
insufficient in AES key unwrapping case.

Signed-off-by: Yihong Wang <yh.wang@ibm.com>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels May 8, 2018
@yhwang
Copy link
Member Author

yhwang commented May 8, 2018

I tried to use the same mechanism to detect the output length for the AES key unwrapping and I found it may be a glitch in openssl. In the AES key unwrapping code path, it returns ciphertext length - 8 as output lenght. But when it performs the unwrapping, it expect the output length should be equal to ciphertext length. So I skip the output length detection in decipher case.

You can see CRYPTO_128_unwrap_pad() in wrap128.c. Inside CRYPTO_128_unwrap_pad(), it says the minimal buffer length of out should be equal to inlen. But in aes_wrap_cipher(), the padding decipher case, it says minimal buffer length of out is inlen - 8.

I have a patch for wrap128.c here:

--- a/deps/openssl/openssl/crypto/modes/wrap128.c
+++ b/deps/openssl/openssl/crypto/modes/wrap128.c
@@ -237,7 +237,7 @@ size_t CRYPTO_128_wrap_pad(void *key, const unsigned char *icv,
  *
  *  @param[in]  key    Key value.
  *  @param[in]  icv    (Non-standard) IV, 4 bytes. NULL = use default_aiv.
- *  @param[out] out    Plaintext. Minimal buffer length = inlen bytes.
+ *  @param[out] out    Plaintext. Minimal buffer length = inlen - 8 bytes.
  *                     Input and output buffers can overlap if block function
  *                     supports that.
  *  @param[in]  in     Ciphertext as n 64-bit blocks.
@@ -267,7 +267,6 @@ size_t CRYPTO_128_unwrap_pad(void *key, const unsigned char *icv,
     if ((inlen & 0x7) != 0 || inlen < 16 || inlen >= CRYPTO128_WRAP_MAX)
         return 0;
 
-    memmove(out, in, inlen);
     if (inlen == 16) {
         /*
          * Section 4.2 - special case in step 1: When n=1, the ciphertext
@@ -275,14 +274,15 @@ size_t CRYPTO_128_unwrap_pad(void *key, const unsigned char *icv,
          * single AES block using AES in ECB mode: AIV | P[1] = DEC(K, C[0] |
          * C[1])
          */
+        memmove(out, in + 8, inlen - 8);
         block(out, out, key);
-        memcpy(aiv, out, 8);
+        memcpy(aiv, in, 8);
         /* Remove AIV */
         memmove(out, out + 8, 8);
         padded_len = 8;
     } else {
         padded_len = inlen - 8;
-        ret = crypto_128_unwrap_raw(key, aiv, out, out, inlen, block);
+        ret = crypto_128_unwrap_raw(key, aiv, out, in, inlen, block);
         if (padded_len != ret) {
             OPENSSL_cleanse(out, inlen);
             return 0;

But even if we patch wrap128.c, for openssl shared lib user, we still need to skip the output length detection.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM. Were you planning to bring this up with the upstream openssl project as well?

@@ -2998,7 +2998,7 @@ CipherBase::UpdateResult CipherBase::Update(const char* data,
int buff_len = len + EVP_CIPHER_CTX_block_size(ctx_.get());
// For key wrapping algorithms, get output size by calling
// EVP_CipherUpdate() with null output.
if (mode == EVP_CIPH_WRAP_MODE &&
if (kind_ == kCipher && mode == EVP_CIPH_WRAP_MODE &&
Copy link
Member

Choose a reason for hiding this comment

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

Is this change intended? If so, this likely should be a different commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell this is needed. Sorry the title of this commit is a little misleading but I explained that I need the code change in the description

@yhwang
Copy link
Member Author

yhwang commented May 8, 2018

@bnoordhuis I will try it

@yhwang yhwang added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 9, 2018
@targos
Copy link
Member

targos commented May 12, 2018

@targos
Copy link
Member

targos commented May 12, 2018

Landed in e3ceae7

@targos targos closed this May 12, 2018
targos pushed a commit that referenced this pull request May 12, 2018
Add test cases for AES key wrapping and only detect output length in
cipher case. The reason being is the returned output length is
insufficient in AES key unwrapping case.

PR-URL: #20587
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
tniessen pushed a commit to tniessen/node that referenced this pull request May 13, 2018
Add test cases for AES key wrapping and only detect output length in
cipher case. The reason being is the returned output length is
insufficient in AES key unwrapping case.

PR-URL: nodejs#20587
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
addaleax pushed a commit that referenced this pull request May 14, 2018
Add test cases for AES key wrapping and only detect output length in
cipher case. The reason being is the returned output length is
insufficient in AES key unwrapping case.

Backport-PR-URL: #20706
PR-URL: #20587
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@addaleax addaleax mentioned this pull request May 14, 2018
@targos targos added backported-to-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels May 14, 2018
@yhwang yhwang deleted the add-aes-keywrap-test branch May 16, 2018 05:35
@yhwang
Copy link
Member Author

yhwang commented May 31, 2018

@bnoordhuis the related change is upstreamed to OpenSSL: openssl/openssl#6266

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants