[Spice-devel] [PATCH spice-common v3] lz: Avoid buffer reading overflow checking for image type

[Christophe Fergeau]

Hey,

On Tue, Jun 26, 2018 at 10:31:00AM +0100, Frediano Ziglio wrote:
> The type of the image is just copied from network without
> any check and later used for array indexing.

Have you considered something like this? This way we can be sure no code
can overflow the IS_IMAGE_TYPE_PLT[] array. Similar changes would be
needed for the other arrays (IS_IMAGE_TYPE_RGB, PLT_PIXELS_PER_BYTE,
RGB_BYTES_PER_PIXEL)

diff --git a/common/lz.c b/common/lz.c
index 87c13db..947facc 100644
--- a/common/lz.c
+++ b/common/lz.c
@@ -493,7 +493,7 @@ int lz_encode(LzContext *lz, LzImageType type, int width, int height, int top_do
     encoder->height = height;
     encoder->stride = stride;
 
-    if (IS_IMAGE_TYPE_PLT[encoder->type]) {
+    if (lz_image_type_is_plt(encoder->type)) {
         if (encoder->stride > (width / PLT_PIXELS_PER_BYTE[encoder->type])) {
             if (((width % PLT_PIXELS_PER_BYTE[encoder->type]) == 0) || (
                     (encoder->stride - (width / PLT_PIXELS_PER_BYTE[encoder->type])) > 1)) {
@@ -605,7 +605,7 @@ void lz_decode_begin(LzContext *lz, uint8_t *io_ptr, unsigned int num_io_bytes,
 
     // TODO: maybe instead of stride we can encode out_n_pixels
     //       (if stride is not necessary in decoding).
-    if (IS_IMAGE_TYPE_PLT[encoder->type]) {
+    if (lz_image_type_is_plt(encoder->type)) {
         encoder->palette = palette;
         *out_n_pixels = encoder->stride * PLT_PIXELS_PER_BYTE[encoder->type] * encoder->height;
     } else {
@@ -619,7 +619,7 @@ void lz_decode(LzContext *lz, LzImageType to_type, uint8_t *buf)
     size_t out_size = 0;
     size_t alpha_size = 0;
     size_t size = 0;
-    if (IS_IMAGE_TYPE_PLT[encoder->type]) {
+    if (lz_image_type_is_plt(encoder->type)) {
         if (to_type == encoder->type) {
             size = encoder->height * encoder->stride;
             out_size = lz_plt_decompress(encoder, (one_byte_pixel_t *)buf, size);
diff --git a/common/lz_common.h b/common/lz_common.h
index 78df003..d2f255e 100644
--- a/common/lz_common.h
+++ b/common/lz_common.h
@@ -23,7 +23,9 @@
 #ifndef _LZ_COMMON_H
 #define _LZ_COMMON_H
 
+#include <glib.h>
 #include <spice/macros.h>
+#include <stdbool.h>
 #include "verify.h"
 
 SPICE_BEGIN_DECLS
@@ -53,12 +55,21 @@ typedef enum {
 #define LZ_IMAGE_TYPE_LOG 4 // number of bits required for coding the image type
 
 /* access to the arrays is based on the image types */
-static const int IS_IMAGE_TYPE_PLT[] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0};
+static inline bool lz_image_type_is_plt(LzImageType type)
+{
+    static const int IS_IMAGE_TYPE_PLT[] = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0};
+    verify(SPICE_N_ELEMENTS(IS_IMAGE_TYPE_PLT) == (LZ_IMAGE_TYPE_A8 + 1));
+
+    g_return_val_if_fail(type >= LZ_IMAGE_TYPE_INVALID, false);
+    g_return_val_if_fail(type <= LZ_IMAGE_TYPE_A8, false);
+
+    return IS_IMAGE_TYPE_PLT[type];
+}
+
 static const int IS_IMAGE_TYPE_RGB[] = {0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1};
 static const int PLT_PIXELS_PER_BYTE[] = {0, 8, 8, 2, 2, 1};
 static const int RGB_BYTES_PER_PIXEL[] = {0, 1, 1, 1, 1, 1, 2, 3, 4, 4, 4, 1};
 
-verify(SPICE_N_ELEMENTS(IS_IMAGE_TYPE_PLT) == (LZ_IMAGE_TYPE_A8 + 1));
 verify(SPICE_N_ELEMENTS(IS_IMAGE_TYPE_RGB) == (LZ_IMAGE_TYPE_A8 + 1));
 verify(SPICE_N_ELEMENTS(PLT_PIXELS_PER_BYTE) == (LZ_IMAGE_TYPE_PLT8 + 1));
 verify(SPICE_N_ELEMENTS(RGB_BYTES_PER_PIXEL) == (LZ_IMAGE_TYPE_A8 + 1));


> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  common/lz.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> Changes since v3:
> - just fix the issue, without changing much the code/declarations
> 
> diff --git a/common/lz.c b/common/lz.c
> index b7e7d48..ff5f4ef 100644
> --- a/common/lz.c
> +++ b/common/lz.c
> @@ -594,6 +594,9 @@ void lz_decode_begin(LzContext *lz, uint8_t *io_ptr, unsigned int num_io_bytes,
>      }
>  
>      encoder->type = (LzImageType)decode_32(encoder);
> +    if (encoder->type < 0 || encoder->type > LZ_IMAGE_TYPE_A8) {
> +        encoder->usr->error(encoder->usr, "invalid lz type %d\n", encoder->type);
> +    }
>      encoder->width = decode_32(encoder);
>      encoder->height = decode_32(encoder);
>      encoder->stride = decode_32(encoder);
> -- 
> 2.17.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180703/9e2d7ee0/attachment.sig>