patch 9.2.0843: [security]: popup: opacity mask indexed out of bounds

Problem:  [security]: A "clipwindow" popup with "opacity" anchored to a
          text property can keep a negative window row when the anchor
          scrolls above the top of the host window.  The opacity mask
          loop then indexes the screen array before its start, causing
          an out-of-bounds read and a conditional out-of-bounds write
          (tdjackey).
Solution: Keep the popup window row at the first visible row and record
          the clipped-off top rows only in the top offset, so no
          consumer has to clamp the row and the opacity mask loop stays
          in bounds (Hirohito Higashi).

Github Security Advisory:
https://github.com/vim/vim/security/advisories/GHSA-pmvp-6rcj-98p4

Signed-off-by: Hirohito Higashi <h.east.727@gmail.com>
Signed-off-by: Christian Brabandt <cb@256bit.org>

diff --git a/src/popupwin.c b/src/popupwin.c
index 0acb7453df95d54b587692d7c64668e4207e98d6..293305ed92f5fdd5b4c5918844941cb8a227d969 100644 (file)
--- a/src/popupwin.c
+++ b/src/popupwin.c
@@ -2313,10 +2313,8 @@ popup_apply_winupdate_clip(win_T *wp, popup_clip_T *cl)
        if (wp->w_height < 0)
            wp->w_height = 0;
        if (cl->clip_top_content > 0)
-       {
+           // w_winrow already points at the first visible row.
            wp->w_topline += cl->clip_top_content;
-           wp->w_winrow += cl->clip_top_content;
-       }
     }
     if (wp->w_popup_leftclip > 0 || wp->w_popup_rightclip > 0)
     {
@@ -2982,6 +2980,10 @@ popup_adjust_position(win_T *wp)
        return;
     }
 
+    // Make w_winrow the first visible screen row (>= 0); the clipped-off top
+    // rows are recorded in w_popup_topoff.
+    wp->w_winrow += wp->w_popup_topoff;
+
 #ifdef FEAT_IMAGE_SIXEL
     // Final winrow is now known: encode (or re-encode) the sixel image so it
     // fits within the screen and does not trigger sixel-scroll on terminals.
@@ -6120,8 +6122,11 @@ popup_mark_opacity_zindex(win_T *wp)
 
     width = popup_width(wp);
     height = popup_height(wp);
+    // w_winrow/w_wincol are visible cells (>= 0); w_popup_topoff rows are
+    // clipped off the top.
     for (r = wp->w_winrow;
-                      r < wp->w_winrow + height && r < screen_Rows; ++r)
+                      r < wp->w_winrow + height - wp->w_popup_topoff
+                                               && r < screen_Rows; ++r)
        for (c = wp->w_wincol;
                 c < wp->w_wincol + width - wp->w_popup_leftoff
                                                && c < screen_Columns; ++c)
@@ -6417,7 +6422,7 @@ may_update_popup_mask(int type)
            continue;
 
        {
-           int mask_start = wp->w_winrow + wp->w_popup_topoff;
+           int mask_start = wp->w_winrow;
            int mask_end = mask_start + height;
            int mask_col_start = wp->w_wincol + wp->w_popup_leftclip;
            int mask_col_end = wp->w_wincol + width - wp->w_popup_leftoff
@@ -7453,7 +7458,8 @@ update_popups(void (*win_update)(win_T *wp))
                                                        - wp->w_popup_leftoff;
        if (wp->w_wincol + left_extra < 0)
            left_extra = -wp->w_wincol;
-       wp->w_winrow += top_off;
+       // Move to the content top, skipping any clipped top border/padding.
+       wp->w_winrow += MAX(top_off - wp->w_popup_topoff, 0);
        wp->w_wincol += left_extra;
 
        // Draw the popup text, unless it's off screen.
@@ -7486,15 +7492,9 @@ update_popups(void (*win_update)(win_T *wp))
                wp->w_cursor.lnum = wp->w_botline - 1;
        }
 
-       wp->w_winrow -= top_off;
+       wp->w_winrow -= MAX(top_off - wp->w_popup_topoff, 0);
        wp->w_wincol -= left_extra;
 
-       // "clipwindow" with top-clip shifts all popup decorations down so the
-       // first visible row of the popup lands at the host window's top edge.
-       // Apply the shift before drawing borders/padding/etc. and restore at
-       // the end of this popup's iteration.
-       wp->w_winrow += wp->w_popup_topoff;
-
        // Add offset for border and padding if not done already.
        if ((wp->w_flags & WFLAG_WCOL_OFF_ADDED) == 0)
        {
@@ -7896,10 +7896,6 @@ update_popups(void (*win_update)(win_T *wp))
        if (override_success)
            pop_highlight_overrides();
 
-       // Undo the topoff shift applied before drawing the borders so the
-       // next iteration sees the popup's logical winrow.
-       wp->w_winrow -= wp->w_popup_topoff;
-
 #ifdef FEAT_IMAGE
        // Emit the popup image right after this popup's decorations land in
        // ScreenLines.  Popups are walked in zindex order, so a higher

diff --git a/src/testdir/test_popupwin.vim b/src/testdir/test_popupwin.vim
index 364a50fe02ccf811858c90fb274f3b16ae96bc54..6aed7be3ecc097dca68318d4f1e73289a5eb79c1 100644 (file)
--- a/src/testdir/test_popupwin.vim
+++ b/src/testdir/test_popupwin.vim
@@ -4685,6 +4685,37 @@ func Test_popup_clipwindow_hide_when_prop_off_screen()
   call prop_type_delete('clipprop')
 endfunc
 
+func Test_popup_clipwindow_opacity_negative_winrow()
+  " A "clipwindow" popup with "opacity" whose textprop anchor scrolls above
+  " the window top must not index the opacity mask out of bounds.
+  call prop_type_add('clipprop', {})
+  new
+  call setline(1, range(1, 200)->mapnew({_, v -> 'line ' .. v}))
+  call prop_add(5, 1, #{type: 'clipprop', length: 5})
+  let host = win_getid()
+
+  let id = popup_create(['aaa', 'bbb', 'ccc', 'ddd', 'eee'], #{
+        \ textprop: 'clipprop',
+        \ textpropwin: host,
+        \ wrap: v:false,
+        \ fixed: v:true,
+        \ clipwindow: v:true,
+        \ opacity: 50,
+        \ })
+  call assert_true(id > 0)
+  redraw
+
+  " Scroll so the prop (line 5) sits a couple of lines above the top, so the
+  " popup is clipped at the host window's top edge.
+  call win_execute(host, 'normal! 8Gzt')
+  redraw
+  redraw
+
+  call popup_close(id)
+  bwipe!
+  call prop_type_delete('clipprop')
+endfunc
+
 func Test_popup_clipwindow_top_clip()
   CheckScreendump
 

diff --git a/src/version.c b/src/version.c
index a5cca3061b8acf97b5473588b02940ad94a32620..a9bcdc88ecdf90d3313aae0dd1cb1dddc6c1b354 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -758,6 +758,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    843,
 /**/
     842,
 /**/