Re: [PATCH 13/23] libsepol: assure string NUL-termination

From: James Carter <jwcart2@gmail.com>
To: "Christian Göttsche" <cgzones@googlemail.com>
Cc: SElinux list <selinux@vger.kernel.org>
Subject: Re: [PATCH 13/23] libsepol: assure string NUL-termination
Date: Wed, 9 Jun 2021 10:38:42 -0400	[thread overview]
Message-ID: <CAP+JOzRyZ+rqdRp=SjDnjq7bqm1YO+w1EJOVCCm2O30BiKZZMg@mail.gmail.com> (raw)
In-Reply-To: <20210608155912.32047-14-cgzones@googlemail.com>

On Tue, Jun 8, 2021 at 12:00 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> ibendport_record.c: In function ‘sepol_ibendport_get_ibdev_name’:
> ibendport_record.c:169:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
>   169 |  strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
>       |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ibendport_record.c: In function ‘sepol_ibendport_set_ibdev_name’:
> ibendport_record.c:189:2: error: ‘strncpy’ specified bound 64 equals destination size [-Werror=stringop-truncation]
>   189 |  strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX);
>       |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> strncpy(3) does not NUL-terminate the destination if the source is of
> the same length or longer then the specified size.
> Reduce the size to copy by 1.
>
> Found by Clang
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libsepol/src/ibendport_record.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libsepol/src/ibendport_record.c b/libsepol/src/ibendport_record.c
> index adf67161..2eb8ca18 100644
> --- a/libsepol/src/ibendport_record.c
> +++ b/libsepol/src/ibendport_record.c
> @@ -166,7 +166,7 @@ int sepol_ibendport_get_ibdev_name(sepol_handle_t *handle,
>         if (sepol_ibendport_alloc_ibdev_name(handle, &tmp_ibdev_name) < 0)
>                 goto err;
>
> -       strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX);
> +       strncpy(tmp_ibdev_name, ibendport->ibdev_name, IB_DEVICE_NAME_MAX - 1);
>         *ibdev_name = tmp_ibdev_name;
>         return STATUS_SUCCESS;
>
> @@ -186,7 +186,7 @@ int sepol_ibendport_set_ibdev_name(sepol_handle_t *handle,
>         if (sepol_ibendport_alloc_ibdev_name(handle, &tmp) < 0)
>                 goto err;
>
> -       strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX);
> +       strncpy(tmp, ibdev_name, IB_DEVICE_NAME_MAX - 1);
>         free(ibendport->ibdev_name);
>         ibendport->ibdev_name = tmp;
>         return STATUS_SUCCESS;
> --
> 2.32.0
>

It seems like the strncpy()s at line 65 in
sepol_ibendport_key_create(), at line 233 in sepol_ibendport_clone(),
and in ibendports.c at line 37 in ibendport_from_record() also need to
be changed.
Thanks,
Jim