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 <ler...@redhat.com> > Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4...@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));
>
signature.asc
Description: OpenPGP digital signature
