| Message ID | 20240605134054.2626953-37-jmarchan@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | trace-cmd: fix misc issues found by static analysis | expand |
On Wed, 5 Jun 2024 15:40:51 +0200 "Jerome Marchand" <jmarchan@redhat.com> wrote: > In check_protocol_version we compare the protocol version string with > the expected one ("V3") with memcmp(). The received string could be > longer than the constant string used for the comparison. That could > lead to out of range access. > > Check that the received protocol version is not too long. > > Fixes a OVERRUN error (CWE-119) > > Signed-off-by: Jerome Marchand <jmarchan@redhat.com> > --- > tracecmd/trace-record.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > index dc3e5285..c3118546 100644 > --- a/tracecmd/trace-record.c > +++ b/tracecmd/trace-record.c > @@ -3810,7 +3810,7 @@ static void check_protocol_version(struct tracecmd_msg_handle *msg_handle) > msg_handle->version = V1_PROTOCOL; > tracecmd_plog("Use the v1 protocol\n"); > } else { > - if (memcmp(buf, "V3", n) != 0) > + if (n > 3 || memcmp(buf, "V3", n) != 0) > die("Cannot handle the protocol %s", buf); Actually, we may add more to it, so this should be: if (n < 3 || memcmp(buf, "V3", 3) != 0) -- Steve > /* OK, let's use v3 protocol */ > write(fd, V3_MAGIC, sizeof(V3_MAGIC));
On 18/07/2024 04:11, Steven Rostedt wrote: > On Wed, 5 Jun 2024 15:40:51 +0200 > "Jerome Marchand" <jmarchan@redhat.com> wrote: > >> In check_protocol_version we compare the protocol version string with >> the expected one ("V3") with memcmp(). The received string could be >> longer than the constant string used for the comparison. That could >> lead to out of range access. >> >> Check that the received protocol version is not too long. >> >> Fixes a OVERRUN error (CWE-119) >> >> Signed-off-by: Jerome Marchand <jmarchan@redhat.com> >> --- >> tracecmd/trace-record.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c >> index dc3e5285..c3118546 100644 >> --- a/tracecmd/trace-record.c >> +++ b/tracecmd/trace-record.c >> @@ -3810,7 +3810,7 @@ static void check_protocol_version(struct tracecmd_msg_handle *msg_handle) >> msg_handle->version = V1_PROTOCOL; >> tracecmd_plog("Use the v1 protocol\n"); >> } else { >> - if (memcmp(buf, "V3", n) != 0) >> + if (n > 3 || memcmp(buf, "V3", n) != 0) >> die("Cannot handle the protocol %s", buf); > > Actually, we may add more to it, so this should be: > > if (n < 3 || memcmp(buf, "V3", 3) != 0) That's definitely more future proof. I'll send an updated version. Jerome > > -- Steve > >> /* OK, let's use v3 protocol */ >> write(fd, V3_MAGIC, sizeof(V3_MAGIC)); >
diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c index dc3e5285..c3118546 100644 --- a/tracecmd/trace-record.c +++ b/tracecmd/trace-record.c @@ -3810,7 +3810,7 @@ static void check_protocol_version(struct tracecmd_msg_handle *msg_handle) msg_handle->version = V1_PROTOCOL; tracecmd_plog("Use the v1 protocol\n"); } else { - if (memcmp(buf, "V3", n) != 0) + if (n > 3 || memcmp(buf, "V3", n) != 0) die("Cannot handle the protocol %s", buf); /* OK, let's use v3 protocol */ write(fd, V3_MAGIC, sizeof(V3_MAGIC));
In check_protocol_version we compare the protocol version string with the expected one ("V3") with memcmp(). The received string could be longer than the constant string used for the comparison. That could lead to out of range access. Check that the received protocol version is not too long. Fixes a OVERRUN error (CWE-119) Signed-off-by: Jerome Marchand <jmarchan@redhat.com> --- tracecmd/trace-record.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)