MS09-050, SMBv2 and the SDL

MS09-050, SMBv2 and the SDL

  • Comments 1

10/20/2009: Updated with correct CVE - thanks to Matthieu Suiche for pointing this out to me.

Hi, Michael here.

When I wrote the first analysis of why the SDL had missed a security vulnerability, I made a comment that I would continue to write these posts, but only for bugs that interested me. To be honest, all security bugs interest me, but this one really got me to sit up because it’s in new code.

For reference, the security update that fixes this is MS09-050, and the bug is CVE-2009-3103.

What makes the bug of concern is it’s in networking code; thankfully, there are some mitigations available, such as the Windows Firewall, that reduce exposure to attacks.

First, let’s take a look at the vulnerable code. Can you spot the bug?

#define Smb2GetWorkItem( WI ) ((PSMB2_WORK_ITEM)(WI->ProviderWorkItem))
...
typedef struct _SRV_WORK_ITEM
{
...
    //
    // This is the Receive Buffer for the incoming request
    //
    PSRVBUFFER ReceiveBuffer;
    PSRVBUFFER ResponseBuffer;
 
...
} SRV_WORK_ITEM, *PSRV_WORK_ITEM;
 
...
NTSTATUS
Smb2ValidateProviderCallback( PSRV_WORK_ITEM WorkItem )
{
    PSMB2_HEADER pHeader = (PSMB2_HEADER)WorkItem->ReceiveBuffer->Buffer;
    PSMB2_WORK_ITEM pWI = Smb2GetWorkItem( WorkItem );
    PSMB2_CONNECTION pC = Smb2GetConnection( WorkItem->Connection );
    NTSTATUS status;
 
    pWI->ParentWorkItem = WorkItem;
    pWI->AsyncId = RFSTABLE64_INVALID_ITEM;
    WorkItem->ProviderWorkItemCleanupRoutine = Smb2CleanupWorkItem;
 
...
 
    if( pHeader->ProtocolId != SMB2_PROTOCOL_ID )
    {
        if( pHeader->ProtocolId == SMB_PROTOCOL_ID &&
            pC->Dialect == 0xFFFF )
        {
            //
            // Handle downlevel multi-negotiate
            //
            pWI->Command = SMB2_0_COMMAND_NEGOTIATE;
            goto process_packet;
        }
        else
        {
            WorkItem->DisconnectConnection = TRUE;
            return STATUS_INVALID_PARAMETER;
        }
    }
 
    pWI->Command = pHeader->Command;
 
 
...
 
process_packet:
    if( SRVWPP_LOG_MESSAGE( DEBUG_MODULE_SRV2, DEBUG_PERF ) )
    {
        Smb2OutputWorkItemRequest( WorkItem );
    }
 
    if( ValidateRoutines[pHeader->Command ] == NULL )
    {
        return Smb2ValidateNotImplemented( WorkItem );
    }
    else
    {
        return (ValidateRoutines[pHeader->Command])( WorkItem );
    }
}

If you can’t see the bug, here’s the fix:

    if( SRVWPP_LOG_MESSAGE( DEBUG_MODULE_SRV2, DEBUG_PERF ) )
    {
        Smb2OutputWorkItemRequest( WorkItem );
    }

    if( ValidateRoutines[pWI->Command] == NULL )
    {
        return Smb2ValidateNotImplemented( WorkItem );
    }
    else
    {
        return (ValidateRoutines[pWI->Command])( WorkItem );
    }
}

Look at the two array references to ValidateRoutines[] near the end, the array index to both is the wrong variable: pHeader->Command should be pWI->Command.

So why did the SDL miss this bug?

There is only one current SDL requirement or recommendation that could potentially find this, and that is fuzz testing. In fact we did find it very late in the Windows 7 development process through network fuzzing and that is why post-RC versions of Windows 7 do not have this bug.

Right now there is no static analysis tool I know of that would point out the developer used the wrong variable, and our analysis tools didn’t spot the potential array bounds problem in part because it’s hard to do so with generate a very large quantity of false positives. With that said, we’re looking deeper into the latter challenge now.

The only other method that could find this kind of bug is very slow and painstaking code review. This code was peer-reviewed prior to check-in into Windows Vista; but the bug was missed. Humans are fallible, after all.

Some years ago I created a “How to review code for Security Bugs” class and toward the end I explain that code reviewers need to question all coding logic assumptions when the code deals with untrusted data; I will add a new bullet point: are the correct variables used?

Going Out on a Limb!

I’ve mentioned this before, but it’s worth mentioning again. I think we’re getting to a stage at Microsoft where the SDL has whittled away most of the ‘low-hanging’ bugs. Of course, I might be proven wrong, but looking at all the bugs over the last year in Windows, the only pattern I can spot is there is no pattern! The majority of the bugs I see in Windows are one-off bugs that can’t be found easily through static analysis or education, which leaves only manual code review, and for some bug classes, fuzz testing. But fuzz testing is hardly perfect, because the malformed data might not hit the vulnerable code path or trigger a failure in the code.

I would say that this is a great argument for software developers spending more time on defenses against unknown vulnerabilities, as well as trying to prevent or remove vulnerabilities. The SDL mantra of “Reduce the number of vulnerabilities and reduce the severity of the bugs you miss” is very consistent with this belief.

- Michael

luv u kim x

Comments
  • There is another aspect for the vulnerability. The vulnerability gets triggered when the pid high is not 0. for negotiate protocol 0x72.

    If we refer to SMB specification it states that

    " for 16- bit process id the value has to be 0 and for 32- bit process Id, the value of PIDHigh is as per the "A Common Internet File System (CIFS/1.0)Protocol" specification.        

     Further referring to the CIFS[4], section 2.4.2 as shown in the PidHigh is used only in NtCreateAndX request. The command value of NtCreateAndx is 0xa2. Since the value are used in NtCreateAndx, so for the command, negotiate (0x72) the value of PIDHigh has to be 0.

    If the fuzzing tools would have ensured that the values defined in the protocol specifications are enforced, then this vulnerability could have been caught.

    Abhishek

Page 1 of 1 (1 items)
Leave a Comment
  • Please add 8 and 2 and type the answer here:
  • Post