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

dcap-ql: should accept extra trailing data in quote #676

Open
1 of 4 tasks
jason-liang-vault opened this issue Jan 6, 2025 · 1 comment · May be fixed by #677
Open
1 of 4 tasks

dcap-ql: should accept extra trailing data in quote #676

jason-liang-vault opened this issue Jan 6, 2025 · 1 comment · May be fixed by #677
Labels
bug C-dcap-ql Crate: dcap-ql

Comments

@jason-liang-vault
Copy link

jason-liang-vault commented Jan 6, 2025

Describe the bug:

we've recently receive following error, which is generated in this line (https://github.com/fortanix/rust-sgx/blob/master/intel-sgx/dcap-ql/src/quote.rs#L240)
Invalid signature length, expected 4164, got 4168

After some investigation, following is the root cause

The quote function in dcap-ql which generate quote contains following two steps

  1. get_quote_size, to get an ESTIMATED size of the quote, and it is the application's (which is our) responsibility to allocate a buffer that is AT LEAST this big, and pass to get_quote function. Since this is just estimation, it has some default value if PCCS return empty data / failed to return data at the moment
  2. get_quote

The erroneous signature that trigger the error above is actually still valid, just that there are four trailing '\x00' at the end of the quote, which should be caused by a larger estimated quote size
get quote function looks like this, and the let mut quote = vec![0; quote_size as _]; should be the root cause of those trailing '\x00'

pub fn quote(report: &Report) -> Result<Vec<u8>, Quote3Error> {
    unsafe {
        let mut quote_size = 0;
        err_code_to_result(get_quote_size(&mut quote_size))?;

        let mut quote = vec![0; quote_size as _];
        err_code_to_result(get_quote(&report, quote_size, quote.as_mut_ptr()))?;
        Ok(quote)
    }
}

And when parsing the signature in fortanix's library, it requires the input buffer (which is vec![0; quote_size as _] to match the exact content (which is represented by format [length_of_following_bytes, byte_1, byte_2, ...], e.g. [0x5, 0xab, 0xcd, 0xef, 0x01, 0x23])

And the fix is simple, instead of failing when the expected quote length and actual quote length mismatch, we just keep parsing, and discard any trailing data (so effectively accepting longer quote, but will still reject quote that is shorter than expected)

To Reproduce:
Steps to reproduce the behavior:

  1. Append several random bytes after getting quote from dcap_ql::quote
  2. Parse the quote using dcap_ql crate

Expected behavior:
Extra bytes are ignored, and parsing passed

Reproducibility:

  • Always

Environment:

  • Ubuntu 22.04 LTS

Possible Solution:
Parse the quote as normal, but instead of failing directly after detecting extra trailing bytes, simply discard them

Severity:

  • Critical
  • Major
  • Normal
  • Minor
@mzohreva mzohreva added bug C-dcap-ql Crate: dcap-ql labels Jan 6, 2025
@mzohreva mzohreva changed the title [BUG]: should accept extra trailing data in quote dcap-ql: should accept extra trailing data in quote Jan 6, 2025
@jethrogb
Copy link
Member

jethrogb commented Jan 7, 2025

I don't think this is a bug as described. It is correct that a validation function errors out when extraneous data is passed in. The bug is that fn quote returns data that is longer than the quote.

See https://download.01.org/intel-sgx/sgx-dcap/1.3/linux/docs/Intel_SGX_ECDSA_QuoteLibReference_DCAP_API.pdf for the data structure specification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug C-dcap-ql Crate: dcap-ql
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants