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

Expose sgx report body fields by oe claims #3723

Merged
merged 1 commit into from
Nov 14, 2020
Merged

Conversation

qiucwang
Copy link
Collaborator

@qiucwang qiucwang commented Nov 9, 2020

Fixes #3689
Added two more fields in sgx_report_body struct that consistent with intel SGX report structure

uint8_t configid[64];    
uint16_t configsvn;

Added oe_claim entries to expose sgx_report_body fields. Flag fields as misc_select and attributes are converted to multiple booleans.

Signed-off-by: Qiucheng Wang qiucwang@microsoft.com

common/sgx/verifier.c Outdated Show resolved Hide resolved
@qiucwang
Copy link
Collaborator Author

bors try

bors bot pushed a commit that referenced this pull request Nov 10, 2020
@bors
Copy link

bors bot commented Nov 10, 2020

try

Build succeeded:

@yentsanglee
Copy link
Contributor

bors r+

@bors
Copy link

bors bot commented Nov 11, 2020

👎 Rejected by too few approved reviews

Copy link
Collaborator

@ryanhsu19 ryanhsu19 left a comment

Choose a reason for hiding this comment

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

Great work!

@ryanhsu19
Copy link
Collaborator

bors retry

bors bot pushed a commit that referenced this pull request Nov 11, 2020
3723: Expose sgx report body fields by oe claims r=yentsanglee a=qiucwang

Fixes #3689 
Added two more fields in `sgx_report_body` struct that consistent with intel [SGX report structure](https://github.com/intel/SGXDataCenterAttestationPrimitives/blob/master/QuoteGeneration/common/inc/internal/linux/arch.h#L55-L77)
```
uint8_t configid[64];    
uint16_t configsvn;
```
Added oe_claim entries to expose `sgx_report_body` fields. Flag fields as `misc_select` and `attributes` are converted to multiple booleans.

Signed-off-by: Qiucheng Wang <qiucwang@microsoft.com>

Co-authored-by: Qiucheng Wang <qiucwang@microsoft.com>
@bors
Copy link

bors bot commented Nov 11, 2020

Build failed:

@@ -302,6 +312,91 @@ static oe_result_t _fill_with_known_claims(
format_id,
sizeof(*format_id)));

// SGX Report CPU SVN
Copy link
Collaborator

Choose a reason for hiding this comment

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

CPUSVN in SGX QUOTE/REPORT is a "hint" only. Security analysis should not be made based on the CPUSVN value reported by the SGX enclave SW, including the SGX PCE/QE. The SGX CPU's security property is endorsed by the PCK Cert Chain. The leaf cert of the PCK Cert Chain includes SVNs of CPU firmware/microcode modules that are inside SGX TCB. Successful QUOTE verification, using the PCK Cert Chain, indicates the underlining SGX CPU's security property is equal to or higher than the SVNs in the PCK Cert Chain leaf Cert. The SVNs in the PCK Cert Chain leaf Cert might be associated with a SGX TCB that's out-of-date, ie, with known vulnerability. In that case, it's possible that the enclave SW that produced the SGX QUOTE has been compromised and might have spoofed the CPUSVN value in the SGX QUOTE. Only if the leaf Cert indicates the SGX CPU is free of any known security vulnerability, should the CPUSVN value in SGX QUOTE be trusted.

CPUSVN value in SGX QUOTE/REPORT is an opaque value. Two different value of CPUSVN can not be numerically compared for TCB level comparison purpose.

Therefore, exposing the CPUSVN value in SGX QUOTE as a claim almost has no value. It should not be included in the outputted claims.

Copy link
Collaborator Author

@qiucwang qiucwang Nov 11, 2020

Choose a reason for hiding this comment

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

Hid CPUSVN from QUOTE.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good.

common/sgx/verifier.c Outdated Show resolved Hide resolved
@yentsanglee
Copy link
Contributor

Looks good to me.

@yentsanglee
Copy link
Contributor

bors r+

@bors
Copy link

bors bot commented Nov 12, 2020

👎 Rejected by code reviews

@yentsanglee yentsanglee self-requested a review November 12, 2020 20:52
@yentsanglee
Copy link
Contributor

bors retry

@bors
Copy link

bors bot commented Nov 12, 2020

👎 Rejected by code reviews

@qiucwang
Copy link
Collaborator Author

bors retry

bors bot pushed a commit that referenced this pull request Nov 12, 2020
3723: Expose sgx report body fields by oe claims r=yentsanglee a=qiucwang

Fixes #3689 
Added two more fields in `sgx_report_body` struct that consistent with intel [SGX report structure](https://github.com/intel/SGXDataCenterAttestationPrimitives/blob/master/QuoteGeneration/common/inc/internal/linux/arch.h#L55-L77)
```
uint8_t configid[64];    
uint16_t configsvn;
```
Added oe_claim entries to expose `sgx_report_body` fields. Flag fields as `misc_select` and `attributes` are converted to multiple booleans.

Signed-off-by: Qiucheng Wang <qiucwang@microsoft.com>

Co-authored-by: Qiucheng Wang <qiucwang@microsoft.com>
Copy link
Collaborator

@ryanhsu19 ryanhsu19 left a comment

Choose a reason for hiding this comment

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

Thanks for the great work!

@bors
Copy link

bors bot commented Nov 12, 2020

Build failed:

@qiucwang
Copy link
Collaborator Author

bors try

bors bot pushed a commit that referenced this pull request Nov 12, 2020
@bors
Copy link

bors bot commented Nov 12, 2020

try

Build failed:

Signed-off-by: Qiucheng Wang <qiucwang@microsoft.com>
@qiucwang
Copy link
Collaborator Author

bors try

bors bot pushed a commit that referenced this pull request Nov 13, 2020
@bors
Copy link

bors bot commented Nov 13, 2020

try

Build failed:

@qiucwang
Copy link
Collaborator Author

bors retry

bors bot pushed a commit that referenced this pull request Nov 13, 2020
@bors
Copy link

bors bot commented Nov 13, 2020

try

Build succeeded:

@yentsanglee
Copy link
Contributor

bors r+

@bors
Copy link

bors bot commented Nov 14, 2020

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request to expose additional SGX report fields as claims
4 participants