Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DxeRngLib: GetRandomNumber spurious success #6486

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

idigdoug
Copy link
Contributor

@idigdoug idigdoug commented Dec 1, 2024

Description

The GetRandomNumber functions in DxeRngLib can return success without actually generating a random number. This occurs because there is a code path through GenerateRandomNumberViaNist800Algorithm that does not update the output buffer and does not initialize the Status variable.

  • Assume mFirstAlgo == MAX_UINTN (no secure algorithms available)
  • Assume none of the secure algorithms have Available set.
  • Assume PcdEnforceSecureRngAlgorithms is TRUE.

In this condition, the Status variable is never initialized, Buffer data is never touched. It is fairly likely that Status is 0, so we can return EFI_SUCCESS without writing anything to Buffer.

Fix is to set Status = error_code in this code path. EFI_SECURITY_VIOLATION seems appropriate.

  • Breaking change?
    • Breaking change - Does this PR cause a break in build or boot behavior?
    • Examples: Does it add a new library class or move a module to a different repo.
  • Impacts security?
    • Security - Does this PR have a direct security impact?
    • Examples: Crypto algorithm change or buffer overflow fix.
  • Includes tests?
    • Tests - Does this PR include any explicit test code?
    • Examples: Unit tests or integration tests.

How This Was Tested

  • Observed EFI_SUCCESS returned from GetRandomNumber16 even though output value remained 0.
  • Applied fix.
  • Observed EFI_SECURITY_VIOLATION returned from GetRandomNumber16 when expected.
  • Set PcdEnforceSecureRngAlgorithms to false, observed successful RNG generation.

Integration Instructions

N/A

@github-actions github-actions bot added the impact:security This change has a direct security impact such as changing a crypto algorithm. label Dec 1, 2024
@idigdoug idigdoug force-pushed the DxeRngStatus branch 2 times, most recently from b3450cf to bceb981 Compare December 1, 2024 21:30
@lgao4 lgao4 added the push Auto push patch series in PR if all checks pass label Dec 6, 2024
The GetRandomNumber functions in DxeRngLib can return success without
actually generating a random number. This occurs because there are code
paths through `GenerateRandomNumberViaNist800Algorithm` that do not
initialize the `Status` variable.

- Assume mFirstAlgo == MAX_UINTN (no secure algorithms available)
- Assume none of the secure algorithms have `Available` set.
- Assume PcdEnforceSecureRngAlgorithms is TRUE.

In this condition, the `Status` variable is never initialized, `Buffer`
data is never touched. It is fairly likely that Status is 0, so we can
return EFI_SUCCESS without writing anything to Buffer.

Fix is to set `Status = error_code` in this code path.
`EFI_SECURITY_VIOLATION` seems appropriate.

Signed-off-by: Doug Cook <[email protected]>
@mergify mergify bot merged commit fd9501f into tianocore:master Dec 6, 2024
126 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:security This change has a direct security impact such as changing a crypto algorithm. push Auto push patch series in PR if all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants