[Support] Fix buffer overflow in regcomp

[DavidKorczynski]

OQUEST_ and OCH_ causes the scan pointer to skip elements in g's strip buffer. However, the terminating character of g->strip may be within the skipped elements, and there is currently no checking of that. This adds a check on the skipped elements to ensure no overflow happens.

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65423

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: None (DavidKorczynski)

Changes

OQUEST_ and OCH_ causes the scan pointer to skip elements in g's strip buffer. However, the terminating character of g->strip may be within the skipped elements, and there is currently no checking of that. This adds a check on the skipped elements to ensure no overflow happens.

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65423

---

Full diff: https://github.com/llvm/llvm-project/pull/76681.diff

1 Files Affected:

• (modified) llvm/lib/Support/regcomp.c (+11-1)

diff --git a/llvm/lib/Support/regcomp.c b/llvm/lib/Support/regcomp.c
index 990aef32a396fa..1f68008d6a2937 100644
--- a/llvm/lib/Support/regcomp.c
+++ b/llvm/lib/Support/regcomp.c
@@ -1601,6 +1601,7 @@ findmust(struct parse *p, struct re_guts *g)
 	sop s;
 	char *cp;
 	sopno i;
+	unsigned int skipsize;
 
 	/* avoid making error situations worse */
 	if (p->error != 0)
@@ -1625,7 +1626,16 @@ findmust(struct parse *p, struct re_guts *g)
 		case OCH_:
 			scan--;
 			do {
-				scan += OPND(s);
+				/* Ensure end is not skipped */
+				skipsize = OPND(s);
+				while (skipsize > 0) {
+					if (OP(*scan) == OEND) {
+						g->iflags |= REGEX_BAD;
+						return;
+					}
+					scan++;
+					skipsize--;
+				}
 				s = *scan;
 				/* assert() interferes w debug printouts */
 				if (OP(s) != O_QUEST && OP(s) != O_CH &&

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 463dad107f4cb60ae1d49138143d6797599fb1fb 9ec1407e3f3fc4ab7518b2b7c28869bd870705f9 -- llvm/lib/Support/regcomp.c llvm/unittests/Support/SpecialCaseListTest.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Support/regcomp.c b/llvm/lib/Support/regcomp.c
index 1f68008d6a..08ef1adcc5 100644
--- a/llvm/lib/Support/regcomp.c
+++ b/llvm/lib/Support/regcomp.c
@@ -1601,9 +1601,9 @@ findmust(struct parse *p, struct re_guts *g)
 	sop s;
 	char *cp;
 	sopno i;
-	unsigned int skipsize;
+        unsigned int skipsize;
 
-	/* avoid making error situations worse */
+        /* avoid making error situations worse */
 	if (p->error != 0)
 		return;
 
@@ -1626,17 +1626,17 @@ findmust(struct parse *p, struct re_guts *g)
 		case OCH_:
 			scan--;
 			do {
-				/* Ensure end is not skipped */
-				skipsize = OPND(s);
-				while (skipsize > 0) {
-					if (OP(*scan) == OEND) {
-						g->iflags |= REGEX_BAD;
-						return;
-					}
-					scan++;
-					skipsize--;
-				}
-				s = *scan;
+                          /* Ensure end is not skipped */
+                          skipsize = OPND(s);
+                          while (skipsize > 0) {
+                            if (OP(*scan) == OEND) {
+                              g->iflags |= REGEX_BAD;
+                              return;
+                            }
+                            scan++;
+                            skipsize--;
+                          }
+                                s = *scan;
 				/* assert() interferes w debug printouts */
 				if (OP(s) != O_QUEST && OP(s) != O_CH &&
 							OP(s) != OOR2) {

[DavidKorczynski]

I'm not entirely sure about what to do with the formatting -- the current PR uses tabs as the rest of regcomp.c does that. However, I'm being asked by the formatter to change my additions to spaces, although this will be inconsistent with the rest of regcomp.c.

What to do?

[dwblaikie]

No worries about the formatting - sticking with the formatting of the rest of the file is fine.

Does this need test coverage?

Is there any chance this bug is fixed in the upstream/original regex library this code is based on? Be nice to be able to just take their fix rather than coming up with our own if possible.

[nikic]

What's the regex that causes this issue? I'm not sure this fix is correct -- I don't think we should be emitting an OCH_ that skips over OEND in the first place.

[DavidKorczynski]

This is the stacktrace:

    #1 0x84d0f7 in llvm_regcomp [llvm-project/llvm/lib/Support/regcomp.c:371](https://github.com/llvm/llvm-project/blob/7e186d366d6c7def0543acc255931f617e76dff0/llvm/lib/Support/regcomp.c#L371):2
    #2 0x7fb86b in llvm::Regex::Regex(llvm::StringRef, llvm::Regex::RegexFlags) [llvm-project/llvm/lib/Support/Regex.cpp:36](https://github.com/llvm/llvm-project/blob/7e186d366d6c7def0543acc255931f617e76dff0/llvm/lib/Support/Regex.cpp#L36):11
    #3 0x57c5d2 in llvm::SpecialCaseList::Matcher::insert(llvm::StringRef, unsigned int, bool) [llvm-project/llvm/lib/Support/SpecialCaseList.cpp:45](https://github.com/llvm/llvm-project/blob/7e186d366d6c7def0543acc255931f617e76dff0/llvm/lib/Support/SpecialCaseList.cpp#L45):11
    #4 0x581555 in llvm::SpecialCaseList::parse(llvm::MemoryBuffer const*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&) [llvm-project/llvm/lib/Support/SpecialCaseList.cpp:194](https://github.com/llvm/llvm-project/blob/7e186d366d6c7def0543acc255931f617e76dff0/llvm/lib/Support/SpecialCaseList.cpp#L194):26
    #5 0x57fd73 in createInternal [llvm-project/llvm/lib/Support/SpecialCaseList.cpp:127](https://github.com/llvm/llvm-project/blob/7e186d366d6c7def0543acc255931f617e76dff0/llvm/lib/Support/SpecialCaseList.cpp#L127):8
    #6 0x57fd73 in llvm::SpecialCaseList::create(llvm::MemoryBuffer const*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&) [llvm-project/llvm/lib/Support/SpecialCaseList.cpp:93](https://github.com/llvm/llvm-project/blob/7e186d366d6c7def0543acc255931f617e76dff0/llvm/lib/Support/SpecialCaseList.cpp#L93):12
    #7 0x56dc21 in LLVMFuzzerTestOneInput [llvm-project/llvm/tools/llvm-special-case-list-fuzzer/special-case-list-fuzzer.cpp:23](https://github.com/llvm/llvm-project/blob/7e186d366d6c7def0543acc255931f617e76dff0/llvm/tools/llvm-special-case-list-fuzzer/special-case-list-fuzzer.cpp#L23):3

In this sense it seems the pattern is defined here:

https://github.com/llvm/llvm-project/blob/7e186d366d6c7def0543acc255931f617e76dff0/llvm/lib/Support/SpecialCaseList.cpp#L185-L192C32

In ASCII, the input from the fuzzer looks as follows:

#!special-case-list-v1
:){0}(   | )(\1

[DavidKorczynski]

Does this need test coverage?

I'm not strongly opinionated on this: OSS-Fuzz will catch and flag the bug if a regression happens, but having a test will make it more obvious, and instantaneous, if a regression happens. Added a test.