Skip to content

Conversation

@trylek
Copy link
Member

@trylek trylek commented Jun 30, 2022

As part of Control Flow Enforcement Technology (CET) testing we need
to make sure that CET is actually active on the execution machines;
otherwise subtle infra changes could easily regress the testing by
inadvertently deactivating CET without anyone noticing. This change
introduces an initial CET availability test for this purpose.

Thanks

Tomas

P.S. In practice this initial commit only introduces the necessary infra
changes required for implementing the CET test. I have verified in the private
run

https://dev.azure.com/dnceng/public/_build/results?buildId=1855384&view=ms.vss-test-web.build-test-results-tab&runId=48818082&resultId=100238&paneView=debug

that the new test gets now built and run and it fails as expected. I need
help regarding its actual implementation, according to JanV we likely need
a bit of native interop to read the value of the SSP register, I'm definitely
no expert in this field.

@ghost
Copy link

ghost commented Jun 30, 2022

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

As part of Control Flow Enforcement Technology (CET) testing we need
to make sure that CET is actually active on the execution machines;
otherwise subtle infra changes could easily regress the testing by
inadvertently deactivating CET without anyone noticing. This change
introduces an initial CET availability test for this purpose.

Thanks

Tomas

P.S. In practice this initial commit only introduces the necessary infra
changes required for implementing the CET test. I have verified in the private
run

https://dev.azure.com/dnceng/public/_build/results?buildId=1855384&view=ms.vss-test-web.build-test-results-tab&runId=48818082&resultId=100238&paneView=debug

that the new test gets now built and run and it fails as expected. I need
help regarding its actual implementation, according to JanV we likely need
a bit of native interop to read the value of the SSP register, I'm definitely
no expert in this field.

Author: trylek
Assignees: -
Labels:

area-Infrastructure-coreclr

Milestone: -

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The strongest check would be to actually trigger a crash by performing an operation that is prohibited with CET.

We can then have a pair of tests: One that verifies that the operation is succeeding when CET is disabled, and one that verifies that the operation is crashing when CET is enabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trigger a crash by performing an operation that is prohibited with CET.

You probably still need to a bit of unmanaged code for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think checking the value of SSP is sufficient. The SSP is always nonzero if CET is enabled for the process and zero otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already implemented the SSP check. We can easily add more tests if desired once that's shown to work.

trylek added 2 commits July 19, 2022 22:21
As part of Control Flow Enforcement Technology (CET) testing we need
to make sure that CET is actually active on the execution machines;
otherwise subtle infra changes could easily regress the testing by
inadvertently deactivating CET without anyone noticing. This change
introduces an initial CET availability test for this purpose.

Thanks

Tomas
@trylek trylek force-pushed the NegativeCETTest branch from 60a1064 to 433848b Compare July 20, 2022 00:56
@trylek
Copy link
Member Author

trylek commented Jul 20, 2022

/azp run runtime-cet

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@trylek
Copy link
Member Author

trylek commented Jul 20, 2022

@eduardo-vp - I have created the CET activation check test and run it through the runtime-cet pipeline where it failed claiming CET is not enabled:

https://dev.azure.com/dnceng/public/_build/results?buildId=1892579&view=ms.vss-test-web.build-test-results-tab&runId=49353780&resultId=100196&paneView=dotnet-dnceng.dnceng-build-release-tasks.helix-test-information-tab

    baseservices\CET\CheckCETPresence\CheckCETPresence.cmd [FAIL]
      
      Return code:      1
      Raw output file:      C:\h\w\A1AF08B4\w\A9AF09A7\uploads\Reports\baseservices.CET\CheckCETPresence\CheckCETPresence.output.txt
      Raw output:
      BEGIN EXECUTION
       "C:\h\w\A1AF08B4\p\corerun.exe" -p "System.Reflection.Metadata.MetadataUpdater.IsSupported=false"  CheckCETPresence.dll 
      Checking whether codeflow enforcement technology (CET) is active
      Shadow stack pointer: 0x0000000000000000
      Expected: 100
      Actual: 101
      END EXECUTION - FAILED
      FAILED

Can you please follow up with @ilyas1974 and @janvorli to figure out what remains to be done to turn CET on for the pipeline? IIRC we require a certain version of Windows and some registry keys to be set.

Thanks

Tomas

@trylek trylek changed the title WIP: Implement test checking whether CET is active Implement test checking whether CET is active Jul 30, 2022
@trylek
Copy link
Member Author

trylek commented Jul 30, 2022

After receiving heads-up from @ilyas1974 that the new CET pool should now have all the required artifacts, I have retested the runtime-cet pipeline on this change and it is passing now so I'm merging it in.

@trylek trylek merged commit aa8489c into dotnet:main Jul 30, 2022
@trylek trylek deleted the NegativeCETTest branch July 30, 2022 15:40
@ghost ghost locked as resolved and limited conversation to collaborators Aug 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants