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

Update crypto.md to correct function description for decipher.setAAD #33095

Closed
wants to merge 3 commits into from

Conversation

jbuhacoff
Copy link
Contributor

@jbuhacoff jbuhacoff commented Apr 27, 2020

According to the NodeJS CCM example, when decrypting the plaintextLength parameter actually refers to the ciphertext length, not the plaintext length:

decipher.setAAD(aad, {
  plaintextLength: ciphertext.length
});

The same can be seen in the OpenSSL docs where a call to EVP_DecryptUpdate passes the ciphertext length:

/* Provide the total ciphertext length */
    if(1 != EVP_DecryptUpdate(ctx, NULL, &len, NULL, ciphertext_len))
        handleErrors();

This parameter probably should have been called inputLength or bufferLength instead of plaintextLength, so that it makes sense both when encrypting and decrypting, but at least we can correct the sentence in the documentation for now to refer to the correct value.

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

According to the [NodeJS CCM example](https://nodejs.org/docs/latest-v14.x/api/crypto.html#crypto_ccm_mode], when decrypting the `plaintextLength` parameter actually refers to the ciphertext length, not the plaintext length:

```
decipher.setAAD(aad, {
  plaintextLength: ciphertext.length
});
```


The same can be seen in the [OpenSSL docs](https://wiki.openssl.org/index.php/EVP_Authenticated_Encryption_and_Decryption) where a call to `EVP_DecryptUpdate` passes the ciphertext length:

```
/* Provide the total ciphertext length */
    if(1 != EVP_DecryptUpdate(ctx, NULL, &len, NULL, ciphertext_len))
        handleErrors();
```

This parameter probably should have been called `inputLength` or `bufferLength` instead of `plaintextLength`, so that it makes sense both when encrypting and decrypting, but at least we can correct the sentence in the documentation for now to refer to the correct value.
@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Apr 27, 2020
@addaleax
Copy link
Member

@nodejs/crypto

@tniessen
Copy link
Member

I am not sure if the PR in its current state is an improvement.

when decrypting the plaintextLength parameter actually refers to the ciphertext length, not the plaintext length

The plaintextLength option only applies to CCM ciphers. The ciphertext length produced by CCM ciphers is always equal to the plaintext length. So there is really no difference between "plaintext length" and "ciphertext length" (except for the caveat below).

This parameter probably should have been called inputLength or bufferLength instead of plaintextLength

When I implemented this, I called the option plaintextLength to avoid confusion as to whether it includes the length of the authentication tag. Many crypto libraries include the authentication tag in the ciphertext, which means that they produce ciphertexts of the length plaintextLength + authTagLength. Node.js does not include the authentication tag, so the ciphertext length is always plaintextLength.


I think a better approach would be to explain that the ciphertext length is equal to the plaintext length.

@jbuhacoff
Copy link
Contributor Author

That's a great explanation, it would be helpful to add it to the documentation. Since there is already a link to the CCM Mode section, how about adding it there?

Add explanation in [CCM mode](https://nodejs.org/docs/latest-v14.x/api/crypto.html#crypto_ccm_mode], that ciphertext length is equal to plaintext length in Node.js crypto output because the authentication tag is returned separately:

```
decipher.setAAD(aad, {
  plaintextLength: ciphertext.length
});
```

The same can be seen in the [OpenSSL docs](https://wiki.openssl.org/index.php/EVP_Authenticated_Encryption_and_Decryption) where a call to `EVP_DecryptUpdate` passes the ciphertext length:

```
/* Provide the total ciphertext length */
    if(1 != EVP_DecryptUpdate(ctx, NULL, &len, NULL, ciphertext_len))
        handleErrors();
```
@addaleax
Copy link
Member

addaleax commented May 7, 2020

@tniessen Do you want to take another look here?

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

The entire paragraph is a verbatim quote from me, so I guess I'll have to approve it :)

@addaleax
Copy link
Member

Landed in d093e78 🎉 Thanks for the PR again!

@addaleax addaleax closed this May 19, 2020
addaleax pushed a commit that referenced this pull request May 19, 2020
According to the
[NodeJS CCM example](https://nodejs.org/docs/latest-v14.x/api/crypto.html#crypto_ccm_mode],
when decrypting the `plaintextLength` parameter actually refers to the
ciphertext length, not the plaintext length:

```
decipher.setAAD(aad, {
  plaintextLength: ciphertext.length
});
```

The same can be seen in the
[OpenSSL docs](https://wiki.openssl.org/index.php/EVP_Authenticated_Encryption_and_Decryption)
where a call to `EVP_DecryptUpdate` passes the ciphertext length:

```
/* Provide the total ciphertext length */
    if(1 != EVP_DecryptUpdate(ctx, NULL, &len, NULL, ciphertext_len))
        handleErrors();
```

This parameter probably should have been called `inputLength` or
`bufferLength` instead of `plaintextLength`, so that it makes sense
both when encrypting and decrypting, but at least we can correct the
sentence in the documentation for now to refer to the correct value.

PR-URL: #33095
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
codebytere pushed a commit that referenced this pull request May 21, 2020
According to the
[NodeJS CCM example](https://nodejs.org/docs/latest-v14.x/api/crypto.html#crypto_ccm_mode],
when decrypting the `plaintextLength` parameter actually refers to the
ciphertext length, not the plaintext length:

```
decipher.setAAD(aad, {
  plaintextLength: ciphertext.length
});
```

The same can be seen in the
[OpenSSL docs](https://wiki.openssl.org/index.php/EVP_Authenticated_Encryption_and_Decryption)
where a call to `EVP_DecryptUpdate` passes the ciphertext length:

```
/* Provide the total ciphertext length */
    if(1 != EVP_DecryptUpdate(ctx, NULL, &len, NULL, ciphertext_len))
        handleErrors();
```

This parameter probably should have been called `inputLength` or
`bufferLength` instead of `plaintextLength`, so that it makes sense
both when encrypting and decrypting, but at least we can correct the
sentence in the documentation for now to refer to the correct value.

PR-URL: #33095
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
codebytere pushed a commit that referenced this pull request Jun 7, 2020
According to the
[NodeJS CCM example](https://nodejs.org/docs/latest-v14.x/api/crypto.html#crypto_ccm_mode],
when decrypting the `plaintextLength` parameter actually refers to the
ciphertext length, not the plaintext length:

```
decipher.setAAD(aad, {
  plaintextLength: ciphertext.length
});
```

The same can be seen in the
[OpenSSL docs](https://wiki.openssl.org/index.php/EVP_Authenticated_Encryption_and_Decryption)
where a call to `EVP_DecryptUpdate` passes the ciphertext length:

```
/* Provide the total ciphertext length */
    if(1 != EVP_DecryptUpdate(ctx, NULL, &len, NULL, ciphertext_len))
        handleErrors();
```

This parameter probably should have been called `inputLength` or
`bufferLength` instead of `plaintextLength`, so that it makes sense
both when encrypting and decrypting, but at least we can correct the
sentence in the documentation for now to refer to the correct value.

PR-URL: #33095
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
codebytere pushed a commit to codebytere/node that referenced this pull request Jun 9, 2020
According to the
[NodeJS CCM example](https://nodejs.org/docs/latest-v14.x/api/crypto.html#crypto_ccm_mode],
when decrypting the `plaintextLength` parameter actually refers to the
ciphertext length, not the plaintext length:

```
decipher.setAAD(aad, {
  plaintextLength: ciphertext.length
});
```

The same can be seen in the
[OpenSSL docs](https://wiki.openssl.org/index.php/EVP_Authenticated_Encryption_and_Decryption)
where a call to `EVP_DecryptUpdate` passes the ciphertext length:

```
/* Provide the total ciphertext length */
    if(1 != EVP_DecryptUpdate(ctx, NULL, &len, NULL, ciphertext_len))
        handleErrors();
```

This parameter probably should have been called `inputLength` or
`bufferLength` instead of `plaintextLength`, so that it makes sense
both when encrypting and decrypting, but at least we can correct the
sentence in the documentation for now to refer to the correct value.

PR-URL: nodejs#33095
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@codebytere codebytere mentioned this pull request Jun 9, 2020
@jbuhacoff jbuhacoff deleted the patch-1 branch June 26, 2020 22:03
@codebytere codebytere mentioned this pull request Jun 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants