1. 13 2月, 2018 15 次提交
    • D
      ui: correctly advance output buffer when writing SASL data · 88ab8538
      Daniel P. Berrangé 提交于
      In this previous commit:
      
        commit 8f61f1c5
        Author: Daniel P. Berrange <berrange@redhat.com>
        Date:   Mon Dec 18 19:12:20 2017 +0000
      
          ui: track how much decoded data we consumed when doing SASL encoding
      
      I attempted to fix a flaw with tracking how much data had actually been
      processed when encoding with SASL. With that flaw, the VNC server could
      mistakenly discard queued data that had not been sent.
      
      The fix was not quite right though, because it merely decremented the
      vs->output.offset value. This is effectively discarding data from the
      end of the pending output buffer. We actually need to discard data from
      the start of the pending output buffer. We also want to free memory that
      is no longer required. The correct way to handle this is to use the
      buffer_advance() helper method instead of directly manipulating the
      offset value.
      Reported-by: NLaszlo Ersek <lersek@redhat.com>
      Signed-off-by: NDaniel P. Berrangé <berrange@redhat.com>
      Reviewed-by: NEric Blake <eblake@redhat.com>
      Reviewed-by: NLaszlo Ersek <lersek@redhat.com>
      Message-id: 20180201155841.27509-1-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit 627ebec2)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      88ab8538
    • D
      ui: avoid sign extension using client width/height · 64653b7f
      Daniel P. Berrange 提交于
      Pixman returns a signed int for the image width/height, but the VNC
      protocol only permits a unsigned int16. Effective framebuffer size
      is determined by the guest, limited by the video RAM size, so the
      dimensions are unlikely to exceed the range of an unsigned int16,
      but this is not currently validated.
      
      With the current use of 'int' for client width/height, the calculation
      of offsets in vnc_update_throttle_offset() suffers from integer size
      promotion and sign extension, causing coverity warnings
      
      *** CID 1385147:  Integer handling issues  (SIGN_EXTENSION)
      /ui/vnc.c: 979 in vnc_update_throttle_offset()
      973      * than that the client would already suffering awful audio
      974      * glitches, so dropping samples is no worse really).
      975      */
      976     static void vnc_update_throttle_offset(VncState *vs)
      977     {
      978         size_t offset =
      >>>     CID 1385147:  Integer handling issues  (SIGN_EXTENSION)
      >>>     Suspicious implicit sign extension:
          "vs->client_pf.bytes_per_pixel" with type "unsigned char" (8 bits,
          unsigned) is promoted in "vs->client_width * vs->client_height *
          vs->client_pf.bytes_per_pixel" to type "int" (32 bits, signed), then
          sign-extended to type "unsigned long" (64 bits, unsigned).  If
          "vs->client_width * vs->client_height * vs->client_pf.bytes_per_pixel"
          is greater than 0x7FFFFFFF, the upper bits of the result will all be 1.
      979             vs->client_width * vs->client_height * vs->client_pf.bytes_per_pixel;
      
      Change client_width / client_height to be a size_t to avoid sign
      extension and integer promotion. Then validate that dimensions are in
      range wrt the RFB protocol u16 limits.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Message-id: 20180118155254.17053-1-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit 4c956bd8)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      64653b7f
    • D
      ui: mix misleading comments & return types of VNC I/O helper methods · 9a26ca6b
      Daniel P. Berrange 提交于
      While the QIOChannel APIs for reading/writing data return ssize_t, with negative
      value indicating an error, the VNC code passes this return value through the
      vnc_client_io_error() method. This detects the error condition, disconnects the
      client and returns 0 to indicate error. Thus all the VNC helper methods should
      return size_t (unsigned), and misleading comments which refer to the possibility
      of negative return values need fixing.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Reviewed-by: NDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-id: 20171218191228.31018-14-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit 30b80fd5)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      9a26ca6b
    • D
      ui: add trace events related to VNC client throttling · 172f4e5a
      Daniel P. Berrange 提交于
      The VNC client throttling is quite subtle so will benefit from having trace
      points available for live debugging.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Reviewed-by: NDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-id: 20171218191228.31018-13-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit 6aa22a29)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      172f4e5a
    • D
      ui: place a hard cap on VNC server output buffer size · 0c85a40e
      Daniel P. Berrange 提交于
      The previous patches fix problems with throttling of forced framebuffer updates
      and audio data capture that would cause the QEMU output buffer size to grow
      without bound. Those fixes are graceful in that once the client catches up with
      reading data from the server, everything continues operating normally.
      
      There is some data which the server sends to the client that is impractical to
      throttle. Specifically there are various pseudo framebuffer update encodings to
      inform the client of things like desktop resizes, pointer changes, audio
      playback start/stop, LED state and so on. These generally only involve sending
      a very small amount of data to the client, but a malicious guest might be able
      to do things that trigger these changes at a very high rate. Throttling them is
      not practical as missed or delayed events would cause broken behaviour for the
      client.
      
      This patch thus takes a more forceful approach of setting an absolute upper
      bound on the amount of data we permit to be present in the output buffer at
      any time. The previous patch set a threshold for throttling the output buffer
      by allowing an amount of data equivalent to one complete framebuffer update and
      one seconds worth of audio data. On top of this it allowed for one further
      forced framebuffer update to be queued.
      
      To be conservative, we thus take that throttling threshold and multiply it by
      5 to form an absolute upper bound. If this bound is hit during vnc_write() we
      forceably disconnect the client, refusing to queue further data. This limit is
      high enough that it should never be hit unless a malicious client is trying to
      exploit the sever, or the network is completely saturated preventing any sending
      of data on the socket.
      
      This completes the fix for CVE-2017-15124 started in the previous patches.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Reviewed-by: NDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-id: 20171218191228.31018-12-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit f887cf16)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      0c85a40e
    • D
      ui: fix VNC client throttling when forced update is requested · f9e53c77
      Daniel P. Berrange 提交于
      The VNC server must throttle data sent to the client to prevent the 'output'
      buffer size growing without bound, if the client stops reading data off the
      socket (either maliciously or due to stalled/slow network connection).
      
      The current throttling is very crude because it simply checks whether the
      output buffer offset is zero. This check is disabled if the client has requested
      a forced update, because we want to send these as soon as possible.
      
      As a result, the VNC client can cause QEMU to allocate arbitrary amounts of RAM.
      They can first start something in the guest that triggers lots of framebuffer
      updates eg play a youtube video. Then repeatedly send full framebuffer update
      requests, but never read data back from the server. This can easily make QEMU's
      VNC server send buffer consume 100MB of RAM per second, until the OOM killer
      starts reaping processes (hopefully the rogue QEMU process, but it might pick
      others...).
      
      To address this we make the throttling more intelligent, so we can throttle
      full updates. When we get a forced update request, we keep track of exactly how
      much data we put on the output buffer. We will not process a subsequent forced
      update request until this data has been fully sent on the wire. We always allow
      one forced update request to be in flight, regardless of what data is queued
      for incremental updates or audio data. The slight complication is that we do
      not initially know how much data an update will send, as this is done in the
      background by the VNC job thread. So we must track the fact that the job thread
      has an update pending, and not process any further updates until this job is
      has been completed & put data on the output buffer.
      
      This unbounded memory growth affects all VNC server configurations supported by
      QEMU, with no workaround possible. The mitigating factor is that it can only be
      triggered by a client that has authenticated with the VNC server, and who is
      able to trigger a large quantity of framebuffer updates or audio samples from
      the guest OS. Mostly they'll just succeed in getting the OOM killer to kill
      their own QEMU process, but its possible other processes can get taken out as
      collateral damage.
      
      This is a more general variant of the similar unbounded memory usage flaw in
      the websockets server, that was previously assigned CVE-2017-15268, and fixed
      in 2.11 by:
      
        commit a7b20a8e
        Author: Daniel P. Berrange <berrange@redhat.com>
        Date:   Mon Oct 9 14:43:42 2017 +0100
      
          io: monitor encoutput buffer size from websocket GSource
      
      This new general memory usage flaw has been assigned CVE-2017-15124, and is
      partially fixed by this patch.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Reviewed-by: NDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-id: 20171218191228.31018-11-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit ada8d2e4)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      f9e53c77
    • D
      ui: fix VNC client throttling when audio capture is active · f9c87678
      Daniel P. Berrange 提交于
      The VNC server must throttle data sent to the client to prevent the 'output'
      buffer size growing without bound, if the client stops reading data off the
      socket (either maliciously or due to stalled/slow network connection).
      
      The current throttling is very crude because it simply checks whether the
      output buffer offset is zero. This check must be disabled if audio capture is
      enabled, because when streaming audio the output buffer offset will rarely be
      zero due to queued audio data, and so this would starve framebuffer updates.
      
      As a result, the VNC client can cause QEMU to allocate arbitrary amounts of RAM.
      They can first start something in the guest that triggers lots of framebuffer
      updates eg play a youtube video. Then enable audio capture, and simply never
      read data back from the server. This can easily make QEMU's VNC server send
      buffer consume 100MB of RAM per second, until the OOM killer starts reaping
      processes (hopefully the rogue QEMU process, but it might pick others...).
      
      To address this we make the throttling more intelligent, so we can throttle
      when audio capture is active too. To determine how to throttle incremental
      updates or audio data, we calculate a size threshold. Normally the threshold is
      the approximate number of bytes associated with a single complete framebuffer
      update. ie width * height * bytes per pixel. We'll send incremental updates
      until we hit this threshold, at which point we'll stop sending updates until
      data has been written to the wire, causing the output buffer offset to fall
      back below the threshold.
      
      If audio capture is enabled, we increase the size of the threshold to also
      allow for upto 1 seconds worth of audio data samples. ie nchannels * bytes
      per sample * frequency. This allows the output buffer to have a mixture of
      incremental framebuffer updates and audio data queued, but once the threshold
      is exceeded, audio data will be dropped and incremental updates will be
      throttled.
      
      This unbounded memory growth affects all VNC server configurations supported by
      QEMU, with no workaround possible. The mitigating factor is that it can only be
      triggered by a client that has authenticated with the VNC server, and who is
      able to trigger a large quantity of framebuffer updates or audio samples from
      the guest OS. Mostly they'll just succeed in getting the OOM killer to kill
      their own QEMU process, but its possible other processes can get taken out as
      collateral damage.
      
      This is a more general variant of the similar unbounded memory usage flaw in
      the websockets server, that was previously assigned CVE-2017-15268, and fixed
      in 2.11 by:
      
        commit a7b20a8e
        Author: Daniel P. Berrange <berrange@redhat.com>
        Date:   Mon Oct 9 14:43:42 2017 +0100
      
          io: monitor encoutput buffer size from websocket GSource
      
      This new general memory usage flaw has been assigned CVE-2017-15124, and is
      partially fixed by this patch.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Reviewed-by: NDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-id: 20171218191228.31018-10-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit e2b72cb6)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      f9c87678
    • D
      ui: refactor code for determining if an update should be sent to the client · 5af9f250
      Daniel P. Berrange 提交于
      The logic for determining if it is possible to send an update to the client
      will become more complicated shortly, so pull it out into a separate method
      for easier extension later.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Reviewed-by: NDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-id: 20171218191228.31018-9-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit 0bad8342)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      5af9f250
    • D
      ui: correctly reset framebuffer update state after processing dirty regions · 2e6571e6
      Daniel P. Berrange 提交于
      According to the RFB protocol, a client sends one or more framebuffer update
      requests to the server. The server can reply with a single framebuffer update
      response, that covers all previously received requests. Once the client has
      read this update from the server, it may send further framebuffer update
      requests to monitor future changes. The client is free to delay sending the
      framebuffer update request if it needs to throttle the amount of data it is
      reading from the server.
      
      The QEMU VNC server, however, has never correctly handled the framebuffer
      update requests. Once QEMU has received an update request, it will continue to
      send client updates forever, even if the client hasn't asked for further
      updates. This prevents the client from throttling back data it gets from the
      server. This change fixes the flawed logic such that after a set of updates are
      sent out, QEMU waits for a further update request before sending more data.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Reviewed-by: NDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-id: 20171218191228.31018-8-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit 728a7ac9)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      2e6571e6
    • D
      ui: introduce enum to track VNC client framebuffer update request state · 126617e6
      Daniel P. Berrange 提交于
      Currently the VNC servers tracks whether a client has requested an incremental
      or forced update with two boolean flags. There are only really 3 distinct
      states to track, so create an enum to more accurately reflect permitted states.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Reviewed-by: NDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-id: 20171218191228.31018-7-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit fef1bbad)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      126617e6
    • D
      ui: track how much decoded data we consumed when doing SASL encoding · 8a9c5c34
      Daniel P. Berrange 提交于
      When we encode data for writing with SASL, we encode the entire pending output
      buffer. The subsequent write, however, may not be able to send the full encoded
      data in one go though, particularly with a slow network. So we delay setting the
      output buffer offset back to zero until all the SASL encoded data is sent.
      
      Between encoding the data and completing sending of the SASL encoded data,
      however, more data might have been placed on the pending output buffer. So it
      is not valid to set offset back to zero. Instead we must keep track of how much
      data we consumed during encoding and subtract only that amount.
      
      With the current bug we would be throwing away some pending data without having
      sent it at all. By sheer luck this did not previously cause any serious problem
      because appending data to the send buffer is always an atomic action, so we
      only ever throw away complete RFB protocol messages. In the case of frame buffer
      updates we'd catch up fairly quickly, so no obvious problem was visible.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Reviewed-by: NDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-id: 20171218191228.31018-6-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit 8f61f1c5)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      8a9c5c34
    • D
      ui: avoid pointless VNC updates if framebuffer isn't dirty · 616d64ac
      Daniel P. Berrange 提交于
      The vnc_update_client() method checks the 'has_dirty' flag to see if there are
      dirty regions that are pending to send to the client. Regardless of this flag,
      if a forced update is requested, updates must be sent. For unknown reasons
      though, the code also tries to sent updates if audio capture is enabled. This
      makes no sense as audio capture state does not impact framebuffer contents, so
      this check is removed.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Reviewed-by: NDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-id: 20171218191228.31018-5-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit 3541b084)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      616d64ac
    • D
      ui: remove redundant indentation in vnc_client_update · a7b2537f
      Daniel P. Berrange 提交于
      Now that previous dead / unreachable code has been removed, we can simplify
      the indentation in the vnc_client_update method.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Reviewed-by: NDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-id: 20171218191228.31018-4-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit b939eb89)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      a7b2537f
    • D
      ui: remove unreachable code in vnc_update_client · de1e7a91
      Daniel P. Berrange 提交于
      A previous commit:
      
        commit 5a8be0f7
        Author: Gerd Hoffmann <kraxel@redhat.com>
        Date:   Wed Jul 13 12:21:20 2016 +0200
      
          vnc: make sure we finish disconnect
      
      Added a check for vs->disconnecting at the very start of the
      vnc_update_client method. This means that the very next "if"
      statement check for !vs->disconnecting always evaluates true,
      and is thus redundant. This in turn means the vs->disconnecting
      check at the very end of the method never evaluates true, and
      is thus unreachable code.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Reviewed-by: NDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-id: 20171218191228.31018-3-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit c53df961)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      de1e7a91
    • D
      ui: remove 'sync' parameter from vnc_update_client · 0181686a
      Daniel P. Berrange 提交于
      There is only one caller of vnc_update_client and that always passes false
      for the 'sync' parameter.
      Signed-off-by: NDaniel P. Berrange <berrange@redhat.com>
      Reviewed-by: NDarren Kenny <darren.kenny@oracle.com>
      Reviewed-by: NMarc-André Lureau <marcandre.lureau@redhat.com>
      Message-id: 20171218191228.31018-2-berrange@redhat.com
      Signed-off-by: NGerd Hoffmann <kraxel@redhat.com>
      (cherry picked from commit 6af998db)
      Signed-off-by: NMichael Roth <mdroth@linux.vnet.ibm.com>
      0181686a
  2. 12 2月, 2018 7 次提交
  3. 06 2月, 2018 18 次提交