Re: [Xen-devel] [PATCH v2] libxc: add LZ4 decompression support

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-09-25 at 08:23 +0100, Jan Beulich wrote:
> >>> On 24.09.13 at 18:53, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> > On Tue, 2013-09-24 at 17:29 +0100, Jan Beulich wrote:
> >> @@ -573,6 +575,124 @@ int xc_try_xz_decode(struct xc_dom_image
> >>  
> >>  #endif /* !__MINIOS__ */
> >>  
> >> +#define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> >> +#define STATIC static
> >> +#define u8 uint8_t
> >> +#define u16 uint16_t
> >> +#define u32 uint32_t
> >> +#define u64 uint64_t
> >> +#define INIT
> >> +#define unlikely(x) (x)
> > 
> > I think rather than pollute this file with this sort of thing (which
> > might have unexpected consequences in the future) this would be better
> > of placed in a separate file compiled for both regular and stub use.
> 
> So you mean to move the whole thing quoted above down to ...
> 
> >> +
> >> +static inline uint_fast16_t le16_to_cpup(const unsigned char *buf)
> >> +{
> >> +    return buf[0] | (buf[1] << 8);
> >> +}
> >> +
> >> +static inline uint_fast32_t le32_to_cpup(const unsigned char *buf)
> >> +{
> >> +    return le16_to_cpup(buf) | ((uint32_t)le16_to_cpup(buf + 2) << 16);
> >> +}
> >> +
> >> +#include "../../xen/common/lz4/decompress.c"
> 
> ... here out into a separate file. I can surely do that.

I was thinking to include xc_try_lz4_decode too, but this would work
well, whichever you prefer.

> Any preference for the name (xc_dom_decompress_unsafe_lz4.c
> would seem the prime candidate, albeit the other similarly
> named files have a slightly different purpose).

Following the pattern used by the existing unsafe decompressors would
seem reasonable.

> The main problem doing so is going to be the uses of
> get_unaligned_le32() in xc_try_lz4_decode() - they'd need to
> be adjusted, increasing the amount of change from the original.
> And lz4_decompress() would the also need to be declared in
> some (new?) header. All of which didn't seem very desirable...

In which case maybe moving xc_try.. too is the right answer, there is
already a list of prototypes for the others (albeit in a minios ifdef)

> > I don't really like exit_0 and exit_2 as names (what happened to
> > exit_1?). "err" and "err_free" would be more usual.
> 
> I didn't like those too, but tried to modify the original as little
> as possible. exit_1 disappeared as having got orphaned in the
> process of the adaptation.

I didn't realise this had come from elsewhere rather than being newly
written xc glue code. I suppose its fine then.

> 
> >> +  }
> >> +
> >> +  out_size = out_len = get_unaligned_le32(inp + size);
> >> +  if (xc_dom_kernel_check_size(dom, out_len)) {
> >> +          msg = "Decompressed image too large";
> >> +          goto exit_0;
> >> +  }
> >> +
> >> +  output = malloc(out_len);
> >> +  if (!output) {
> >> +          msg = "Could not allocate output buffer";
> >> +          goto exit_0;
> >> +  }
> >> +  outp = output;
> >> +
> >> +  chunksize = get_unaligned_le32(inp);
> >> +  if (chunksize == ARCHIVE_MAGICNUMBER) {
> >> +          inp += 4;
> >> +          size -= 4;
> >> +  } else {
> >> +          msg = "invalid header";
> >> +          goto exit_2;
> >> +  }
> >> +
> >> +  for (;;) {
> >> +          if (size < 4) {
> >> +                  msg = "missing data";
> >> +                  goto exit_2;
> >> +          }
> >> +          chunksize = get_unaligned_le32(inp);
> >> +          if (chunksize == ARCHIVE_MAGICNUMBER) {
> >> +                  inp += 4;
> >> +                  size -= 4;
> >> +                  continue;
> >> +          }
> >> +          inp += 4;
> >> +          size -= 4;
> > 
> > We know size is at least 4 from the check at the head of the loop, but
> > now we subtracted 4 so size could be 0, but lz4_decompress doesn't take
> > size as an argument so whatever it does it isn't basing the return value
> > in &chunksize on it, so I assume it reads over the end of the buffer via
> > inp.
> > 
> > I didn't look to see what the minimum number of bytes which
> > lz4_decompress will consume is, but I think we need a check of some
> > description.
> 
> Sadly there doesn't appear to be a minimum - the main loop in
> lz4_uncompress() has just a single exit point, which isn't
> dependent upon the input size at all (the function doesn't even
> know how much input there is).
> 
> Immediately before the call to lz4_decompress() chunksize gets
> read, but I can't tell whether we could expect that to be the
> required amount of bytes to follow. Yann - can you clarify this?

Whatever expectations we might have from the encoder a malicious
attacker can construct a byte stream which violates it and has
ARCHIVE_MAGICNUMBER as the last thing.

Under the circumstances I don't see how lz4_decompress() can be safely
used on untrusted data since it has no knowledge of the remaining input
length and therefore no way to avoid running off the end of the data.
Digging down a couple of layers of callchain it seems like this
knowledge isn't use anywhere (i.e. it's not just lost in the outermost
API.

This is a blocker for inclusion IMHO.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel