On 6/5/26 3:20 PM, Zhuoying Cai wrote:
Thanks for the feedback!
On 5/24/26 6:32 PM, Jared Rossi wrote:
On 5/5/26 4:18 PM, Zhuoying Cai wrote:
[...]
+
+ rc = handle_hash(vce, cert, keyid_field_len);
+ if (rc) {
+ return -1;
+ }
+ hash_field_len = ROUND_UP(be16_to_cpu(vce->vce_hdr.hash_len), 4);
+
+ rc = handle_cert(vce, cert, hash_field_len);
+ if (rc || !is_cert_valid(cert)) {
+ return -1;
+ }
+ cert_field_len = ROUND_UP(be32_to_cpu(vce->vce_hdr.cert_len), 4);
+
+ vce_len = sizeof(VCEntryHeader) + keyid_field_len + hash_field_len +
cert_field_len;
+ if (vce_len > be32_to_cpu(vce->vce_hdr.len)) {
+ return -1;
+ }
We already used the maximum size when we allocated space for the VCE, so
this
check is redundant I think?
Although exceeding the maximum size is unlikely, I think it’s good
practice to keep this check, along with those in the handle_* functions.
These checks help ensure that the actual lengths of the certificate
metadata retrieved from the qcrypto functions remain within the defined
limits.
Please let me know what you think.
After taking another look, I realize this check is not redundant in the
way I
thought it was. As far as I can tell, when building the cert store, the
size
of each entry is checked using the largest_cert_size variable, but we never
validate that this value is within our max cert size. So the check here is
actually the first time we validate the total size.
In my opinion we should check this in the update_cert_store() function
that was
introduced in Patch 4 and gets called as the cert store is populated. I
guess
this ties back in to some of the other comments about defining
MAX_ENTRY_SIZE
earlier in the series.
Maybe we could detect invalid certs earlier if we add a secondary check that
data_buf_size <= CERT_BUF_MAX_LEN when updating largest_cert_size. I
think both
update_cert_store() and build_vce_data() are doing the same size
calculations,
right?
I think we have all of this information available to reject certs before
they
are added to the cert store.
Regards,
Jared Rossi
