[libvirt PATCH 3/4] virFileReadLimFD: Limit maximum file size to INT_MAX - 1

[Daniel P. Berrangé]

On Wed, Jul 21, 2021 at 03:02:59PM +0200, Peter Krempa wrote:
> On Wed, Jul 21, 2021 at 14:46:42 +0200, Tim Wiederhake wrote:
> > virFileReadLimFD always returns null-terminated data. To that end, it has to
> > add one to the maximum file size. If the maxium file size is INT_MAX, this
> > triggers a signed integer overflow.
> > 
> > There is no instance left where a caller would call virFileReadLimFD with a
> > maxium file size of INT_MAX. Make virFileReadLimFD error out if the maximum
> > file size is INT_MAX to prevent the reintroduction of this issue.
> > 
> > Signed-off-by: Tim Wiederhake <twiederh at redhat.com>
> > ---
> >  src/util/virfile.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/util/virfile.c b/src/util/virfile.c
> > index 723e1ca6e5..b5600658d5 100644
> > --- a/src/util/virfile.c
> > +++ b/src/util/virfile.c
> > @@ -1418,7 +1418,7 @@ virFileReadLimFD(int fd, int maxlen, char **buf)
> >      size_t len;
> >      char *s;
> >  
> > -    if (maxlen <= 0) {
> > +    if ((maxlen <= 0) || (maxlen >= INT_MAX)) {
> >          errno = EINVAL;
> >          return -1;
> 
> While '< 0' is common sense here, limiting to INT_MAX -1 should be
> mentioned in the docs.
> 
> Or better, why aren't we converting this to 'size_t' instead?
> 
> saferead_lim is already operating on 'size_t' and I think we could
> simply get rid of the overflow checks altogether when working with
> size_t.

Honestly we could probably eliminate essentially all use of this
method. The only scenario in which we need to limit the amount
of data we read from a file is for untrustworthy input files
controlled by a user with lower privilege level than libvirt.

There might be such a case somewhere, but in usage I've looked
at so far it is not required, as the files are generally all
owned y the same user, or by root, or are exposed by the kernel
as magic sysfs files.

The "maxlen" parameter is something I'm to blame for introducing
years ago in

  commit 94e49e3f0e82b8b059d71b131f5e6be2a1d6de2d
  Author: Daniel P. Berrangé <berrange at redhat.com>
  Date:   Mon Jan 7 15:21:33 2008 +0000

    Fix config file reading to not truncate large files

Before this commit there was a buflen parameter that was genuinely
needed because we were reading into a preallocated buffer. My
change switched to dynamically allocated buffers, but kept a limit
for reasons I can't really justify today.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|