Skip to content
This repository was archived by the owner on Feb 18, 2025. It is now read-only.

Fix process crash (!) when no certificate is present (can be considered a vulnerability) #108

Merged
merged 1 commit into from
Oct 24, 2020

Conversation

nikitaeverywhere
Copy link
Contributor

@nikitaeverywhere nikitaeverywhere commented Oct 24, 2020

It can be that the certificate is just not returned by the server for https connections (port 443):

image

The weirdest part is, it crashes the entire NodeJS process when using your package, i.e. all programs using this package are vulnerable:

...\node_modules\ssl-checker\lib\cmjs\index.js:59
                    validFrom: new Date(valid_from).toISOString(),
                                                    ^

RangeError: Invalid time value
    at Date.toISOString (<anonymous>)
    at ClientRequest.<anonymous> (...\node_modules\ssl-checker\lib\cmjs\index.js:59:53)
    at Object.onceWrapper (events.js:421:26)
    at ClientRequest.emit (events.js:314:20)
    at HTTPParser.parserOnIncomingClient [as onIncoming] (_http_client.js:640:27)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:119:17)
    at TLSSocket.socketOnData (_http_client.js:509:22)
    at TLSSocket.emit (events.js:314:20)
    at addChunk (_stream_readable.js:303:12)
    at readableAddChunk (_stream_readable.js:279:9)
Exit status 1

$ node -v
v14.9.0

The above error is a process-level error, and there is no way to catch it (but only globally). I suspect NodeJS is also not handling errors in the https.request callback correctly, but I can't invest too much time to investigate it, rather I am suggesting a quick fix. The root cause of the problem is .getPeerCertificate() returning {}. I also didn't find any suitable domain at https://badssl.com to write the test which reproduces it, but you can reproduce it easily by adding a throw new Error('whatever') inside the https.request's callback.

Thanks for a great package! Looking forward to seeing this fix on npm.

P.S. If you want this PR to count towards my Hacktoberfest contributions, please add a hacktoberfest-accepted label to it. Thanks!

@dyaa dyaa merged commit 4ff028a into dyaa:master Oct 24, 2020
@nikitaeverywhere
Copy link
Contributor Author

Thanks! Any chance for adding a hacktoberfest-accepted label? 😇

@nikitaeverywhere
Copy link
Contributor Author

I mean this:

image

Thanks in advance!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants