Skip to content
This repository has been archived by the owner on Apr 7, 2020. It is now read-only.

FIX - updated grpc due to security vuln #100

Merged
merged 7 commits into from Oct 23, 2018
Merged

FIX - updated grpc due to security vuln #100

merged 7 commits into from Oct 23, 2018

Conversation

dannypaz
Copy link
Contributor

@dannypaz dannypaz commented Oct 17, 2018

We've received a security notice for lnd-engine that asks us to upgrade grpc to a version > 1.12.x. This PR updates grpc to latest to relieve this notice.

Vulnerability: our current version of node-grpc depends on protobufjs < 5.0.3 which has a DoS security issue. The security issue is a RegEx DoS attack where the evaluation of a certain string takes a long amount of time (and can effectively jam the process using protobufjs)

More info on ReDos: https://www.owasp.org/index.php/Regular_expression_Denial_of_Service_-_ReDoS
Fix in protobufjs protobufjs/protobuf.js#1030

https://www.npmjs.com/advisories/605

screen shot 2018-10-17 at 12 15 41 am

@dannypaz
Copy link
Contributor Author

Copy link
Member

@treygriffith treygriffith left a comment

Choose a reason for hiding this comment

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

lgtm

*
* @global
* @constant
* @type {Object}
* @default
*/
const GRPC_OPTIONS = {
convertFieldsToCamelCase: true,
binaryAsBase64: true,
Copy link
Member

Choose a reason for hiding this comment

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

does grpc-loader not support this, or is it default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to set bytes: String in the newer options. Good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to set bytes: String in the newer options. Good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to set bytes: String in the newer options. Good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Github is back up

*
* @global
* @constant
* @type {Object}
* @default
*/
const GRPC_OPTIONS = {
convertFieldsToCamelCase: true,
Copy link
Member

Choose a reason for hiding this comment

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

does grpc-loader not support this, or is it default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This happens by default. more info here: https://www.npmjs.com/package/@grpc/proto-loader

@dannypaz
Copy link
Contributor Author

@treygriffith ive made the change for bytes.

Copy link
Contributor

@martineehrlich martineehrlich left a comment

Choose a reason for hiding this comment

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

This LGTM

Copy link
Member

@treygriffith treygriffith left a comment

Choose a reason for hiding this comment

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

LGTM

@dannypaz dannypaz merged commit f3b9df7 into master Oct 23, 2018
@dannypaz dannypaz deleted the fix/grpc-upgrade branch October 23, 2018 21:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants