[ros-diffs] [reactos] 01/01: [NTUSER] Scrollbar.c, Avoid potential out-of-bounds-accesses in co_IntSetScrollInfo() CORE-17777

[Joachim Henze]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=222acf5a3ed11f74adbe0…

commit 222acf5a3ed11f74adbe0d1bdee527447fd81bbf
Author:     Joachim Henze <Joachim.Henze(a)reactos.org>
AuthorDate: Mon Sep 20 03:05:05 2021 +0200
Commit:     Joachim Henze <Joachim.Henze(a)reactos.org>
CommitDate: Mon Sep 20 03:05:05 2021 +0200

    [NTUSER] Scrollbar.c, Avoid potential out-of-bounds-accesses in co_IntSetScrollInfo()
CORE-17777
    
    This is an addendum to
    0.4.15-dev-3174-g dda9c3979e87223b66e61f0f936f46920221e509 CORE-17769 and
    0.4.15-dev-3147-g 3bf7e3ac13c5c9df373827c102b763b5b9822204 CORE-17754 CORE-17755
    
    We have not seen this happening in real-life yet, but some code-fragments within
co_IntSetScrollInfo()
    e.g. line 628 if (nBar == SB_CTL) do clearly indicate that nBar can be 2 (SB_CTL).
    Some lines below we definitely must not access those 4 static arrays out of bounds
then via nBar as access index!
    
    Ftr with a bit of grepping I also found some calls like NtUserSetScrollInfo(Wnd,
SB_CTL, &Info, FALSE);
    e.g: in win32ss/user/user32/controls/scrollbar.c so I am pretty sure nBar == 2 can
happen in practice within co_IntSetScrollInfo().
    
    I question whether any of those reads/writes to those static arrays (or the
comparisons) would make any sense on index 2,
    so we should aim to eliminate them altogether in the future.
---
 win32ss/user/ntuser/scrollbar.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/win32ss/user/ntuser/scrollbar.c b/win32ss/user/ntuser/scrollbar.c
index eba1b41f666..e4e0ba436af 100644
--- a/win32ss/user/ntuser/scrollbar.c
+++ b/win32ss/user/ntuser/scrollbar.c
@@ -492,15 +492,15 @@ co_IntSetScrollInfo(PWND Window, INT nBar, LPCSCROLLINFO lpsi, BOOL
bRedraw)
    BOOL bChangeParams = FALSE; /* Don't show/hide scrollbar if params don't
change */
    UINT MaxPage;
    int MaxPos;
-   /* [0] = HORZ, [1] = VERT */
-   static PWND PrevHwnd[2] = { 0 };
-   static DWORD PrevPos[2] = { 0 };
-   static DWORD PrevMax[2] = { 0 };
-   static INT PrevAction[2] = { 0 };
+   /* [0] = SB_HORZ, [1] = SB_VERT, [2] = SB_CTL */
+   static PWND PrevHwnd[3] = { 0 };
+   static DWORD PrevPos[3] = { 0 };
+   static DWORD PrevMax[3] = { 0 };
+   static INT PrevAction[3] = { 0 };
 
    ASSERT_REFS_CO(Window);
 
-   if(!SBID_IS_VALID(nBar))
+   if(!SBID_IS_VALID(nBar)) /* Assures nBar is 0, 1, or 2 */
    {
       EngSetLastError(ERROR_INVALID_PARAMETER);
       ERR("Trying to set scrollinfo for unknown scrollbar type %d", nBar);