search

contentauth / c2pa-rs

Rust SDK for the core C2PA (Coalition for Content Provenance and Authenticity) specification
Other
128 stars 56 forks source link

pad_cose_sig() not correctly calculating padding size #492

Open vancejc-mt opened 4 months ago

vancejc-mt commented 4 months ago

If the reserve size returned by AsyncSigner::reserve_size() is fairly close (but not exact) to the size of the Cose_Sign1 structure which c2pa-rs generates, we're seeing CoseSigboxTooSmall errors be returned.

The behavior of this is somewhat weird, and I believe it's because the padding calculation (let mut target_guess = end_size - cur_size - PAD_OFFSET) doesn't accurately account for the size which the header itself will take up once it's DER encoded. Thus it calculates a pad which is too large which then causes the reserve size to be overwhelmed.

I don't believe this should ever be the case - if the reserve size is larger than the size of the eventual Cose_Sign1 structure, this should succeed.

Maybe this is an understanding on my part? But to get this to work correctly, we're having to add in overly big reserve sizes to avoid the issue.

mauricefisher64 commented 4 months ago

This needs to be documented better, The reserve_size IS the is the size of the fiinal CoseSign1 cbor (not anything to do with DER). The reserve_size must be large enough the cover the completed size. Any extra space is absorbed by the padding fields in the CoseSign1. If the cbor ends up too big you will get a box too small error

mauricefisher64 commented 4 months ago

To calc the reserve_size you must account for the size of [chain chain + timestamp response + OCSP response + size of Claim cbor]

vancejc-mt commented 4 months ago

Understood - this was referencing what I believe to be a bug though. If the reserve size is larger than the size needed for the CoseSign1 cbor, but only by a small amount, the padding calculation in the SDK I think is incorrect. It ends up adding too much padding which then causes a CoseSigboxTooSmall error. It seems he's not correctly taking into account (or maybe incorrectly calculating) that adding padding requires the variable size integer which describes the padding size.

mauricefisher64 commented 4 months ago

I will take a look

mauricefisher64 commented 4 months ago

Still curious why someone would try to get within 7 bytes of the limit. Also the DER encoding has to be accounted for by the Signer not us.

vancejc-mt commented 4 months ago

We were trying to get the size down as small as possible when we ran into this - we ended up backing off and adding more padding, but thought this may be interesting as an issue.