src,crypto: fix 0-length output crash in webcrypto #38913

XadillaX · 2021-06-03T06:52:50Z

src/crypto/crypto_cipher.h

tniessen · 2021-06-03T13:33:57Z

src/crypto/crypto_cipher.h

@@ -249,7 +249,7 @@ class CipherJob final : public CryptoJob<CipherTraits> {
      v8::Local<v8::Value>* result) override {
    Environment* env = AsyncWrap::env();
    CryptoErrorStore* errors = CryptoJob<CipherTraits>::errors();
-    if (out_.size() > 0) {
+    if (out_.size() > 0 || errors->Empty()) {


A few lines below, there's this:

if (errors->Empty()) errors->Capture(); // this is unreachable now CHECK(!errors->Empty());

So, either those lines are not needed, or there is no guarantee that errors->IsEmpty() will return false only if no error occurred.

If errors->IsEmpty() is true if and only if the operation succeeded, then out_.size() > 0 is not a required condition.

If errors->IsEmpty() is never true when the operation failed, then the lines below should be removed.

Should I move if (errors->Empty()) errors->Capture(); before if (out_.size() > 0)?

I think @jasnell might be the best person to ask, but that might be enough to solve the problem. It would make the sanity check CHECK(!errors->Empty()) below useless.

I've moved it above.

/cc @jasnell

tniessen

LGTM but I'd appreciate a review from @jasnell due to the error handling dilemma.

test/parallel/test-crypto-subtle-zero-length.js

XadillaX · 2021-06-15T06:42:35Z

/ping @jasnell

XadillaX · 2021-06-17T06:53:00Z

/ping @jasnell

XadillaX · 2021-06-18T04:54:38Z

/ping @jasnell

nodejs-github-bot · 2021-06-18T10:55:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38679/

nodejs-github-bot · 2021-06-20T11:17:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38701/

nodejs-github-bot · 2021-06-21T02:42:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38709/

tniessen · 2021-06-21T17:16:00Z

Related failure on ubi81_sharedlibs_openssl111fips_x64:

04:49:43 not ok 592 parallel/test-crypto-subtle-zero-length
04:49:43   ---
04:49:43   duration_ms: 0.94
04:49:43   severity: fail
04:49:43   exitcode: 1
04:49:43   stack: |-
04:49:43     node:internal/process/promises:246
04:49:43               triggerUncaughtException(err, true /* fromPromise */);
04:49:43               ^
04:49:43     
04:49:43     [Error: Cipher job failed]
04:49:43   ...

nodejs-github-bot · 2021-06-22T07:01:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38726/

XadillaX · 2021-06-22T09:44:56Z

Related failure on ubi81_sharedlibs_openssl111fips_x64:

04:49:43 not ok 592 parallel/test-crypto-subtle-zero-length
04:49:43   ---
04:49:43   duration_ms: 0.94
04:49:43   severity: fail
04:49:43   exitcode: 1
04:49:43   stack: |-
04:49:43     node:internal/process/promises:246
04:49:43               triggerUncaughtException(err, true /* fromPromise */);
04:49:43               ^
04:49:43     
04:49:43     [Error: Cipher job failed]
04:49:43   ...

How do I mock an environment like this case?

richardlau · 2021-06-22T10:54:56Z

@XadillaX see nodejs/build#2176 for the instructions that the CI build was based on.

XadillaX · 2021-06-22T13:08:41Z

src/crypto/crypto_cipher.h

      CHECK(errors->Empty());
      *err = v8::Undefined(env->isolate());
      *result = out_.ToArrayBuffer(env);
      return v8::Just(!result->IsEmpty());
    }

-    if (errors->Empty())
-      errors->Capture();


/ping @jasnell, why here should errors->Capture() again? Can't we CHECK(!errors->Empty()) directly?

XadillaX · 2021-06-23T03:06:28Z

Related failure on ubi81_sharedlibs_openssl111fips_x64:

04:49:43 not ok 592 parallel/test-crypto-subtle-zero-length
04:49:43   ---
04:49:43   duration_ms: 0.94
04:49:43   severity: fail
04:49:43   exitcode: 1
04:49:43   stack: |-
04:49:43     node:internal/process/promises:246
04:49:43               triggerUncaughtException(err, true /* fromPromise */);
04:49:43               ^
04:49:43     
04:49:43     [Error: Cipher job failed]
04:49:43   ...

After testing, I found that it's not

if (errors->Empty())
  errors->Capture();

occurs the error. It failed either with commenting those two lines.

XadillaX · 2021-06-23T04:05:54Z

Related failure on ubi81_sharedlibs_openssl111fips_x64:

04:49:43 not ok 592 parallel/test-crypto-subtle-zero-length
04:49:43   ---
04:49:43   duration_ms: 0.94
04:49:43   severity: fail
04:49:43   exitcode: 1
04:49:43   stack: |-
04:49:43     node:internal/process/promises:246
04:49:43               triggerUncaughtException(err, true /* fromPromise */);
04:49:43               ^
04:49:43     
04:49:43     [Error: Cipher job failed]
04:49:43   ...

It seems a problem with ubi81_sharedlibs_openssl111fips_x64's sharelib.

Maybe lack of code below (I guess):

    /*
     * CCM mode needs to know about the case where inl == 0 && in == NULL - it
     * means the plaintext/ciphertext length is 0
     */
    if (inl < 0
            || (inl == 0
                && EVP_CIPHER_mode(ctx->cipher) != EVP_CIPH_CCM_MODE)) {
        *outl = 0;
        return inl == 0;
    }

Refs: https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/evp/evp_enc.c#L309-L318

So I will check zero-length in Node.js' source code.

nodejs-github-bot · 2021-06-23T04:08:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38736/

XadillaX · 2021-06-23T07:06:29Z

@tniessen All CI have already passed except commit message due to several f commits. It will be rebased when landing.

richardlau · 2021-06-23T11:43:59Z

Related failure on ubi81_sharedlibs_openssl111fips_x64:

04:49:43 not ok 592 parallel/test-crypto-subtle-zero-length
04:49:43   ---
04:49:43   duration_ms: 0.94
04:49:43   severity: fail
04:49:43   exitcode: 1
04:49:43   stack: |-
04:49:43     node:internal/process/promises:246
04:49:43               triggerUncaughtException(err, true /* fromPromise */);
04:49:43               ^
04:49:43     
04:49:43     [Error: Cipher job failed]
04:49:43   ...

It seems a problem with ubi81_sharedlibs_openssl111fips_x64's sharelib.

Maybe lack of code below (I guess):

    /*
     * CCM mode needs to know about the case where inl == 0 && in == NULL - it
     * means the plaintext/ciphertext length is 0
     */
    if (inl < 0
            || (inl == 0
                && EVP_CIPHER_mode(ctx->cipher) != EVP_CIPH_CCM_MODE)) {
        *outl = 0;
        return inl == 0;
    }

Refs: https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/evp/evp_enc.c#L309-L318

So I will check zero-length in Node.js' source code.

I suspect it's because UBI 8.1 is outdated and has OpenSSL 1.1.1c (which is several versions behind the current OpenSSL 1.1.1k). I'll add updating the UBI container to UBI 8.4, which has OpenSSL 1.1.1g (which is at least more up to date) to my list of things to do.

XadillaX · 2021-06-23T11:59:58Z

Related failure on ubi81_sharedlibs_openssl111fips_x64:
04:49:43 not ok 592 parallel/test-crypto-subtle-zero-length
04:49:43   ---
04:49:43   duration_ms: 0.94
04:49:43   severity: fail
04:49:43   exitcode: 1
04:49:43   stack: |-
04:49:43     node:internal/process/promises:246
04:49:43               triggerUncaughtException(err, true /* fromPromise */);
04:49:43               ^
04:49:43     
04:49:43     [Error: Cipher job failed]
04:49:43   ...
It seems a problem with ubi81_sharedlibs_openssl111fips_x64's sharelib.
Maybe lack of code below (I guess):
    /*
     * CCM mode needs to know about the case where inl == 0 && in == NULL - it
     * means the plaintext/ciphertext length is 0
     */
    if (inl < 0
            || (inl == 0
                && EVP_CIPHER_mode(ctx->cipher) != EVP_CIPH_CCM_MODE)) {
        *outl = 0;
        return inl == 0;
    }
Refs: https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/evp/evp_enc.c#L309-L318

So I will check zero-length in Node.js' source code.
I suspect it's because UBI 8.1 is outdated and has OpenSSL 1.1.1c (which is several versions behind the current OpenSSL 1.1.1k). I'll add updating the UBI container to UBI 8.4, which has OpenSSL 1.1.1g (which is at least more up to date) to my list of things to do.

I think it's really a problem. We can make sure that our UBI 8.4 build container use the right openssl sharedlib, but we can't make sure users use the right one.

richardlau · 2021-06-23T12:03:26Z

src/crypto/crypto_aes.cc

+  // Only `ubi81_sharedlibs_openssl111fips_x64` failed when `in.size()` is zero.
+  // So we regard 0 as special and DO NOT go into `EVP_CipherUpdate()` logic
+  // because it will occur failure under `ubi81_sharedlibs_openssl111fips_x64`.
+  //
+  // Refs: https://github.com/nodejs/node/pull/38913#issuecomment-866505244


I don't know if it's possible to work out which version of OpenSSL 1.1.1 changed the behavior without going through and build/testing against OpenSSL 1.1.1c, 1.1.1d, 1.1.1e etc. but the comment would be a lot better if we could refer to an OpenSSL release rather than a container specific to our build CI.

I've updated the comment and you may review again.

@richardlau btw, I think we should leave at least one outdated OpenSSL CI to discover similar problems.

nodejs-github-bot · 2021-06-24T03:41:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38748/

richardlau · 2021-06-24T13:11:44Z

src/crypto/crypto_aes.cc

+  // it up. But we still have to regard zero as special in Node.js code to
+  // prevent old OpenSSL failure.
+  //
+  // Refs: https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubi81_sharedlibs_openssl111fips_x64/27534/console


This link will expire -- build logs are only kept for around two weeks or so.

Fixes: nodejs#38883

nodejs-github-bot · 2021-06-25T02:24:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38758/

github-actions · 2021-06-26T14:21:52Z

Landed in b3d4a2c...8954e03

Fixes: #38883 PR-URL: #38913 Refs: #38883 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

github-actions 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. needs-ci PRs that need a full CI run. labels Jun 3, 2021

XadillaX linked an issue Jun 3, 2021 that may be closed by this pull request

Decrypting a zero-length array with SubtleCrypto triggers Assertion failures #38883

Closed

mscdex reviewed Jun 3, 2021

View reviewed changes

src/crypto/crypto_cipher.h Outdated Show resolved Hide resolved

tniessen requested changes Jun 3, 2021

View reviewed changes

tniessen approved these changes Jun 10, 2021

View reviewed changes

test/parallel/test-crypto-subtle-zero-length.js Outdated Show resolved Hide resolved

XadillaX requested a review from jasnell June 15, 2021 06:42

nodejs deleted a comment from nodejs-github-bot Jun 22, 2021

XadillaX commented Jun 22, 2021

View reviewed changes

XadillaX force-pushed the cipher_zero_length branch from 7d4b493 to 46d0c55 Compare June 22, 2021 14:09

XadillaX requested a review from mscdex June 23, 2021 07:07

richardlau mentioned this pull request Jun 23, 2021

Update UBI 8 containers nodejs/build#2689

Closed

richardlau reviewed Jun 23, 2021

View reviewed changes

XadillaX force-pushed the cipher_zero_length branch from 96b9f19 to 7eb1d04 Compare June 24, 2021 03:37

XadillaX added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 24, 2021

XadillaX requested a review from richardlau June 24, 2021 05:29

richardlau reviewed Jun 24, 2021

View reviewed changes

src,crypto: fix 0-length output crash in webcrypto

4986fb9

Fixes: nodejs#38883

XadillaX force-pushed the cipher_zero_length branch from dcf45a1 to 4986fb9 Compare June 25, 2021 02:23

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 26, 2021

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 26, 2021

github-actions bot closed this Jun 26, 2021

nodejs-github-bot pushed a commit that referenced this pull request Jun 26, 2021

src,crypto: fix 0-length output crash in webcrypto

8954e03

Fixes: #38883 PR-URL: #38913 Refs: #38883 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

targos pushed a commit that referenced this pull request Jul 11, 2021

src,crypto: fix 0-length output crash in webcrypto

ea8d83b

Fixes: #38883 PR-URL: #38913 Refs: #38883 Reviewed-By: Tobias Nießen <tniessen@tnie.de>

targos mentioned this pull request Jul 13, 2021

v16.5.0 release proposal #39373

Merged

targos added the dont-land-on-v14.x label Sep 4, 2021

bnoordhuis mentioned this pull request May 10, 2022

Crypto issues on Ubuntu 22.04 - Assertion `errors->Empty()' failed #42919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src,crypto: fix 0-length output crash in webcrypto #38913

src,crypto: fix 0-length output crash in webcrypto #38913

XadillaX commented Jun 3, 2021

tniessen Jun 3, 2021

XadillaX Jun 4, 2021

tniessen Jun 6, 2021

XadillaX Jun 10, 2021

tniessen left a comment

XadillaX commented Jun 15, 2021

XadillaX commented Jun 17, 2021

XadillaX commented Jun 18, 2021

nodejs-github-bot commented Jun 18, 2021

nodejs-github-bot commented Jun 20, 2021

nodejs-github-bot commented Jun 21, 2021

tniessen commented Jun 21, 2021

nodejs-github-bot commented Jun 22, 2021

XadillaX commented Jun 22, 2021

richardlau commented Jun 22, 2021

XadillaX Jun 22, 2021

XadillaX commented Jun 23, 2021

XadillaX commented Jun 23, 2021

nodejs-github-bot commented Jun 23, 2021

XadillaX commented Jun 23, 2021

richardlau commented Jun 23, 2021

XadillaX commented Jun 23, 2021

richardlau Jun 23, 2021

XadillaX Jun 24, 2021

XadillaX Jun 24, 2021

nodejs-github-bot commented Jun 24, 2021

richardlau Jun 24, 2021

XadillaX Jun 25, 2021

nodejs-github-bot commented Jun 25, 2021

github-actions bot commented Jun 26, 2021

src,crypto: fix 0-length output crash in webcrypto #38913

src,crypto: fix 0-length output crash in webcrypto #38913

Conversation

XadillaX commented Jun 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen left a comment

Choose a reason for hiding this comment

XadillaX commented Jun 15, 2021

XadillaX commented Jun 17, 2021

XadillaX commented Jun 18, 2021

nodejs-github-bot commented Jun 18, 2021

nodejs-github-bot commented Jun 20, 2021

nodejs-github-bot commented Jun 21, 2021

tniessen commented Jun 21, 2021

nodejs-github-bot commented Jun 22, 2021

XadillaX commented Jun 22, 2021

richardlau commented Jun 22, 2021

Choose a reason for hiding this comment

XadillaX commented Jun 23, 2021

XadillaX commented Jun 23, 2021

nodejs-github-bot commented Jun 23, 2021

XadillaX commented Jun 23, 2021

richardlau commented Jun 23, 2021

XadillaX commented Jun 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jun 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jun 25, 2021

github-actions bot commented Jun 26, 2021