Destroy socket on failure to CONNECT (avoids TLS risk) #76
Conversation
Signed-off-by: Kris Adler <spurfan15@gmail.com>
kadler15
changed the title
kadler15
changed the title
|
@TooTallNate Nate, hello there! Nice to GitHub meet you. Do you think you'll be accepting or considering this PR for acceptance? If not, do you think you'll be taking any steps to address the security bulletin that indicated there's a potential security bug in this repo? https://hackerone.com/reports/541502 Asking because a lot of the downstream security products have picked up this report, which in turn is flagging our package (which uses No worries if not -- I know how unpaid open source work can go -- but knowing what the plans are would help us decide what we need to do for our own package(s). |
This is a fix for https://hackerone.com/reports/541502. Aborts the upstream proxy connection and instead uses a vanilla `EventEmitter` instance to replay the "data" events on to. This way, the node core `http` Client doesn't attempt to write the HTTP request that is intended to go to the destination server to the proxy server. Closes #76.
|
Hey @kadler15 @astormnewrelic. Thanks for reaching out. I have proposed an alternative solution in #77, which doesn't end up altering the API (the replay still happens, but it's "synthetic" now). Please take a look and let me know if that adequately fixes the vulnerability. Cheers! |
|
Nice! Thank you @TooTallNate -- as to whether #77 is an appropriate fix or not, I don't know. I'm just a programmer at the whim of security products. @kadler15 is seems like you're familiar with the |
This is a fix for https://hackerone.com/reports/541502. Aborts the upstream proxy connection and instead uses a vanilla `EventEmitter` instance to replay the "data" events on to. This way, the node core `http` Client doesn't attempt to write the HTTP request that is intended to go to the destination server to the proxy server. Closes #76.
|
@astormnewrelic I'll double check these changes today. @TooTallNate Your changes seem like a great way to preserve the current API while closing the vulnerability. Nice work! |
|
Closing in favor of #77 |
* Use an `EventEmitter` to replay failed proxy connect HTTP requests This is a fix for https://hackerone.com/reports/541502. Aborts the upstream proxy connection and instead uses a vanilla `EventEmitter` instance to replay the "data" events on to. This way, the node core `http` Client doesn't attempt to write the HTTP request that is intended to go to the destination server to the proxy server. Closes #76. * Adjust comment
Vulnerability reference: https://hackerone.com/reports/541502
Unfortunately, this results in the loss of proxy error playback for client-side error handling added in ae03c68
Other parts of the proxy error playback machinery can be removed in the future.