In the 2.11 release we fixed CVE-2017-15268, which allowed the VNC websockets
server to consume arbitrary memory when a slow client was connected. I have
since discovered that this same type of problem can be triggered in several
other ways in the regular (non-websockets) VNC server. This patch series
attempts to fix this problem by limiting framebuffer updates and other data
sent from server to client. The mitigating factor is that you need to have
successfully authenticated with the VNC server to trigger these new flaws.
This new more general flaw is assigned CVE-2017-15124 by the Red Hat security
team.

The key patches containing the security fix are 9, 10, 11.

Since this code is incredibly subtle & hard to understand though, the first
8 patches do a bunch of independant cleanups/refactoring to make the security
fixes clearer.  The last two patches are just some extra cleanup / help for
future maint.

Daniel P. Berrange (13):
  ui: remove 'sync' parametr from vnc_update_client
  ui: remove unreachable code in vnc_update_client
  ui: remove redundant indentation in vnc_client_update
  ui: avoid pointless VNC updates if framebuffer isn't dirty
  ui: track how much decoded data we consumed when doing SASL encoding
  ui: introduce enum to track VNC client framebuffer update request
    state
  ui: correctly reset framebuffer update state after processing dirty
    regions
  ui: refactor code for determining if an update should be sent to the
    client
  ui: fix VNC client throttling when audio capture is active
  ui: fix VNC client throttling when forced update is requested
  ui: place a hard cap on VNC server output buffer size
  ui: add trace events related to VNC client throttling
  ui: mix misleading comments & return types of VNC I/O helper methods

 ui/trace-events    |   7 ++
 ui/vnc-auth-sasl.c |  16 ++-
 ui/vnc-auth-sasl.h |   5 +-
 ui/vnc-jobs.c      |   5 +
 ui/vnc.c           | 320 ++++++++++++++++++++++++++++++++++++++---------------
 ui/vnc.h           |  28 ++++-
 6 files changed, 277 insertions(+), 104 deletions(-)

-- 
2.14.3

Hi Daniel,

For the series:

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

With one small nit on patch 1.

Thanks,

Darren.

On Mon, Dec 18, 2017 at 07:12:15PM +0000, Daniel P. Berrange wrote:
>In the 2.11 release we fixed CVE-2017-15268, which allowed the VNC websockets
>server to consume arbitrary memory when a slow client was connected. I have
>since discovered that this same type of problem can be triggered in several
>other ways in the regular (non-websockets) VNC server. This patch series
>attempts to fix this problem by limiting framebuffer updates and other data
>sent from server to client. The mitigating factor is that you need to have
>successfully authenticated with the VNC server to trigger these new flaws.
>This new more general flaw is assigned CVE-2017-15124 by the Red Hat security
>team.
>
>The key patches containing the security fix are 9, 10, 11.
>
>Since this code is incredibly subtle & hard to understand though, the first
>8 patches do a bunch of independant cleanups/refactoring to make the security
>fixes clearer.  The last two patches are just some extra cleanup / help for
>future maint.
>
>Daniel P. Berrange (13):
>  ui: remove 'sync' parametr from vnc_update_client
>  ui: remove unreachable code in vnc_update_client
>  ui: remove redundant indentation in vnc_client_update
>  ui: avoid pointless VNC updates if framebuffer isn't dirty
>  ui: track how much decoded data we consumed when doing SASL encoding
>  ui: introduce enum to track VNC client framebuffer update request
>    state
>  ui: correctly reset framebuffer update state after processing dirty
>    regions
>  ui: refactor code for determining if an update should be sent to the
>    client
>  ui: fix VNC client throttling when audio capture is active
>  ui: fix VNC client throttling when forced update is requested
>  ui: place a hard cap on VNC server output buffer size
>  ui: add trace events related to VNC client throttling
>  ui: mix misleading comments & return types of VNC I/O helper methods
>
> ui/trace-events    |   7 ++
> ui/vnc-auth-sasl.c |  16 ++-
> ui/vnc-auth-sasl.h |   5 +-
> ui/vnc-jobs.c      |   5 +
> ui/vnc.c           | 320 ++++++++++++++++++++++++++++++++++++++---------------
> ui/vnc.h           |  28 ++++-
> 6 files changed, 277 insertions(+), 104 deletions(-)
>
>-- 
>2.14.3
>
>

On Mon, Dec 18, 2017 at 8:12 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
> In the 2.11 release we fixed CVE-2017-15268, which allowed the VNC websockets
> server to consume arbitrary memory when a slow client was connected. I have
> since discovered that this same type of problem can be triggered in several
> other ways in the regular (non-websockets) VNC server. This patch series
> attempts to fix this problem by limiting framebuffer updates and other data
> sent from server to client. The mitigating factor is that you need to have
> successfully authenticated with the VNC server to trigger these new flaws.
> This new more general flaw is assigned CVE-2017-15124 by the Red Hat security
> team.
>
> The key patches containing the security fix are 9, 10, 11.
>
> Since this code is incredibly subtle & hard to understand though, the first
> 8 patches do a bunch of independant cleanups/refactoring to make the security
> fixes clearer.  The last two patches are just some extra cleanup / help for
> future maint.
>
> Daniel P. Berrange (13):
>   ui: remove 'sync' parametr from vnc_update_client
>   ui: remove unreachable code in vnc_update_client
>   ui: remove redundant indentation in vnc_client_update
>   ui: avoid pointless VNC updates if framebuffer isn't dirty
>   ui: track how much decoded data we consumed when doing SASL encoding
>   ui: introduce enum to track VNC client framebuffer update request
>     state
>   ui: correctly reset framebuffer update state after processing dirty
>     regions
>   ui: refactor code for determining if an update should be sent to the
>     client
>   ui: fix VNC client throttling when audio capture is active
>   ui: fix VNC client throttling when forced update is requested
>   ui: place a hard cap on VNC server output buffer size
>   ui: add trace events related to VNC client throttling
>   ui: mix misleading comments & return types of VNC I/O helper methods
>
>  ui/trace-events    |   7 ++
>  ui/vnc-auth-sasl.c |  16 ++-
>  ui/vnc-auth-sasl.h |   5 +-
>  ui/vnc-jobs.c      |   5 +
>  ui/vnc.c           | 320 ++++++++++++++++++++++++++++++++++++++---------------
>  ui/vnc.h           |  28 ++++-
>  6 files changed, 277 insertions(+), 104 deletions(-)
>

For the series:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>




-- 
Marc-André Lureau

Hi

----- Original Message -----
> In the 2.11 release we fixed CVE-2017-15268, which allowed the VNC websockets
> server to consume arbitrary memory when a slow client was connected. I have
> since discovered that this same type of problem can be triggered in several
> other ways in the regular (non-websockets) VNC server. This patch series
> attempts to fix this problem by limiting framebuffer updates and other data
> sent from server to client. The mitigating factor is that you need to have
> successfully authenticated with the VNC server to trigger these new flaws.
> This new more general flaw is assigned CVE-2017-15124 by the Red Hat security
> team.
> 
> The key patches containing the security fix are 9, 10, 11.
> 
> Since this code is incredibly subtle & hard to understand though, the first
> 8 patches do a bunch of independant cleanups/refactoring to make the security
> fixes clearer.  The last two patches are just some extra cleanup / help for
> future maint.

I already reviewed the series privately, and in general, I'd ack it.

Did you manage to run some throttle tests? (I tried to trigger them manually by slowing artificially network or calling framebuffer_update_request() in gdb, I didn't manage yet :-/)

> 
> Daniel P. Berrange (13):
>   ui: remove 'sync' parametr from vnc_update_client
>   ui: remove unreachable code in vnc_update_client
>   ui: remove redundant indentation in vnc_client_update
>   ui: avoid pointless VNC updates if framebuffer isn't dirty
>   ui: track how much decoded data we consumed when doing SASL encoding
>   ui: introduce enum to track VNC client framebuffer update request
>     state
>   ui: correctly reset framebuffer update state after processing dirty
>     regions
>   ui: refactor code for determining if an update should be sent to the
>     client
>   ui: fix VNC client throttling when audio capture is active
>   ui: fix VNC client throttling when forced update is requested
>   ui: place a hard cap on VNC server output buffer size
>   ui: add trace events related to VNC client throttling
>   ui: mix misleading comments & return types of VNC I/O helper methods
> 
>  ui/trace-events    |   7 ++
>  ui/vnc-auth-sasl.c |  16 ++-
>  ui/vnc-auth-sasl.h |   5 +-
>  ui/vnc-jobs.c      |   5 +
>  ui/vnc.c           | 320
>  ++++++++++++++++++++++++++++++++++++++---------------
>  ui/vnc.h           |  28 ++++-
>  6 files changed, 277 insertions(+), 104 deletions(-)
> 

Series:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> --
> 2.14.3
> 
> 

On Tue, Dec 19, 2017 at 09:57:59AM -0500, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > In the 2.11 release we fixed CVE-2017-15268, which allowed the VNC websockets
> > server to consume arbitrary memory when a slow client was connected. I have
> > since discovered that this same type of problem can be triggered in several
> > other ways in the regular (non-websockets) VNC server. This patch series
> > attempts to fix this problem by limiting framebuffer updates and other data
> > sent from server to client. The mitigating factor is that you need to have
> > successfully authenticated with the VNC server to trigger these new flaws.
> > This new more general flaw is assigned CVE-2017-15124 by the Red Hat security
> > team.
> > 
> > The key patches containing the security fix are 9, 10, 11.
> > 
> > Since this code is incredibly subtle & hard to understand though, the first
> > 8 patches do a bunch of independant cleanups/refactoring to make the security
> > fixes clearer.  The last two patches are just some extra cleanup / help for
> > future maint.
> 
> I already reviewed the series privately, and in general, I'd ack it.
> 
> Did you manage to run some throttle tests? (I tried to trigger them manually
> by slowing artificially network or calling framebuffer_update_request() in
> gdb, I didn't manage yet :-/)

I used two approaches. First a MITM socket server that just throttles the
rate at which is reads from QEMU. Second, I wrote an intentionally malicious
VNC client to artificially exercise the codepaths & verify they are fixed.

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 :|