Closed ArashPartow closed 3 years ago
Could you clarify why these are not valid issues? My understanding of this library is limited but it seems that errors in evaluating a custom expression format should not leak out to memory corruption issues in the implementation. E.g. an error in caused by a javascript program should not cause a memory corruption issue in the evaluation engine itself.
I'm not sure what we can do here to distinguish between "real" bugs and cases like these, apart from manually ignoring such cases, if they are indeed invalid/not actionable.
The ExprTk fuzzer is comprised of 2 sections:
The expressions denoted above, generated by oss-fuzz, are indeed valid parsable, compilable expressions - no argument there. They pass the first stage no issues (no memory leak, UB etc)
Once the expressions have been compiled, they go to the second phase which is evaluation. This is where the issues arise:
Lets take Issue 28195, whose expression (aka testcase): 'e'[y:3427z]+'e'
This is saying, given a string 'e' (of length 1 char), return the substring in the range [y,3427 * z] and add the string 'e' to the result.
The issue here is that 'e'[y:3427z] maps directly to: s.substr(y,((3427*z) - y) + 1) where s is a std::string representing 'e' and y and z are non-zero numeric variables.
I agree that these are valid expressions - but I do not agree that the evaluation engine is responsible for the above.
Perhaps run-time checks for every memory access could be embedded in the evaluation stage but then one might as well give up on performance altogether.
I'm sure one could easily generate test cases that would create memory leaks/corruption for v8:
https://stackoverflow.com/questions/16256487/how-do-i-create-a-memory-leak-in-javascript
If such test cases happened to be generated how are they handled?
My options seem to be either:
TBH I don't like either option much. As I would very much like the evaluation pass to be fuzzed as much as possible and any legitimate issues raised to be resolved. But at the same time junking test-cases sort of invalidates the robustness of the fuzzing test suite.
So I guess manually ignoring them will have to be the path I go down. That being the case, how do I go about requesting they be ignored - Do I raise an issue?
I can't seem to change the state of the testcase from the link here: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28195
I'm sure one could easily generate test cases that would create memory leaks/corruption for v8:
Leaks maybe, but memory corruption is taken very seriously and fixed very quickly for V8 and most other JS engines. They allow exploitation and remote code execution by a malicious attacker. This is a big benefit of of fuzzing in uncovering these issues (in most cases the main point of fuzzing) and they should be taken seriously. We would strongly recommend run-time checks.
I'm not sure how you would distinguish what you consider to be a legitimate issue with the issues fuzzing is designed to find.
So I guess manually ignoring them will have to be the path I go down. That being the case, how do I go about requesting they be ignored - Do I raise an issue?
Unfortunately we can't grant access to change issue status as that's a bit too powerful to grant to generally.
@oliverchang I completely agree with all your comments and understand the necessity of the restrictions on test cases.
For the short-term I'll disable the evaluation part of the fuzzer and look to enabling it once I've added the necessary runtime checks.
I'm not sure what to do about the following test cases :
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28195 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28169
They are both syntactically valid expressions, and they (correctly) trigger the fuzzers due to invalid/bad memory accesses during the evaluation part of the test.
Note: Not saying the result is incorrect, just that exprtk can't do anything about them - like most other languages.
The options I see going forward are:
I would rather not do [2] as I'd very much like the evaluation part be fuzzed as well.
Is it possible to mark these test cases as false positives?
Any suggestions on how to proceed will be greatly appreciated.
wrt: https://github.com/google/oss-fuzz/pull/4789