crypto: trim input for NETSCAPE_SPKI_b64_decode - #40757
Closed
codebytere wants to merge 1 commit into
Closed
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
jasnell
approved these changes
Nov 8, 2021
codebytere
force-pushed
the
fix-certificate-trimming
branch
from
de356ca to
0a9df6a
Compare
November 8, 2021 14:52
4 tasks
cjihrig
approved these changes
Nov 8, 2021
tniessen
reviewed
Nov 8, 2021
tniessen
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
For posterity, the issue arises from the fact that, within NETSCAPE_SPKI_b64_decode,
- OpenSSL uses EVP_DecodeBlock, which explicitly removes trailing whitespace etc.,
- BoringSSL uses EVP_DecodedLength and EVP_DecodeBase64, which do not remove trailing whitespace etc.
Personally, I don't think adapting the way we read keys to be compatible with BoringSSL's implementation details is the best solution.
Also, the reason for this change and for trimming the keys is difficult if not impossible to determine without going through the git history.
Lastly, if the behavior of OpenSSL or BoringSSL ever changes to match that of the other library, we won't notice if we always trim the keys in our tests (but real applications don't).
I'd personally prefer to
- either adapt the specific test to trim the strings only if the crypto library is BoringSSL, or
- change the implementation in src to trim the input if the crypto library is BoringSSL.
Whichever option we go with, the reason for the trimming should be explained in the code.
Sorry, something went wrong.
Copy link
Copy Markdown
Member
|
(Or, if BoringSSL is closer to being "correct" than OpenSSL, then we should adopt BoringSSL's behavior.) |
Sorry, something went wrong.
JungMinu
approved these changes
Nov 9, 2021
Trott
reviewed
Nov 10, 2021
Comment thread
test/common/fixtures.js
Outdated
Comment thread
test/parallel/test-crypto-certificate.js
Outdated
Comment thread
test/parallel/test-crypto-certificate.js
Outdated
codebytere
force-pushed
the
fix-certificate-trimming
branch
from
0a9df6a to
7335d4f
Compare
November 15, 2021 10:38
Copy link
Copy Markdown
Member
Author
|
@tniessen updated with the second approach you suggested! |
Sorry, something went wrong.
codebytere
force-pushed
the
fix-certificate-trimming
branch
from
7335d4f to
cdf0cd4
Compare
November 15, 2021 10:40
tniessen
approved these changes
Nov 15, 2021
tniessen
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
@tniessen updated with the second approach you suggested!
Thank you @codebytere, I think this is a better solution overall 😃
(FWIW, I believe that EVP_DecodeBlock will also refuse inputs that start with whitespace, but if that never occurs in practice, there is no reason to "fix" that.)
Sorry, something went wrong.
richardlau
approved these changes
Nov 15, 2021
40 tasks
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
39 tasks
Trott
approved these changes
Nov 17, 2021
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
codebytere
added a commit
that referenced
this pull request
Nov 17, 2021
PR-URL: #40757 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced Nov 18, 2021
targos
pushed a commit
that referenced
this pull request
Nov 21, 2021
PR-URL: #40757 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams
pushed a commit
that referenced
this pull request
Jan 30, 2022
PR-URL: #40757 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams
pushed a commit
that referenced
this pull request
Feb 1, 2022
PR-URL: #40757 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Merged
codebytere
added a commit
to electron/electron
that referenced
this pull request
Feb 10, 2022
codebytere
added a commit
to electron/electron
that referenced
this pull request
Mar 9, 2022
codebytere
added a commit
to electron/electron
that referenced
this pull request
Mar 17, 2022
jkleinsc
pushed a commit
to electron/electron
that referenced
this pull request
Mar 23, 2022
* chore: bump node in DEPS to v16.14.0 * src: add flags for controlling process behavior nodejs/node#40339 * src: add x509.fingerprint512 to crypto module nodejs/node#39809 * deps: upgrade to libuv 1.43.0 nodejs/node#41398 * chore: fixup patch indices * chore: add missing filenames nodejs/node#39283 nodejs/node#40665 * crypto: trim input for NETSCAPE_SPKI_b64_decode nodejs/node#40757 * chore: update patches * chore: bump node in DEPS to v16.14.1 * tools: enable no-empty ESLint rule nodejs/node#41831 * chore: update patches * chore: update patches * chore: bump node in DEPS to v16.14.2 * chore: update patches Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
bavulapati
pushed a commit
to bavulapati/electron
that referenced
this pull request
Apr 29, 2022
* chore: bump node in DEPS to v16.14.0 * src: add flags for controlling process behavior nodejs/node#40339 * src: add x509.fingerprint512 to crypto module nodejs/node#39809 * deps: upgrade to libuv 1.43.0 nodejs/node#41398 * chore: fixup patch indices * chore: add missing filenames nodejs/node#39283 nodejs/node#40665 * crypto: trim input for NETSCAPE_SPKI_b64_decode nodejs/node#40757 * chore: update patches * chore: bump node in DEPS to v16.14.1 * tools: enable no-empty ESLint rule nodejs/node#41831 * chore: update patches * chore: update patches * chore: bump node in DEPS to v16.14.2 * chore: update patches Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
khalwa
pushed a commit
to solarwindscloud/electron
that referenced
this pull request
Feb 22, 2023
* chore: bump node in DEPS to v16.14.0 * src: add flags for controlling process behavior nodejs/node#40339 * src: add x509.fingerprint512 to crypto module nodejs/node#39809 * deps: upgrade to libuv 1.43.0 nodejs/node#41398 * chore: fixup patch indices * chore: add missing filenames nodejs/node#39283 nodejs/node#40665 * crypto: trim input for NETSCAPE_SPKI_b64_decode nodejs/node#40757 * chore: update patches * chore: bump node in DEPS to v16.14.1 * tools: enable no-empty ESLint rule nodejs/node#41831 * chore: update patches * chore: update patches * chore: bump node in DEPS to v16.14.2 * chore: update patches Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds exta handling for spkac decoding if we are using OpenSSL instead of BoringSSL.
In our smoke tests, Electron was finding a consistent failure in parallel/test-crypto-certificate. In tracking this down, I found that the issue originated with a strict requirement in BoringSSL that base64 strings be evenly divisible by 4 in their implementation of NETSCAPE_SPKI_b64_decode - a requirement which Node.js' cert tests were mysteriously failing. It turned out that the cause of the failure was that the fixture files were being read and their newlines left intact, so the number of characters was always a single character over the actual number of characters in the .spkac file.
This fixes that issue by trimming the newlines out of the file cc @jasnell