Uploaded image for project: 'Core ReactOS'
  1. Core ReactOS
  2. CORE-15902

Bugs in FsRtlIsNameInExpressionPrivate

    XMLWordPrintable

Details

    • Bug
    • Resolution: Fixed
    • Major
    • 0.4.12
    • NTCore

    Description

      I identified 4 issues in the FsRtlIsNameInExpressionPrivate function.

      1. Overflow of the BackTrackingBuffer stack buffer by 2 bytes.
        Triggered by input: Expression="FI<<<<<<<<", Name="FILE"
        Bug found with afl-fuzz.
        Patch:

        diff --git a/ntoskrnl/fsrtl/name.c b/ntoskrnl/fsrtl/name.c
        index fef798cc86d..5ad84ada7bf 100644
        --- a/ntoskrnl/fsrtl/name.c
        +++ b/ntoskrnl/fsrtl/name.c
        @@ -135,7 +135,7 @@ FsRtlIsNameInExpressionPrivate(IN PUNICODE_STRING Expression,
                         }
         
                         /* If buffer too small */
        -                if (BackTrackingPosition > BackTrackingBufferSize - 2)
        +                if (BackTrackingPosition > BackTrackingBufferSize - 3)
                         {
                             /* We should only ever get here once! */
                             ASSERT(AllocatedBuffer == NULL);
        

      2. Logic error that can result in inadvertently swapping the contents of BackTracking and OldBackTracking.
        Triggered by input: Expression="<<<<<<<<<.<", Name="."
        Bug found by myself, triggering input found with afl-fuzz.
        Patch:

        diff --git a/ntoskrnl/fsrtl/name.c b/ntoskrnl/fsrtl/name.c
        index fef798cc86d..37da9531ddd 100644
        --- a/ntoskrnl/fsrtl/name.c
        +++ b/ntoskrnl/fsrtl/name.c
        @@ -157,21 +157,21 @@ FsRtlIsNameInExpressionPrivate(IN PUNICODE_STRING Expression,
                                 goto Exit;
                             }
         
        -                    /* Backtracking is at the start of the buffer */
        -                    BackTracking = AllocatedBuffer;
        -
        -                    /* Copy BackTrackingBuffer content */
        -                    RtlCopyMemory(BackTracking,
        -                                  BackTrackingBuffer,
        +                    /* Copy BackTracking content. Note that it can point to either BackTrackingBuffer or OldBackTrackingBuffer */
        +                    RtlCopyMemory(AllocatedBuffer,
        +                                  BackTracking,
                                           RTL_NUMBER_OF(BackTrackingBuffer) * sizeof(USHORT));
         
        -                    /* OldBackTracking is after BackTracking */
        -                    OldBackTracking = &BackTracking[BackTrackingBufferSize];
        +                    /* Place current Backtracking is at the start of the new buffer */
        +                    BackTracking = AllocatedBuffer;
         
        -                    /* Copy OldBackTrackingBuffer content */
        -                    RtlCopyMemory(OldBackTracking,
        -                                  OldBackTrackingBuffer,
        +                    /* Copy OldBackTracking content */
        +                    RtlCopyMemory(&BackTracking[BackTrackingBufferSize],
        +                                  OldBackTracking,
                                           RTL_NUMBER_OF(OldBackTrackingBuffer) * sizeof(USHORT));
        +
        +                    /* Place current OldBackTracking after current BackTracking in the buffer */
        +                    OldBackTracking = &BackTracking[BackTrackingBufferSize];
                         }
         
                         /* Basic check to test if chars are equal */
        

      3. Incorrect handling of DOS_STAR due to a logic bug. It causes DontSkipDot to be always set to FALSE whenever the for loop is entered, effectively rendering DOS_STAR equivalent to regular star (*).
        Triggered by input: Expression="F<", Name="FILE.TXT"
        Patch:

        diff --git a/ntoskrnl/fsrtl/name.c b/ntoskrnl/fsrtl/name.c
        index fef798cc86d..8bc14da7cfc 100644
        --- a/ntoskrnl/fsrtl/name.c
        +++ b/ntoskrnl/fsrtl/name.c
        @@ -200,7 +200,7 @@ FsRtlIsNameInExpressionPrivate(IN PUNICODE_STRING Expression,
                             DontSkipDot = TRUE;
                             if (!EndOfName && Name->Buffer[NamePosition] == '.')
                             {
        -                        for (Position = NamePosition - 1; Position < Name->Length; Position++)
        +                        for (Position = NamePosition + 1; Position < Name->Length / sizeof(WCHAR); Position++)
                                 {
                                     if (Name->Buffer[Position] == L'.')
                                     {
        

      4. Excessive buffer allocation by a factor of 2. The following loop exit condition should make it clear that BackTrackingBufferSize need not be larger than Expression->Length + 1:

                        if (ExpressionPosition == Expression->Length)
                        {
                            BackTracking[BackTrackingPosition++] = Expression->Length * 2;
                            break;
                        }
        

        Patch:

        diff --git a/ntoskrnl/fsrtl/name.c b/ntoskrnl/fsrtl/name.c
        index fef798cc86d..a6f0c004b7f 100644
        --- a/ntoskrnl/fsrtl/name.c
        +++ b/ntoskrnl/fsrtl/name.c
        @@ -142,9 +142,9 @@ FsRtlIsNameInExpressionPrivate(IN PUNICODE_STRING Expression,
                             ASSERT((BackTracking == BackTrackingBuffer) || (BackTracking == OldBackTrackingBuffer));
                             ASSERT((OldBackTracking == BackTrackingBuffer) || (OldBackTracking == OldBackTrackingBuffer));
         
                             /* Calculate buffer size */
        -                    BackTrackingBufferSize = (Expression->Length + 1) * 2;
        +                    BackTrackingBufferSize = Expression->Length + 1;
         
                             /* Allocate memory for both back-tracking buffers */
                             AllocatedBuffer = ExAllocatePoolWithTag(PagedPool | POOL_RAISE_IF_ALLOCATION_FAILURE,
                                                                     2 * BackTrackingBufferSize * sizeof(USHORT),
        
        

      Attachments

        Activity

          People

            Heis Spiter Pierre Schweitzer
            bbrachaczek Bartosz Brachaczek
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: