Add guidance about unknown fields

[asraa]

#163

Addressing this, specify that implementers should preserve unknown key-value pairs and include them in calculating hashes or verifying signatures.

I would like to add examples here, but it feels like an awkward place if the format hasn't been defined at this point in the spec.

Signed-off-by: Asra Ali asraa@google.com

[mnm678]

Thanks @asraa! The text of the change looks great, but could you remove the whitespace changes? They make the diff hard to read.

[asraa]

Sorry about that! Must've been auto-formatting.

[trishankatdatadog]

I can't think of how this PR affects security, at least for TUF purposes. Did I miss anything? @mnm678@SantiagoTorres@JustinCappos

[trishankatdatadog]

Suggested change

	attribute-value fields to dictionary objects for backwards-compatible format
	key-value fields to dictionary objects for backwards-compatible format

[joshuagl]

The rest of the spec is fairly consistent in referring to them as attributes and attribute-value fields.

[joshuagl]

I think for this PR we should revert to attribute-value fields, to be consistent.

We might want to edit the entire spec to change the object related terminology. Filed as #175

[mnm678]

I can't think of how this PR affects security, at least for TUF purposes. Did I miss anything?

Well, someone could try some kind of code injection through the unknown field, but it's signed metadata, so that's pretty unlikely.

[asraa]

Well, someone could try some kind of code injection through the unknown field, but it's signed metadata, so that's pretty unlikely.

This would be like someone adding an strange field that disappears when canonicalizing and verifying the signature, right? Did this ever have some discussion with regards to the CUSTOM targets dictionary? I suppose one mitigation is that users must consume canonicalized custom key-values or something like that.

[trishankatdatadog]

This would be like someone adding an strange field that disappears when canonicalizing and verifying the signature, right? Did this ever have some discussion with regards to the CUSTOM targets dictionary? I suppose one mitigation is that users must consume canonicalized custom key-values or something like that.

I think Marina means something else, but I'm not sure what

[mnm678]

I was thinking of a json parsing vulnerability. But any of these attacks would require an attacker to tamper with signed metadata, so there shouldn't be a security implication.

[trishankatdatadog]

I was thinking of a json parsing vulnerability. But any of these attacks would require an attacker to tamper with signed metadata, so there shouldn't be a security implication.

Right, and any attacker who controls the key can do this without unknown key-value fields anyway.

Asra, what attack did you have in mind?

[asraa]

Asra, what attack did you have in mind?

I thought similar -- as in an attacker adds some specially crafted fields that when canonicalized disappear, so the signature is still verified, by a client thinks there's some extra "verified" data. This may not be possible though (and is obviously visible in the files)

[trishankatdatadog]

I thought similar -- as in an attacker adds some specially crafted fields that when canonicalized disappear, so the signature is still verified, by a client thinks there's some extra "verified" data. This may not be possible though (and is obviously visible in the files)

Yeap, the good news is that they will be caught, provided we log everything that goes in and out 🙂

The safest thing to do is to explicitly disallow superset fields, but I'm afraid this is too restrictive for valid, real-world use cases.

I can see how TUF metadata can be used to carry other things that might lead to insecurity, but I don't see how that is directly a problem, so long as it does not break TUF's security guarantees. This is already doable with the targets themselves anyway, not to mention the custom targets metadata.

[trishankatdatadog]

BTW, this has implications for the sigstore key ceremony this Friday, which uses superset fields to record YubiKey attestations alongside public keys in their TUF root metadata. So, if anyone is really concerned, please speak up ASAP.

[trishankatdatadog]

I can't think of major security issues right now with this change. Does anyone see issues? @JustinCappos?

[joshuagl]

I can't think of a security issue with the change either, I did suggest we change some phrasing to be more consistent with the rest of the spec #169 (comment) (but I also prefer the phrasing used in this PR, and think we should convert the entire spec to use it #175)

[trishankatdatadog]

Approved, but I want everyone here to think about this

[joshuagl]

LGTM, thanks @asraa – just a minor suggestion to use the specification standard terms.