microsoft / microsoft/DirectXShaderCompiler
Improve robustness of `CShaderCodeParser::SetShader` to malformed inputs
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 3.7k
- Forks
- 900
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 44
Description
The dxilconv's CShaderCodeParser::SetShader(CONST CShaderToken *pBuffer) does not validate its input.
It assumes pBuffer[1] is correct and is an in-bounds memory read, when it may or may not be the case.
An incorrect pBuffer[1] results in undefined behavior due to the CShaderCodeParser reading potentially out-of-bounds memory.
There are two call-paths to the SetShader function:
- IDxbcConverter::Convert -> DxbcConverter::Convert -> DxbcConverter::ConvertImpl -> CShaderCodeParser::SetShader
- IDxbcConverter::ConvertInDriver -> DxbcConverter::ConvertInDriver -> DxbcConverter::ConvertInDriverImpl -> CShaderCodeParser::SetShader
The first path (IDxbcConverter::Convert) starts with a dxil container and performs container validation in DxbcConverter::ConvertImpl before passing the dxbc container part to CShaderCodeParser::SetShader:
https://github.com/microsoft/DirectXShaderCompiler/blob/ae7413c782147f551767636d90b03f0968166701/projects/dxilconv/lib/DxbcConverter/DxbcConverter.cpp#L165-L169
The dxbcReader.Load invokes isValidDxilContainer which validates that the container is well-formed.
Therefore, there is no action required for the first path.
However, the second path (IDxbcConverter::ConvertInDriver) performs no such validation, because it directly accepts a dxbc container part and not the full container. The only thing known about the input pBytecode being passed to CShaderCodeParser::SetShader is that it is a const UINT32 * with a size specified (in SAL) by pByteCode[1].
An investigation should be performed to determine if there is a way to validate the input given to CShaderCodeParser::SetShader in the second path, and return early to prevent undefined behavior as a result of out-of-bounds memory accesses due to a potentially malformed input.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with CShaderCodeParser::SetShader in projects/dxilconv/lib/ShaderBinary/ShaderBinary.cpp and trace both ConvertImpl and ConvertInDriverImpl in DxbcConverter.cpp. Compare the container validation in DxilContainer.cpp with the direct ConvertInDriver input path. Done means determining how the second path can validate its bounded input and return before out-of-bounds reads.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100