-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Nodejs] initial integration. #3860
Conversation
@mikeal @bnoordhuis @ry, can you please review this. Also, can you suggest good api places to fuzz. |
Hey @DavidKorczynski, TravisCI finished with status TravisBuddy Request Identifier: 1dafddd0-9ba7-11ea-bde9-fb9aa5b18627 |
Hey @DavidKorczynski, TravisCI finished with status TravisBuddy Request Identifier: 48a18f60-9ba8-11ea-bde9-fb9aa5b18627 |
Note that the failures of the builds are due to timeouts in Travis. |
Any plans on a target taking javascript as input ? |
Yes i think those would be good targets. Still we need maintainer approval first, asking @mikeal @bnoordhuis @ry |
Yeah - I think in general my approach on Nodejs will be to cover it extensively. @inferno-chromium I have also reached out to the security team of nodejs and am waiting to hear back. Should have something soon. |
@addaleax or @jasnell could you maybe assist here? The set up in this PR is used to find the bug in nodejs/node#33640 and it would be nice to get some responds on whether the NodeJS maintainers would be happy to have fuzzing of NodeJS integrated via OSS-Fuzz? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM insofar I'm able to judge that. Thanks, David.
I'm open to extending node's build to make it easier to integrate with this project. That build.sh script looks like it could be a lot of work to maintain.
Thanks a lot - this one took quite a bit more effort!
I would be happy to assist here and can give references on the difficulties that occurred when integrating it into the OSS-Fuzz environment. Should we do it over an issue on Nodejs? |
Yes, good idea. |
@oliverchang - can you please help to review Nodejs, we are very excited to accept this project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As is this LGTM
#include "string_bytes.h" | ||
|
||
extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { | ||
node::url::URL url2((char*)data, size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this target is pretty basic. do you have plans for targets that test more core nodejs functionality?
I likely won't be able to take a detailed look at this for at least a few more days but in general: yes, I'd love to see some regular fuzz testing for Node.js |
Yes build.sh is extremely complicated right now, can you please make it a proper build config upstream (optionally if it can be tested in CI, that is super nice). Then we can merge this. |
Hey @DavidKorczynski, TravisCI finished with status TravisBuddy Request Identifier: 509ed480-a85e-11ea-8d1c-67d490be1668 |
Travis tests have failedHey @DavidKorczynski, 1st Build./infra/travis/travis_build.py
TravisBuddy Request Identifier: c8d01b10-a8b5-11ea-b23f-e753f45396de |
Hey @DavidKorczynski, TravisCI finished with status TravisBuddy Request Identifier: b36bae60-a8bf-11ea-b23f-e753f45396de |
Hey @DavidKorczynski, TravisBuddy Request Identifier: aaef5eb0-a8c1-11ea-b23f-e753f45396de |
Hey @DavidKorczynski, TravisBuddy Request Identifier: a4605380-a8d8-11ea-b23f-e753f45396de |
@inferno-chromium please check it now. The build script is now simple and clean, and I also got it to work in CI. |
Hey @DavidKorczynski, TravisBuddy Request Identifier: 9085c3b0-a8f9-11ea-b23f-e753f45396de |
Refs: google/oss-fuzz#3860 Fixes: #33724 PR-URL: #34761 Fixes: #33724 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>
Refs: google/oss-fuzz#3860 Fixes: #33724 PR-URL: #34761 Fixes: #33724 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>
Refs: google/oss-fuzz#3860 Fixes: #33724 PR-URL: #34761 Fixes: #33724 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>
Refs: google/oss-fuzz#3860 Fixes: #33724 PR-URL: #34761 Fixes: #33724 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com>
Fuzzing for nodejs core. Notice that the build time here can take quite some time ~10/15 minutes, since we need to build v8 first.
The
project.yaml
file has been here since December 2016 so I am not sure if the same information is valid anymore. The originalproject.yaml
was added by @oliverchang so perhaps you might have an idea here? I have not reached out to the security people at Nodejs as I assume if the contact information is accurate, if not then please do tell me.