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.

Sign up for GitHub

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

Jump to bottom

[Bug]: BootAuditTestApp is not Passable due to UefiVarCheck #252

Closed
1 task done
TaylorBeebe opened this issue Jun 2, 2023 · 1 comment
Closed
1 task done

[Bug]: BootAuditTestApp is not Passable due to UefiVarCheck #252

TaylorBeebe opened this issue Jun 2, 2023 · 1 comment
Assignees
@TaylorBeebe
Labels
state:needs-triage Needs to triaged to determine next steps type:bug Something isn't working urgency:medium Important with a moderate impact

Comments

@TaylorBeebe
Copy link
Contributor

Is there an existing issue for this?

  • I have searched existing issues

Current Behavior

BootAuditTestApp attempts to write to various boot variables to check if they are write protected. There exists a sanity check which ensures that the set value makes sense, but BootAuditTestApp passes in garbage data causing the SetVariable() call to return Invalid Parameter.

Expected Behavior

BootAuditTestApp should be able to test all UEFI boot variables.

Steps To Reproduce

Run the test app on a platform with UefiVarCheckLib included.

Build Environment

- OS(s): Any
- Tool Chain(s): Any
- Targets Impacted: DEBUG

Version Information

Top of tree

Urgency

Medium

Are you going to fix this?

I will fix it

Do you need maintainer feedback?

No maintainer feedback needed

Anything else?

No response
@TaylorBeebe TaylorBeebe added type:bug Something isn't working state:needs-triage Needs to triaged to determine next steps labels Jun 2, 2023
@github-actions github-actions bot added the urgency:medium Important with a moderate impact label Jun 2, 2023
This was referenced Jun 2, 2023
Merged
Merged
TaylorBeebe added a commit that referenced this issue Jun 6, 2023
@TaylorBeebe
Update BootAuditTestApp to Correctly Test Load Option Variables (#256)
04e52c4 
## Description

BootAuditTestApp tests the variable policy for the SysPrep####,
PlatformRecovery####, and Driver#### variables by attempting to get and
set the variables. When calling SetVariable with one of these three
variables, VarCheckLib will sanity check the variable data and return
EFI_INVALID_PARAMETER if the data is not a valid load option. Because we
want to check if the variables are writable, we need to create properly
formatted data before calling SetVariable.

This PR uses the DevicePath of the running EFI application to create a
load option to properly test the writability of the boot option
variables securely.

Fixes Issue #252

- [x] Impacts functionality?
- **Functionality** - Does the change ultimately impact how firmware
functions?
- Examples: Add a new library, publish a new PPI, update an algorithm,
...
- [ ] Impacts security?
- **Security** - Does the change have a direct security impact on an
application,
    flow, or firmware?
  - Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
- [ ] Breaking change?
- **Breaking change** - Will anyone consuming this change experience a
break
    in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo,
call
    a function in a new library class in a pre-existing module, ...
- [x] Includes tests?
  - **Tests** - Does the change include any explicit test code?
  - Examples: Unit tests, integration tests, robot tests, ...
- [ ] Includes documentation?
- **Documentation** - Does the change contain explicit documentation
additions
    outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to
documentation
    on an a separate Web page, ...

## How This Was Tested

Tested on Q35

## Integration Instructions

N/A
@TaylorBeebe
Copy link
Contributor Author

Fixed with #256

TaylorBeebe added a commit to TaylorBeebe/mu_plus that referenced this issue Jun 6, 2023
@TaylorBeebe
Update BootAuditTestApp to Correctly Test Load Option Variables (micr…
1920768 
…osoft#256)

## Description

BootAuditTestApp tests the variable policy for the SysPrep####,
PlatformRecovery####, and Driver#### variables by attempting to get and
set the variables. When calling SetVariable with one of these three
variables, VarCheckLib will sanity check the variable data and return
EFI_INVALID_PARAMETER if the data is not a valid load option. Because we
want to check if the variables are writable, we need to create properly
formatted data before calling SetVariable.

This PR uses the DevicePath of the running EFI application to create a
load option to properly test the writability of the boot option
variables securely.

Fixes Issue microsoft#252

- [x] Impacts functionality?
- **Functionality** - Does the change ultimately impact how firmware
functions?
- Examples: Add a new library, publish a new PPI, update an algorithm,
...
- [ ] Impacts security?
- **Security** - Does the change have a direct security impact on an
application,
    flow, or firmware?
  - Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
- [ ] Breaking change?
- **Breaking change** - Will anyone consuming this change experience a
break
    in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo,
call
    a function in a new library class in a pre-existing module, ...
- [x] Includes tests?
  - **Tests** - Does the change include any explicit test code?
  - Examples: Unit tests, integration tests, robot tests, ...
- [ ] Includes documentation?
- **Documentation** - Does the change contain explicit documentation
additions
    outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to
documentation
    on an a separate Web page, ...

## How This Was Tested

Tested on Q35

## Integration Instructions

N/A
TaylorBeebe added a commit that referenced this issue Jun 7, 2023
@TaylorBeebe
Update BootAuditTestApp to Correctly Test Load Option Variables (#256)
8502050 
## Description

BootAuditTestApp tests the variable policy for the SysPrep####,
PlatformRecovery####, and Driver#### variables by attempting to get and
set the variables. When calling SetVariable with one of these three
variables, VarCheckLib will sanity check the variable data and return
EFI_INVALID_PARAMETER if the data is not a valid load option. Because we
want to check if the variables are writable, we need to create properly
formatted data before calling SetVariable.

This PR uses the DevicePath of the running EFI application to create a
load option to properly test the writability of the boot option
variables securely.

Fixes Issue #252

- [x] Impacts functionality?
- **Functionality** - Does the change ultimately impact how firmware
functions?
- Examples: Add a new library, publish a new PPI, update an algorithm,
...
- [ ] Impacts security?
- **Security** - Does the change have a direct security impact on an
application,
    flow, or firmware?
  - Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
- [ ] Breaking change?
- **Breaking change** - Will anyone consuming this change experience a
break
    in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo,
call
    a function in a new library class in a pre-existing module, ...
- [x] Includes tests?
  - **Tests** - Does the change include any explicit test code?
  - Examples: Unit tests, integration tests, robot tests, ...
- [ ] Includes documentation?
- **Documentation** - Does the change contain explicit documentation
additions
    outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to
documentation
    on an a separate Web page, ...

## How This Was Tested

Tested on Q35

## Integration Instructions

N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@TaylorBeebe TaylorBeebe

Labels
state:needs-triage Needs to triaged to determine next steps type:bug Something isn't working urgency:medium Important with a moderate impact
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@TaylorBeebe