microsoft / microsoft/DirectXShaderCompiler

Improve robustness of `CShaderCodeParser::SetShader` to malformed inputs

Open
#8,406 0 comments 0 reactions 0 assignees View on GitHub

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.

https://github.com/microsoft/DirectXShaderCompiler/blob/e7180626a7ebf75dc6a48f220d9a0cc3577374ff/projects/dxilconv/lib/ShaderBinary/ShaderBinary.cpp#L362-L367

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:

  1. IDxbcConverter::Convert -> DxbcConverter::Convert -> DxbcConverter::ConvertImpl -> CShaderCodeParser::SetShader

https://github.com/microsoft/DirectXShaderCompiler/blob/ae7413c782147f551767636d90b03f0968166701/projects/dxilconv/include/DxbcConverter.h#L34-L39

  1. IDxbcConverter::ConvertInDriver -> DxbcConverter::ConvertInDriver -> DxbcConverter::ConvertInDriverImpl -> CShaderCodeParser::SetShader

https://github.com/microsoft/DirectXShaderCompiler/blob/ae7413c782147f551767636d90b03f0968166701/projects/dxilconv/include/DxbcConverter.h#L41-L51

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].

https://github.com/microsoft/DirectXShaderCompiler/blob/ae7413c782147f551767636d90b03f0968166701/projects/dxilconv/include/DxbcConverter.h#L43-L44

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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.