The 'vs->as.freq' value is a signed integer, which is read from an
unsigned 32-bit int field on the wire. There is thus a risk of overflow
on 32-bit platforms. Move the frequency limit checking to be done at
time of read before casting to a signed integer.

Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index e14e524764..79ac9eccde 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -982,14 +982,7 @@ static void vnc_update_throttle_offset(VncState *vs)
         vs->client_width * vs->client_height * vs->client_pf.bytes_per_pixel;
 
     if (vs->audio_cap) {
-        int freq = vs->as.freq;
-        /* We don't limit freq when reading settings from client, so
-         * it could be upto MAX_INT in size. 48khz is a sensible
-         * upper bound for trustworthy clients */
         int bps;
-        if (freq > 48000) {
-            freq = 48000;
-        }
         switch (vs->as.fmt) {
         default:
         case  AUD_FMT_U8:
@@ -1005,7 +998,7 @@ static void vnc_update_throttle_offset(VncState *vs)
             bps = 4;
             break;
         }
-        offset += freq * bps * vs->as.nchannels;
+        offset += vs->as.freq * bps * vs->as.nchannels;
     }
 
     /* Put a floor of 1MB on offset, so that if we have a large pending
@@ -2279,6 +2272,7 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
 {
     int i;
     uint16_t limit;
+    uint32_t freq;
     VncDisplay *vd = vs->vd;
 
     if (data[0] > 3) {
@@ -2398,7 +2392,17 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
                     vnc_client_error(vs);
                     break;
                 }
-                vs->as.freq = read_u32(data, 6);
+                freq = read_u32(data, 6);
+                /* No official limit for protocol, but 48khz is a sensible
+                 * upper bound for trustworthy clients, and this limit
+                 * protects calculations involving 'vs->as.freq' later.
+                 */
+                if (freq > 48000) {
+                    VNC_DEBUG("Invalid audio frequency %u > 48000", freq);
+                    vnc_client_error(vs);
+                    break;
+                }
+                vs->as.freq = freq;
                 break;
             default:
                 VNC_DEBUG("Invalid audio message %d\n", read_u8(data, 4));
-- 
2.14.3

On 02/05/18 12:49, Daniel P. Berrangé wrote:
> The 'vs->as.freq' value is a signed integer, which is read from an
> unsigned 32-bit int field on the wire. There is thus a risk of overflow
> on 32-bit platforms. Move the frequency limit checking to be done at
> time of read before casting to a signed integer.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  ui/vnc.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index e14e524764..79ac9eccde 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -982,14 +982,7 @@ static void vnc_update_throttle_offset(VncState *vs)
>          vs->client_width * vs->client_height * vs->client_pf.bytes_per_pixel;
>  
>      if (vs->audio_cap) {
> -        int freq = vs->as.freq;
> -        /* We don't limit freq when reading settings from client, so
> -         * it could be upto MAX_INT in size. 48khz is a sensible
> -         * upper bound for trustworthy clients */
>          int bps;
> -        if (freq > 48000) {
> -            freq = 48000;
> -        }
>          switch (vs->as.fmt) {
>          default:
>          case  AUD_FMT_U8:
> @@ -1005,7 +998,7 @@ static void vnc_update_throttle_offset(VncState *vs)
>              bps = 4;
>              break;
>          }
> -        offset += freq * bps * vs->as.nchannels;
> +        offset += vs->as.freq * bps * vs->as.nchannels;
>      }
>  
>      /* Put a floor of 1MB on offset, so that if we have a large pending
> @@ -2279,6 +2272,7 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
>  {
>      int i;
>      uint16_t limit;
> +    uint32_t freq;
>      VncDisplay *vd = vs->vd;
>  
>      if (data[0] > 3) {
> @@ -2398,7 +2392,17 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
>                      vnc_client_error(vs);
>                      break;
>                  }
> -                vs->as.freq = read_u32(data, 6);
> +                freq = read_u32(data, 6);
> +                /* No official limit for protocol, but 48khz is a sensible
> +                 * upper bound for trustworthy clients, and this limit
> +                 * protects calculations involving 'vs->as.freq' later.
> +                 */
> +                if (freq > 48000) {
> +                    VNC_DEBUG("Invalid audio frequency %u > 48000", freq);
> +                    vnc_client_error(vs);
> +                    break;
> +                }
> +                vs->as.freq = freq;
>                  break;
>              default:
>                  VNC_DEBUG("Invalid audio message %d\n", read_u8(data, 4));
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

On 02/05/2018 08:49 AM, Daniel P. Berrangé wrote:
> The 'vs->as.freq' value is a signed integer, which is read from an
> unsigned 32-bit int field on the wire. There is thus a risk of overflow
> on 32-bit platforms. Move the frequency limit checking to be done at
> time of read before casting to a signed integer.
> 
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  ui/vnc.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index e14e524764..79ac9eccde 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -982,14 +982,7 @@ static void vnc_update_throttle_offset(VncState *vs)
>          vs->client_width * vs->client_height * vs->client_pf.bytes_per_pixel;
>  
>      if (vs->audio_cap) {
> -        int freq = vs->as.freq;
> -        /* We don't limit freq when reading settings from client, so
> -         * it could be upto MAX_INT in size. 48khz is a sensible
> -         * upper bound for trustworthy clients */
>          int bps;
> -        if (freq > 48000) {
> -            freq = 48000;
> -        }
>          switch (vs->as.fmt) {
>          default:
>          case  AUD_FMT_U8:
> @@ -1005,7 +998,7 @@ static void vnc_update_throttle_offset(VncState *vs)
>              bps = 4;
>              break;
>          }
> -        offset += freq * bps * vs->as.nchannels;
> +        offset += vs->as.freq * bps * vs->as.nchannels;
>      }
>  
>      /* Put a floor of 1MB on offset, so that if we have a large pending
> @@ -2279,6 +2272,7 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
>  {
>      int i;
>      uint16_t limit;
> +    uint32_t freq;
>      VncDisplay *vd = vs->vd;
>  
>      if (data[0] > 3) {
> @@ -2398,7 +2392,17 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
>                      vnc_client_error(vs);
>                      break;
>                  }
> -                vs->as.freq = read_u32(data, 6);
> +                freq = read_u32(data, 6);
> +                /* No official limit for protocol, but 48khz is a sensible
> +                 * upper bound for trustworthy clients, and this limit
> +                 * protects calculations involving 'vs->as.freq' later.
> +                 */
> +                if (freq > 48000) {
> +                    VNC_DEBUG("Invalid audio frequency %u > 48000", freq);
> +                    vnc_client_error(vs);
> +                    break;
> +                }
> +                vs->as.freq = freq;
>                  break;
>              default:
>                  VNC_DEBUG("Invalid audio message %d\n", read_u8(data, 4));
>