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

[Jessica Clarke]

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.

Jess

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