Invalid escaped character in string will crash the parser. #222
Assignees
Comments
|
Thanks, good catch! |
wravery
added a commit
to wravery/cppgraphqlgen
that referenced
this issue
Mar 14, 2022
wravery
added a commit
to wravery/cppgraphqlgen
that referenced
this issue
Mar 14, 2022
|
Hmm... I haven't been able to reproduce this. Maybe it was an older build of PEGTL that triggered it. I'm going to merge a new unit test to pin the fix. |
|
This is occuring for me on 3.7.1 |
|
I just upgraded to PEGTL 3.2.5 and this issue still occurs for me in cppgraphqlgen 3.7.1 I have a directive:
Then on a string field I put:
Testing with clientgen I get the crash 100%. I also get the crash when subscribing with the same query. |
|
Are you catching parse exceptions? For example, the unit test I'm adding looks like this: try
{
memory_input<> input(R"gql(query { foo @something(arg: "\.") })gql",
"InvalidStringEscapeSequence");
parsedQuery = parse<executable_document>(input);
}
catch (const peg::parse_error& e)
{
using namespace std::literals;
ASSERT_NE(nullptr, e.what());
EXPECT_TRUE(
"InvalidStringEscapeSequence:1:31: parse error matching struct graphql::peg::string_escape_sequence_content"sv
== e.what())
<< e.what();
caughtException = true;
} |
Ah... you mean specifically |
|
...although catch (const graphql::peg::parse_error& pe)
{
std::cerr << "Invalid GraphQL: " << pe.what() << std::endl;
for (const auto& position : pe.positions())
{
std::cerr << "\tline: " << position.line << " column: " << position.column << std::endl;
}
return 1;
} |
Both client gen and the subscription service, when passed a query string. Anyway I will try and make a repro today |
|
Also note that because this is a callstack overflow it is unable to further call into the exception handling routines and then unwind the stack, instead the callstack runs off into a guard page and crashes/produces a unrecoverable exception. |
|
OK so I have a 100% reproduction on a completly clean check out of the latest, followed with a git checkout of v3.7.1:
schema file: Query file: using clientgen with these files causes the issue. |
|
|
Please also note that I am using VS2019, and this is a 64 bit Debug build |
|
Further to this, using a query file like this Produces the following output So this is to do with nesting of objects, which causes a much larger callstack. |
|
100% this is a stack exhaustion issue. If I change clientgen to use the linker option This will be a potential DOS issue where a query is able to be controlled by an attacker. |
|
Is it possible to set a max nesting depth? Could this be a CMake build option so that it is customisable? |
|
Here is a patch for 3.7.1 with my fix using PEGTL |
|
Thanks for the detailed repro steps, I can reproduce this with the |
|
I can trigger the stack overflow just by wrapping a few more levels of node fields around the selection set, so it does need to be bounded. |
|
Or maybe not... it also depends on throwing an exception. If I fix the invalid escape sequence (replace |
|
I think I'm starting to understand what's going on. I think the stack is being exhausted because the catch block in PEGTL's |
|
I think I can replace the That doesn't remove the need for some kind of depth limits on the parser, otherwise untrusted queries can DoS the application before it can perform any inspection on the AST. However, fixing the stack consumption in the unwind should more or less double the safe depth limit for any given target (assuming the target toolchain unwinds the stack in a similar way). |
|
Relevant discussion: taocpp/PEGTL#256. |
|
I edited my local copy of query {
node1 {
node2 {
node3 {
node4 {
node5 {
node6 {
node7 {
node8 {
node9 {
node10 {
# ...
node100 {
name @test( str : "\." )
}
# ...
}
}
}
}
}
}
}
}
}
}
}
}
type foo {
bar: String
} |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hi,
It is possible to crash the parser (client/query/subscription) by having an invalid escaped character.
For example using a directive:
This looks like it causes an infinite recursion
The text was updated successfully, but these errors were encountered: