git: 410556f1f10f - main - libctf: Fix an out-of-bounds read in ctf_lookup_by_name()

[Mark Johnston]

On Sat, Mar 27, 2021 at 06:16:44PM +0000, Jessica Clarke wrote:
> On 27 Mar 2021, at 18:06, Mark Johnston <markj at FreeBSD.org> wrote:
> > 
> > The branch main has been updated by markj:
> > 
> > URL: https://cgit.FreeBSD.org/src/commit/?id=410556f1f10fd35b350102725fd8504c3cb0afc8
> > 
> > commit 410556f1f10fd35b350102725fd8504c3cb0afc8
> > Author:     Domagoj Stolfa <domagoj.stolfa at gmail.com>
> > AuthorDate: 2021-03-27 18:04:12 +0000
> > Commit:     Mark Johnston <markj at FreeBSD.org>
> > CommitDate: 2021-03-27 18:04:12 +0000
> > 
> >    libctf: Fix an out-of-bounds read in ctf_lookup_by_name()
> > 
> >    When prefixes such as struct, union, etc. are compared with the current
> >    type (e.g. struct foo), a comparison is made with the prefix.  The code
> >    currently assumes that every type is a valid C type with a prefix,
> >    however at times, garbage ends up in this function causing an
> >    unpredictable crash with DTrace due to the isspace(*p) call or
> >    subsequent calls. An example that I've seen of this is the letter 's'
> >    being passed in, comparing true with struct as the comparison size was
> >    (q - p) == 1, but then we increment p with the length of "struct",
> >    resulting in an out of bounds read.
> > 
> >    Reviewed by:    markj
> >    MFC after:      1 week
> >    Differential Revision:  https://reviews.freebsd.org/D29435
> > ---
> > cddl/contrib/opensolaris/common/ctf/ctf_lookup.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/cddl/contrib/opensolaris/common/ctf/ctf_lookup.c b/q
> > index aa58663309b6..5912cc1a36e8 100644
> > --- a/cddl/contrib/opensolaris/common/ctf/ctf_lookup.c
> > +++ b/cddl/contrib/opensolaris/common/ctf/ctf_lookup.c
> > @@ -132,8 +132,9 @@ ctf_lookup_by_name(ctf_file_t *fp, const char *name)
> > 			continue; /* skip qualifier keyword */
> > 
> > 		for (lp = fp->ctf_lookups; lp->ctl_prefix != NULL; lp++) {
> > -			if (lp->ctl_prefix[0] == '\0' ||
> > -			    strncmp(p, lp->ctl_prefix, (size_t)(q - p)) == 0) {
> > +			if ((size_t)(q - p) >= lp->ctl_len &&
> > +			    (lp->ctl_prefix[0] == '\0' ||
> > +			    strncmp(p, lp->ctl_prefix, (size_t)(q - p)) == 0)) {
> > 				for (p += lp->ctl_len; isspace(*p); p++)
> > 					continue; /* skip prefix and next ws */
> 
> We had a student porting DTrace to CheriBSD as a Master's project notice this
> and get this fixed “upstream” in Illumos[1], but neglected to do so in FreeBSD
> (and it seems CheriBSD has an earlier version of the patch that I requested be
> changed in the upstream review...); you might wish to pull that in instead?
> It’s equivalent, just differently formatted, so adds noise to the diff.

Ah, I had pondered suggesting the exact same change.  I think we can
just apply this adjustment as a separate diff.  I posted the diff while
I rerun the test suite: https://reviews.freebsd.org/D29448

> 
> Jess
> 
> [1] https://github.com/illumos/illumos-gate/commit/d15d17d4231f87f1571fa6d585377206f360f667
>