Use assertion to ensure erroroffset return from pcre2_compile is valid #460

alexdowad · 2024-09-03T23:23:06Z

When testing the new pattern rewriting phase for regex compilation using a fuzzer, I had a scary experience. Due to a bug in my pattern rewriting code, pcre2_compile() could return a totally invalid erroroffset. If a library user tried to do something with the erroroffset without checking it for validity, in the worst case, this had the potential to lead to an RCE vulnerability.

In case something similar ever happens again, I've added an assertion which will make it easier to notice the problem.

alexdowad · 2024-09-04T01:00:10Z

I'm surprised to see that one of these assertions fails when RunTest executes in CI. I haven't been able to duplicate this locally yet (all the tests pass for me) and am still trying to figure out the reason.

carenas

the output of the steps from CI could help you replicate locally, maybe didn't configure with --enable-debug?

carenas · 2024-09-04T03:10:12Z

src/pcre2_compile.c

@@ -11147,6 +11147,8 @@ an offset is available in the parsed pattern. */
 ptr = pattern + cb.erroroffset;

 HAD_EARLY_ERROR:
+PCRE2_ASSERT(ptr >= pattern); /* Ensure we don't return invalid erroroffset */
+PCRE2_ASSERT(ptr < (pattern + patlen));


s/</<=/, since the NUL at the end of pattern is a perfectly valid erroroffset

@carenas Thanks for mentioning that. I actually flip-flopped between < and <= when preparing this PR. However, I noticed that (if I have understood well) it seems that patterns which are passed to pcre2_compile are not always NUL-terminated. (When the pattern is NUL-terminated, one can pass PCRE2_ZERO_TERMINATED as the length argument to pcre2_compile.)

Is that correct? If so, should the assertion be predicated on length == PCRE2_ZERO_TERMINATED?

should the assertion be predicated on length == PCRE2_ZERO_TERMINATED?

no; and it is unlikely to work as the length value is reset earlier AFAIK.

the documentation mentions:

Some errors are not detected until the whole pattern has been scanned; in these cases, the offset passed back is the length of the pattern

should the assertion be predicated on length == PCRE2_ZERO_TERMINATED?

no; and it is unlikely to work as the length value is reset earlier AFAIK.

I think I didn't express myself well. I was referring to the value of length on function entry, not at the point where the assertion occurs.

the documentation mentions:

Some errors are not detected until the whole pattern has been scanned; in these cases, the offset passed back is the length of the pattern

Hmm. If that is so, then I guess pattern + erroroffset is not guaranteed to be a valid pointer. Since PCRE2 users may presumably wish to use pattern + erroroffset to display the part of the pattern which caused an error, this feels like a defect in the API to me.

I guess if users read the documentation, then they should know to check for the special value erroroffset == patlength.

Anyways, regardless of the issues mentioned, it is apparent that the assertion should indeed use <= and not <. PR updated.

I guess if users read the documentation, then they should know to check for the special value erroroffset == patlength.

sure hope they do, specially if trying to print the place where the pattern error was found, as they then would realize it might not be printable, if it happens to be in the middle of an UTF8 character, for example.

should the assertion be predicated on length == PCRE2_ZERO_TERMINATED?

no; and it is unlikely to work as the length value is reset earlier AFAIK.

I think I didn't express myself well. I was referring to the value of length on function entry, not at the point where the assertion occurs.

What I meant was that there is no way for PCRE2 to know if a pattern is NUL terminated or not, PCRE2_ZERO_TERMINATED is an "optimization" so callers of this function can rely on PCRE2 doing strlen() for them) if they don't have that value at hand.

For additional context, '\0' in patterns used to be impossible before PCRE2 and I suspect most patterns to be NUL terminated C strings regardless of what was provided in length to pcre2_compile().

…in bounds When testing a patch for PCRE2, I found that due to a bug in my code, `pcre2_compile()` could return a totally invalid error offset. In case something similar ever happens again, I've added an assertion which will make it easier to notice the problem. It should be noted that the pcre2api manpage states: "Some errors are not detected until the whole pattern has been scanned; in these cases, the offset passed back is the length of the pattern." Since patterns are not always null-terminated, this means that `pattern + erroroffset` may sometimes point to uninitialized (or even unmapped) memory. However, it is still worthwhile to guard against other unexpected values being returned in `erroroffset`.

carenas · 2024-09-08T22:43:41Z

Note that this assertion is buggy, since patlen was never updated when the pattern was rewritten, but more importantly, the offseterror from a rewritten pattern is invalid outside this function as it references an offset to a different pattern than the one that was provided.

alexdowad · 2024-09-08T22:48:40Z

@carenas Good point. That would be true if pattern rewriting is done, which is not the case in master yet.

carenas · 2024-09-08T23:23:20Z

That would be true

if being pedantic, you are correct that this assert isn't buggy yet in master, but obviously the comment was meant to address it within the context of this change being part of that series (as explained above) and which was also linked.

I did it this way to avoid polluting the conversation in the other PR, not to make a false statement, but would follow whichever preferences you have to avoid further confusion.

PS: FWIW talking about prefences I prefer not to be "@" to avoid notification spam.

alexdowad · 2024-09-08T23:42:43Z

Understood, I won't keep sending you "@" notifications then.

This PR is not simply part of a series with the other PR; it is a worthwhile change on its own, which is why I submitted it separately.

alexdowad · 2024-09-08T23:54:00Z

Please see added comment in #464 on why the assertion added in this PR is still valid, even if pattern rewriting optimizations are later merged into master.

carenas · 2024-09-09T00:50:18Z

hy the assertion added in this PR is still valid

Glad to be wrong about needing to update patlen, since that was also a major sticking point as there is no way to communicate that back to the user by the current API.

This means though that ALL erroroffset (except for the few ones that were cached in parsed_pattern) are likely incorrect, eventhough you are right that they are also likely to be lower than patlen and therefore not trigger this assert.

alexdowad · 2024-09-09T00:56:11Z

ALL erroroffset (except for the few ones that were cached in parsed_pattern) are likely incorrect

I don't believe that is the case, for the reasons explained in #464. However, it is good to be sure, so I will check this area over again.

carenas · 2024-09-09T02:08:27Z

don't believe that is the case

cb.erroroffset is set by all functions that participate in the compilation with code like:

if (bad)
  {
  *errorcodeptr = ERR
  cb->erroroffset = offset; /* or an expression and sometimes even a ptrdiff */
  return FALSE;
  }

how can that work when offset is based on the rewritten pattern?

alexdowad · 2024-09-09T02:35:02Z

how can that work when offset is based on the rewritten pattern?

The point is that if it is set to a value other than patlen, erroroffset must be set to an offset within the pattern string, by code which is analyzing the pattern string. The pattern rewriter does not modify the pattern string. It only modifies the parsed_pattern vector. Hence, it has no effect on any code which analyzes the pattern string.

If there is any code in PCRE2 which currently sets erroroffset based on the offset of a parsed_pattern item within the parsed_pattern vector, that is very wrong and must be fixed. I don't believe that such code exists, but I would like to check and make sure.

We definitely do have code which sets erroroffset to 0, to patlen, and to PCRE2_UNSET. Setting it to PCRE2_UNSET seems wrong to me and I would like to look into that. However, it is a different issue.

carenas · 2024-09-09T04:34:12Z

I stand corrected, apologies for the confusion.

carenas suggested changes Sep 4, 2024

View reviewed changes

alexdowad force-pushed the guard branch from 36673bd to 15e073d Compare September 4, 2024 05:40

PhilipHazel merged commit 317f339 into PCRE2Project:master Sep 4, 2024
11 checks passed

alexdowad deleted the guard branch September 4, 2024 11:41

carenas mentioned this pull request Sep 8, 2024

Rewrite regexes where common prefix can be pulled out from alternation branches #464

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use assertion to ensure erroroffset return from pcre2_compile is valid #460

Use assertion to ensure erroroffset return from pcre2_compile is valid #460

alexdowad commented Sep 3, 2024

alexdowad commented Sep 4, 2024

carenas left a comment •

edited

Loading

carenas Sep 4, 2024

alexdowad Sep 4, 2024

carenas Sep 4, 2024

alexdowad Sep 4, 2024

alexdowad Sep 4, 2024

carenas Sep 4, 2024

carenas Sep 8, 2024 •

edited

Loading

carenas commented Sep 8, 2024 •

edited

Loading

alexdowad commented Sep 8, 2024

carenas commented Sep 8, 2024 •

edited

Loading

alexdowad commented Sep 8, 2024

alexdowad commented Sep 8, 2024

carenas commented Sep 9, 2024

alexdowad commented Sep 9, 2024

carenas commented Sep 9, 2024

alexdowad commented Sep 9, 2024

carenas commented Sep 9, 2024

Use assertion to ensure erroroffset return from pcre2_compile is valid #460

Use assertion to ensure erroroffset return from pcre2_compile is valid #460

Conversation

alexdowad commented Sep 3, 2024

alexdowad commented Sep 4, 2024

carenas left a comment • edited Loading

Choose a reason for hiding this comment

carenas Sep 4, 2024

Choose a reason for hiding this comment

alexdowad Sep 4, 2024

Choose a reason for hiding this comment

carenas Sep 4, 2024

Choose a reason for hiding this comment

alexdowad Sep 4, 2024

Choose a reason for hiding this comment

alexdowad Sep 4, 2024

Choose a reason for hiding this comment

carenas Sep 4, 2024

Choose a reason for hiding this comment

carenas Sep 8, 2024 • edited Loading

Choose a reason for hiding this comment

carenas commented Sep 8, 2024 • edited Loading

alexdowad commented Sep 8, 2024

carenas commented Sep 8, 2024 • edited Loading

alexdowad commented Sep 8, 2024

alexdowad commented Sep 8, 2024

carenas commented Sep 9, 2024

alexdowad commented Sep 9, 2024

carenas commented Sep 9, 2024

alexdowad commented Sep 9, 2024

carenas commented Sep 9, 2024

carenas left a comment •

edited

Loading

carenas Sep 8, 2024 •

edited

Loading

carenas commented Sep 8, 2024 •

edited

Loading

carenas commented Sep 8, 2024 •

edited

Loading