Very high security flaws found in PCRE: use memcpy_s if available

[WilliamTambellini]

Hi
veracode analysis
https://www.veracode.com/products/binary-static-analysis-sast
has identified 5 "very high security flaws" in the latest release (10.42):
3 Stack-based Buffer Overflow (CWE ID 121) in:

 src/pcre2_match.c 7498 
 src/pcre2_serialize.c 195 
 src/pcre2_serialize.c 230 

and
2 "numeric errors" Integer Overflow or Wraparound (CWE ID 190)

src/pcre2_dfa_match.c 4008 
src/pcre2_match.c 7498

ref:
https://cwe.mitre.org/

Would you consider investigating it ?
best

[carenas]

not sure about the other entries, but at least src/pcre2_match.c:7498 corresponds to:

memcpy((void *)match_data->subject, subject, length);

which can't be neither CWE ID 121 nor CWE ID 190 as it copies data to a newly created pointer in the HEAP.

could there be a mistake in the report?

[PhilipHazel]

All except one of the others are similar: complaining about a memcpy() into a newly allocated heap block. The odd one out is line 4008 in pcre2_match.c which is:

if (MAX_255(fc) && (mb->ctypes[fc] & ctype_word) != 0)

which is just one example of many similar lines with different ctype_xxxx masks - so why only the one complaint? Anyway I can't see anything wrong with any of these.

[WilliamTambellini]

Tks gentlemen.
I ve built that way

./configure --prefix=pcre21042dbg --enable-debug CPPFLAGS=-DDEBUG CFLAGS="-gdwarf-2 -g3 -O0 -fno-builtin"

so perhaps that screwed line numbers ?

The line number 7498 is:

pcre2/src/pcre2_match.c

Line 7498 in 52c0884

memcpy((void *)match_data->subject, subject, length);

and the CWE ID 121
https://cwe.mitre.org/data/definitions/121.html
so perhaps veracode indeed is reporting the bad CWE ID or simply means 'overflow'.
Seen on their website:
https://community.veracode.com/s/question/0D53n00007qVSblCAG/stackbased-buffer-overflow-cwe-id-1212-flaws-in-cc
perhaps they just advice to use memcpy_s ?
https://en.cppreference.com/w/c/string/byte/memcpy

[PhilipHazel]

memcpy_s is defined in an optional Appendix to C11 but I don't seem to have it on my Arch Linux box, which has both gcc and clang installed. I therefore suspect it isn't widely available.

[WilliamTambellini]

hum indeed, memcpy_s hard to find:
https://aticleworld.com/memcpy_s-in-c/
seems to require
#define STDC_WANT_LIB_EXT1 1
and ifdef STDC_LIB_EXT1
I m going to change the title of that issue to just use memcpy_s if possible, if not, memcpy.
Tks

[carenas]

Blindly replacing memcpy with memcpy_s doesn't seem like a productive change, specially since it might be not what the tool was originally suggesting (unless more than a hunch could be used to validate that, and considering the conflicting and unusual reports).

[WilliamTambellini]

I did a dummy test and veracode seems happy with memcpy_s.
That being said, memcpy_s was a proposal from microsoft, not globally approved by the C committee so ended up optional in C11. It seems to be only available on microsoft platforms so would nt resolve the alerts for the linux/mac/... builds.

https://www.cisa.gov/uscert/bsi/articles/knowledge/coding-practices/memcpy_s%28%29-and-memmove_s%28%29

[carenas]

its removal from the standard has been also proposed:

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1967.htm

more importantly; except for a theoretical and unlikely scenario it doesn't seem to add any value.

does the following patch (as an alternative to moving to memcpy_s from memcpy) also resolve the alert in pcre2_match.c:7498 ?:

diff --git a/src/pcre2_match.c b/src/pcre2_match.c
index 168b9fa..2bd0297 100644
--- a/src/pcre2_match.c
+++ b/src/pcre2_match.c
@@ -7491,6 +7491,8 @@ if (rc == MATCH_MATCH)
     mb->last_used_ptr : mb->end_match_ptr) - subject;
   if ((options & PCRE2_COPY_MATCHED_SUBJECT) != 0)
     {
+    if (length > (PCRE2_SIZE_MAX/(PCRE2_CODE_UNIT_WIDTH/8)))
+      return PCRE2_ERROR_NOMEMORY;
     length = CU2BYTES(length + was_zero_terminated);
     match_data->subject = match_data->memctl.malloc(length,
       match_data->memctl.memory_data);

[JetXujing]

I agree with upstairs. In fact, memcpy_s ensures that the copy length does not overflow. This problem can be avoided as long as we ensure that the length of memcpy does not overflow. The memcpy_s solution is not the only one.

[PhilipHazel]

I am closing this issue as it is clear that using memcpy_s() is not something that is going to happen in PCRE2.