Fixing a buffer overflow bug in FixedBitVector

[kevcadieux]

The FixedBitVector implementation contains a buffer overflow bug where a pointer to type of size 1 is reinterpret-casted into a pointer to a bigger type, and then dereferenced. Dereferencing the pointer to type of size bigger than 1 causes more bytes to be read from the buffer than are available. It looks like the code that was meant to ensure the correct pointer type was used was switching on sizeof(BVUnit::BVUnitTContainer) instead of sizeof(Container), resulting in using a bigger pointer type when sizeof(BVUnitTContainer) is bigger than 1 but sizeof(Container) is 1.

This bug was found when building ChakraCore using MSVC with the AddressSanitizer feature turned on. Below is a sample ASan error stack trace:

==52436==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x0849c2e0 at pc 0x7b2ab730 bp 0x0849c284 sp 0x0849c278
READ of size 4 at 0x0849c2e0 thread T1
    #0 0x7b2ab72f in BVFixed::SetRange<struct Js::LoopFlags>(struct Js::LoopFlags *,unsigned int,unsigned int) D:\github\ChakraCore\lib\Common\DataStructures\FixedBitVector.h:278
    #1 0x7b2a4399 in Js::DynamicProfileInfo::Initialize(class Js::FunctionBody * const) D:\github\ChakraCore\lib\Runtime\Language\DynamicProfileInfo.cpp:151
    #2 0x7b2a41d2 in Js::DynamicProfileInfo::New(class Memory::Recycler *,class Js::FunctionBody *,bool) D:\github\ChakraCore\lib\Runtime\Language\DynamicProfileInfo.cpp:125
    #3 0x7b040e3e in Js::ScriptContext::AddDynamicProfileInfo<class Memory::WriteBarrierPtr>(class Js::FunctionBody *,class Memory::WriteBarrierPtr<class Js::DynamicProfileInfo> &) D:\github\ChakraCore\lib\Runtime\Base\ScriptContext.cpp:5696
    #4 0x7b00bb15 in Js::FunctionBody::EnsureDynamicProfileInfo(void) D:\github\ChakraCore\lib\Runtime\Base\FunctionBody.cpp:3568
    #5 0x7b00c1c1 in Js::FunctionBody::EnsureDynamicInterpreterThunk(class Js::FunctionEntryPointInfo *) D:\github\ChakraCore\lib\Runtime\Base\FunctionBody.cpp:3870
    #6 0x7b1e5ef2 in Js::InterpreterStackFrame::EnsureDynamicInterpreterThunk(class Js::ScriptFunction *) D:\github\ChakraCore\lib\Runtime\Language\InterpreterStackFrame.cpp:1787
    #7 0x7b1e5e8b in Js::InterpreterStackFrame::DelayDynamicInterpreterThunk(class Js::RecyclableObject *,struct Js::CallInfo,...) D:\github\ChakraCore\lib\Runtime\Language\InterpreterStackFrame.cpp:1757
    #8 0x7b34143d in LocalCallFunction D:\github\ChakraCore\lib\Runtime\Library\JavascriptFunction.cpp:1326
    #9 0x7b346617 in Js::JavascriptFunction::CallFunction<1>(class Js::RecyclableObject *,void * (*)(class Js::RecyclableObject *,struct Js::CallInfo,...),struct Js::Arguments,bool) D:\github\ChakraCore\lib\Runtime\Library\JavascriptFunction.cpp:1345
    #10 0x7b2916e9 in Js::InterpreterStackFrame::OP_CallCommon<struct Js::OpLayoutDynamicProfile<struct Js::OpLayoutT_CallI<struct Js::LayoutSizePolicy<0> > > >(struct Js::OpLayoutDynamicProfile<struct Js::OpLayoutT_CallI<struct Js::LayoutSizePolicy<0> > > const *,class Js::RecyclableObject *,unsigned int,struct Js::AuxArray<unsigned int> const *) D:\github\ChakraCore\lib\Runtime\Language\InterpreterStackFrame.cpp:4000
    #11 0x7b292471 in Js::InterpreterStackFrame::OP_ProfileCallCommon<struct Js::OpLayoutDynamicProfile<struct Js::OpLayoutT_CallI<struct Js::LayoutSizePolicy<0> > > >(struct Js::OpLayoutDynamicProfile<struct Js::OpLayoutT_CallI<struct Js::LayoutSizePolicy<0> > > const *,class Js::RecyclableObject *,unsigned int,unsigned short,unsigned int,struct Js::AuxArray<unsigned int> const *) D:\github\ChakraCore\lib\Runtime\Language\InterpreterStackFrame.cpp:4028
    #12 0x7b271270 in Js::InterpreterStackFrame::OP_ProfiledCallI<struct Js::OpLayoutT_CallI<struct Js::LayoutSizePolicy<0> > >(struct Js::OpLayoutDynamicProfile<struct Js::OpLayoutT_CallI<struct Js::LayoutSizePolicy<0> > > const *) D:\github\ChakraCore\lib\Runtime\Language\InterpreterStackFrame.h:512
    #13 0x7b1f41d9 in Js::InterpreterStackFrame::ProcessProfiled(void) D:\github\ChakraCore\lib\Runtime\Language\InterpreterHandler.inl:85
    #14 0x7b24f64e in Js::InterpreterStackFrame::Process(void) D:\github\ChakraCore\lib\Runtime\Language\InterpreterStackFrame.cpp:3484
    #15 0x7b1e6d77 in Js::InterpreterStackFrame::InterpreterHelper(class Js::ScriptFunction *,struct Js::ArgumentReader,void *,void *,struct Js::InterpreterStackFrame::AsmJsReturnStruct *) D:\github\ChakraCore\lib\Runtime\Language\InterpreterStackFrame.cpp:2165
    #16 0x7b1e5fe0 in Js::InterpreterStackFrame::InterpreterThunk(class Js::JavascriptCallStackLayout *) D:\github\ChakraCore\lib\Runtime\Language\InterpreterStackFrame.cpp:1845

[MadProbe]

I am just a contributor, but I still can help with failing style checks:
First of all, you need to sign CLA (read /ContributionAgreement.md and append your name and surname in first row and your username in second row in the table if you agree to it)
Also you need to update headers (they basically located at the start of the file and contain licence info) of all files you've changed, if they aren't already, to this:

ChakraCore/lib/Runtime/Library/JavascriptLibrary.cpp

Lines 1 to 5 in 0fc6103

	//-------------------------------------------------------------------------------------------------------
	// Copyright (C) Microsoft Corporation and contributors. All rights reserved.
	// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
	// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
	//-------------------------------------------------------------------------------------------------------

[kevcadieux]

I am just a contributor, but I still can help with failing style checks:
First of all, you need to sign CLA (read /ContributionAgreement.md and append your name and surname in first row and your username in second row in the table if you agree to it)
Also you need to update headers (they basically located at the start of the file and contain licence info) of all files you've changed, if they aren't already, to this:

ChakraCore/lib/Runtime/Library/JavascriptLibrary.cpp

Lines 1 to 5 in 0fc6103

	//-------------------------------------------------------------------------------------------------------
	// Copyright (C) Microsoft Corporation and contributors. All rights reserved.
	// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
	// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
	//-------------------------------------------------------------------------------------------------------

Hi @MadProbe, thanks for your help. I modified the copyright so I think the build should succeed now. I think I am exempt from adding my name ContributionAgreement.md given that I am a Microsoft employee and my change here is done on behalf of Microsoft.

[kevcadieux]

@rhuanjl @ppenzin I saw you two approved PRs recently. Could you please take a look at this change? Thanks!

[rhuanjl]

Hi @kevcadieux sorry for the slow response; I've had a busy week.

Question on saying you're doing this on behalf of microsoft:

• I wrote that clause into the Contribution Agreement because at the time Microsoft was continuing to produce security fixes for the release-1.11 branch; however they committed to doing that only until March, see this issue: ChakraCore Transition #6384
• I was not expecting further contributions from Microsoft - is further MS work planned or is this a one off?
• If this is on behalf of MS is it redistributable under this license: https://github.com/chakra-core/ChakraCore/blob/master/LICENSE.txt (as per all the former MS work in the repository)

[kevcadieux]

Hi @kevcadieux sorry for the slow response; I've had a busy week.

Question on saying you're doing this on behalf of microsoft:

• I wrote that clause into the Contribution Agreement because at the time Microsoft was continuing to produce security fixes for the release-1.11 branch; however they committed to doing that only until March, see this issue: ChakraCore Transition #6384
• I was not expecting further contributions from Microsoft - is further MS work planned or is this a one off?

I work on the MSVC compiler team, and we used open source projects to validate our AddressSanitizer implementation. One of the bugs that were found is in ChakraCore. This PR is just a one off to fix this bug; I am not affiliated with any team at Microsoft that is working on ChakraCore.

• If this is on behalf of MS is it redistributable under this license: https://github.com/chakra-core/ChakraCore/blob/master/LICENSE.txt (as per all the former MS work in the repository)

Yes, I am providing this fix as an open-source contribution under the MIT license used by ChakraCore.

When I say that my change is done on behalf of Microsoft, I only mean that I am a Microsoft employee and that all my work belongs to Microsoft. To keep things simple, I will add my name to the Contributor Agreement.