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

[ujson,perso_tlv,dice] Fix potential signed integer flow, and add buffer checks #25958

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

anthonychen1251
Copy link
Member

This PR has three commits that:

  1. Fixes a potential integer overflow in ujson_parse_integer by using an unsigned integer for parsing and adding a check before converting back to a signed integer.
  2. Adds buffer size checks in perso_tlv_data.c to prevent potential memory corruption vulnerabilities.
  3. Removes the unused cert_size output parameter in dice_cert_check_valid

@anthonychen1251 anthonychen1251 requested a review from a team as a code owner January 21, 2025 12:45
@anthonychen1251 anthonychen1251 requested review from moidx and sasdf and removed request for a team January 21, 2025 12:45
return cert_x509_asn1_check_serial_number(cert_obj->cert_body_p, 0,
(uint8_t *)pubkey_id->digest,
cert_valid_output, &cert_size);
return cert_x509_asn1_check_serial_number(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should check the cert_body_size before calling the check_serial_number?

Copy link
Member Author

Choose a reason for hiding this comment

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

To clarify, are you proposing we compare the cert_obj->cert_body_size with the decoded size of the header using cert_x509_asn1_decode_size_header(&cert_obj->cert_body_p[0])?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm referring a check that ensure the body size is at least the minimum size required by cert_x509_asn1_check_serial_number, since that function doesn't take a size argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but isn't that check already included in

uint32_t cert_size =
cert_x509_asn1_decode_size_header(&cert_page_buffer[offset]);
if (out_cert_size != NULL) {
*out_cert_size = cert_size;
}
if (launder32(cert_size) < kCertX509Asn1FirstBytesWithSerialNumber) {
HARDENED_CHECK_LT(cert_size, kCertX509Asn1FirstBytesWithSerialNumber);
return kErrorOk;
}
? Although it will return kErrorOk when the size is not large enough to include a serial number.

The snippet below performs a sanity check on the certificate body size when populating the perso tlv object.

// Set pointer to certificate body.
obj->cert_body_size =
wrapped_cert_size - sizeof(perso_tlv_cert_header_t) - name_len;
obj->cert_body_p = buf;
// Sanity check on the certificate body size.
// TODO(24281): add sanity check on CWT certificate body size.
if (obj_type == kPersoObjectTypeX509Cert) {
size_t decoded_cert_size =
cert_x509_asn1_decode_size_header(obj->cert_body_p);
if (decoded_cert_size != obj->cert_body_size) {
return kErrorPersoTlvInternal;
}
}

That's why I asked if you were proposing we compare cert_obj->cert_body_size with cert_x509_asn1_decode_size_header(&cert_obj->cert_body_p[0]) (how cert_x509_asn1_check_serial_number() get the cert size actually ) here

Copy link
Contributor

Choose a reason for hiding this comment

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

Offline discussion with Anthony confirmed existing checks are sufficient, but moving the checks to make cert_x509_asn1_check_serial_number and its deps aware of the TLV buffer length is still more preferred. Also, with the ongoing cert template engine change, further simplification is possible to save space.

We will address it in a follow-up PR, and the current implementation remains for now.

Using a signed integer (int64_t) to store the parsed integer value could
lead to an undefined behavior when parsing large numbers due to signed
integer overflow. To address this issue, the parsed value is now stored
in an unsigned integer (uint64_t). The updated implementation also
includes a check to ensure that the parsed value can be safely converted
back to a signed integer before copying it into the `result`.

Signed-off-by: Anthony Chen <[email protected]>
This adds buffer size checks in `perso_tlv_data.c` to prevent potential
memory access vulnerabilities.

Signed-off-by: Anthony Chen <[email protected]>
This removes the unused `cert_size` output parameter in the
`dice_cert_check_valid` function.

Also, the original implementation caused a clang compiler error due to
incompatible pointer types by passing `&cert_size` (size_t *) to the
`cert_x509_asn1_check_serial_number` function (expects a uint32_t * type).

Signed-off-by: Anthony Chen <[email protected]>
@anthonychen1251 anthonychen1251 force-pushed the fix-minor-vulnerabilities branch from b2d9b87 to 0bdad63 Compare January 22, 2025 09:13
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.

2 participants