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

AzTdxVtpm get_evidence report_data maximum length #765

Closed
ssolit opened this issue Oct 23, 2024 · 8 comments
Closed

AzTdxVtpm get_evidence report_data maximum length #765

ssolit opened this issue Oct 23, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@ssolit
Copy link

ssolit commented Oct 23, 2024

Describe the bug

It seems like the AzTdxVtpmAttester has an issue with its get_evidence function where it can only take 11 bytes of report_data. However, based on the TDX_REPORT_DATA_SIZE constant it should be able to take 64 bytes.

How to reproduce

Running the following tests in attestation-agent/attester/src/az_tdx_vtpm/mod.rs will reproduce the error.

pub mod test {
    use crate::Attester;

    #[tokio::test]
    async fn attest_11_bytes() {
        let attester = super::AzTdxVtpmAttester::default();
        let report_data = "nonce678901".as_bytes().to_vec();
        let evidence = attester.get_evidence(report_data).await.unwrap();
    }

    #[tokio::test]
    async fn attest_12_bytes() {
        let attester = super::AzTdxVtpmAttester::default();
        let report_data = "nonce6789012".as_bytes().to_vec();
        let evidence = attester.get_evidence(report_data).await.unwrap();
    }
}

Only attest_11_bytes will pass

CoCo version information

guest-components v0.10.0

What TEE are you seeing the problem on

AzTdxVtpm

Failing command and relevant log output

---- az_tdx_vtpm::test::attest_12_bytes stdout ----
thread 'az_tdx_vtpm::test::attest_12_bytes' panicked at attestation-agent/attester/src/az_tdx_vtpm/mod.rs:86:65:
called `Result::unwrap()` on an `Err` value: Failed to write value to nvindex
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
@ssolit ssolit added the bug Something isn't working label Oct 23, 2024
@ssolit
Copy link
Author

ssolit commented Oct 23, 2024

It seems like this is related to this PR. Azure vTPM has a different max size for the report data, but it should still significantly larger than 11 bytes

@fitzthum
Copy link
Member

cc @mkulke

@mkulke
Copy link
Contributor

mkulke commented Oct 23, 2024

I'm a bit confused because by default the trustee report data is a sha384 digest. this is being tested e.g. here with real tdx hardware. so, it should be able to accept larger report_data than 11 bytes? it doesn't accept 64 bytes due to issues in libtss, but maybe there is an issue with exactly 12 bytes... would 48 bytes work?

In any case the attester is merely calling the relevant crate's function, can you open an issue upstream?

@mythi
Copy link
Contributor

mythi commented Oct 23, 2024

It seems like this is related to this PR. Azure vTPM has a different max size for the report data, but it should still significantly larger than 11 bytes

Interesting. In the default case we pass sha384 or sha256 hashed report_data which are both more than 12 bytes.

This could be about parallelism. Does it work with something like:

RUST_TEST_THREADS=1 cargo test --no-default-features --features az-tdx-vtpm-attester. I have that passin on my side at least.

@ssolit
Copy link
Author

ssolit commented Oct 23, 2024

I opened an issue upstream - kinvolk/azure-cvm-tooling#61

Inside that issue are some more thoughts on what might be happening. TLDR it seems like there is some tpm statefulness that isn't accounted for, and running sudo tpm2_nvundefine -C o 0x01400002 can clear it to allow longer inputs

@mkulke
Copy link
Contributor

mkulke commented Oct 23, 2024

thanks for pointing this out. I think the problem is indeed that the nv_index is not found, then allocated with a certain buffer size initially (11 bytes) and then on the 2nd attempt we do find the nv_index and attempt to write 12 bytes to it.

this might or might not be fixable without introducing state, let's see.

In general fetching multiple attestation report in quick succession will get us into trouble, since you might get a stale report. new attestation reports take up to 3s to be generated and in the meantime you could retrieve stale reports with outdated reportdata. we probably have to protect this with a mutex or something.

note: the report_data you pass into get_evidence will not end up 1:1 in an azure td/snp report, it will be mixed w/ vm metadata and hashed. In trustee's attestation service we don't verify report_data in a td report in the az-tdx-vtpm verifier (it is relevant for ITA, an alternative attestation-service that can be integrated w/ trustee). the report_data is also passed as a nonce to the tpm quote, and we verify that one.

I think a workaround for the time being would be to use a fixed size/padded report_data buffer across invocations of get_evidence(). would that work for you?

@ssolit
Copy link
Author

ssolit commented Oct 23, 2024

Thank you for the extra advice regarding fetching reports in quick succession and how report data is used.

For now the workaround you mentioned is good enough. The main reason this was blocking was the very first time I ran the code a while ago I picked the input randomly, and since the first input was only 11 bytes I couldn't transition to a 32 byte hash.

Now that I know how to clear buffer I should be unblocked. Though fixing the bug would help future users who might hit the same pitfall

@mkulke
Copy link
Contributor

mkulke commented Oct 24, 2024

yup, this should get fixed and I think there's a fix. but I think in the scope of this repo we can close this issue, since we use fixed report_data sizes. agree?

@ssolit ssolit closed this as completed Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants