Skip to content

crypto: prevent Sign::SignFinal from crashing - #21815

Closed
tniessen wants to merge 2 commits into
nodejs:masterfrom
tniessen:crypto-dont-allow-sign-to-crash
Closed

crypto: prevent Sign::SignFinal from crashing#21815
tniessen wants to merge 2 commits into
nodejs:masterfrom
tniessen:crypto-dont-allow-sign-to-crash

Conversation

@tniessen

@tniessentniessen commented Jul 14, 2018

Copy link
Copy Markdown
Member

The validation logic could be tricked into assuming an option was valid using malicious getters, leading to an invalid value being passed to the C++ layer, thus crashing the process.

Copy this into the REPL to crash the process:

constkey=`-----BEGIN RSA PRIVATE KEY----- MIICXQIBAAKBgQDx3wdzpq2rvwm3Ucun1qAD/ClB+wW+RhR1nVix286QvaNqePAd CAwwLL82NqXcVQRbQ4s95splQnwvjgkFdKVXFTjPKKJI5aV3wSRN61EBVPdYpCre 535yfG/uDysZFCnVQdnCZ1tnXAR8BirxCNjHqbVyIyBGjsNoNCEPb2R35QIDAQAB AoGBAJNem9C4ftrFNGtQ2DB0Udz7uDuucepkErUy4MbFsc947GfENjDKJXr42Kx0 kYx09ImS1vUpeKpH3xiuhwqe7tm4FsCBg4TYqQle14oxxm7TNeBwwGC3OB7hiokb aAjbPZ1hAuNs6ms3Ybvvj6Lmxzx42m8O5DXCG2/f+KMvaNUhAkEA/ekrOsWkNoW9 2n3m+msdVuxeek4B87EoTOtzCXb1dybIZUVv4J48VAiM43hhZHWZck2boD/hhwjC M5NWd4oY6QJBAPPcgBVNdNZSZ8hR4ogI4nzwWrQhl9MRbqqtfOn2TK/tjMv10ALg lPmn3SaPSNRPKD2hoLbFuHFERlcS79pbCZ0CQQChX3PuIna/gDitiJ8oQLOg7xEM wk9TRiDK4kl2lnhjhe6PDpaQN4E4F0cTuwqLAoLHtrNWIcOAQvzKMrYdu1MhAkBm Et3qDMnjDAs05lGT72QeN90/mPAcASf5eTTYGahv21cb6IBxM+AnwAPpqAAsHhYR 9h13Y7uYbaOjvuF23LRhAkBoI9eaSMn+l81WXOVUHnzh3ZwB4GuTyxMXXNOhuiFd 0z4LKAMh99Z4xQmqSoEkXsfM4KPpfhYjF/bwIcP5gOei -----END RSA PRIVATE KEY-----`;crypto.createSign('SHA256').update('Test123').sign({counter: 0,getpadding(){if(++this.counter<3)returncrypto.constants.RSA_PKCS1_PSS_PADDING;returnnull;},
key
});

The validation logic could be tricked into assuming an option was
valid using malicious getters, leading to an invalid value being
passed to the C++ layer, thus crashing the process.
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-botnodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Jul 14, 2018
@tniessen

Copy link
Copy Markdown
MemberAuthor

@tniessentniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 15, 2018
tniessen added a commit to tniessen/node that referenced this pull request Jul 17, 2018
The validation logic could be tricked into assuming an option was
valid using malicious getters, leading to an invalid value being
passed to the C++ layer, thus crashing the process.
PR-URL: nodejs#21815
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@tniessen

Copy link
Copy Markdown
MemberAuthor

Landed in 43cc6bc.

targos pushed a commit that referenced this pull request Jul 18, 2018
The validation logic could be tricked into assuming an option was
valid using malicious getters, leading to an invalid value being
passed to the C++ layer, thus crashing the process.
PR-URL: #21815
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@targostargos mentioned this pull request Jul 18, 2018
@tniessentniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cryptoIssues and PRs related to the crypto subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@tniessen@nodejs-github-bot@addaleax@TimothyGu@ryzokuken