-
Notifications
You must be signed in to change notification settings - Fork 246
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
nested ncr() calls can be very slow (potential denial of service) #62
Comments
Thanks for the issue. I don't think it's possible to add a timeout option (without bringing in external dependencies, which I won't do), so I'll probably address this in two ways:
|
Well, it would be possible to add a timeout, albeit not in a very nice way. You could make a The balance between performance and accuracy would be super hard to get right, but it would be a useful feature. |
The cause seems to be Printing the values with
Seeing, that the project is now using c99, |
@codeplea it would be great to see this bug finally fixed via @juntuu 's #82 after 4+ years. This will also help with @DavidKorczynski 's efforts for #114, since the GitHub fuzzing action jobs will otherwise spend a lot of time on timeouts. Basic reproducer based on #include <stdio.h>
#include <stdint.h>
#include <string.h>
#include "tinyexpr.h"
int main(int argc, char *argv[])
{
const char *c = "ncr(ncr(2,7),1)";
double r = te_interp(c, 0);
printf("the execution of this calculation should be slow");
return 0;
} For completeness, here is the fuzzer harness code I used to find the issue: #include <stdio.h>
#include <stdint.h>
#include <string.h>
#include "tinyexpr.h"
#define FUZZER_MAX_INPUT_LENGTH 512
int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
if(size > FUZZER_MAX_INPUT_LENGTH) {
return 0;
}
char inputbuffer[FUZZER_MAX_INPUT_LENGTH + 1] = {0};
memcpy(inputbuffer, data, size);
// null-terminate
inputbuffer[size] = 0;
// call target
double r = te_interp(inputbuffer, 0);
return 0;
} I also make this available under zlib license so it can be used in the project. Compared with the fuzzer harness proposed in #114, it has a restricted max length through fixed stack buffer usage (-> less heap allocations), and doesn't pre-define expression variables. Otherwise it's fairly similar. |
Issue description
I've recently discovered via fuzzing with libFuzzer that nested usage of the
ncr()
expression can result in unexpectedly long expression evaluation times.The smallest representation of the problematic expression that I could find so far, as generated by libFuzzer:
ncr(ncr(2,7),1)
I measured around ~60s runtime at -O3 (with fuzzing instrumentation overhead) on a modern AMD Ryzen CPU.
This may be a low-severity security issue for projects that evaluate externally provided expression input.
Debug information:
The undefined behavior sanitizer also complains about the 'unsigned int' range, here are some variants:
Some gdb debug info for the above mentioned
ncr(ncr(2,7),1)
case where this happens:Disclosure information
I have privately reported this issue to @codeplea. He asked me to document it via a public Github issue.
The text was updated successfully, but these errors were encountered: