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

Should we allow unrecognized fields for the signatures dictionaries? #1802

Closed
MVrachev opened this issue Jan 27, 2022 · 9 comments
Closed

Should we allow unrecognized fields for the signatures dictionaries? #1802

MVrachev opened this issue Jan 27, 2022 · 9 comments
Assignees
Labels
1.0.0 blockers To be addressed before 1.0.0 release discussion Discussions related to the design, implementation and operation of the project
Milestone

Comments

@MVrachev
Copy link
Collaborator

Description of issue or feature request:
In ADR 8 we follow the Document formats section of the specification and allow unrecognized fields at each level of the metadata files.
The question is do we want to allow unrecognized fields inside the signatures dictionaries?
Are there possible security issues to that?

For context see the specification issue on the matter: theupdateframework/specification#203

@MVrachev MVrachev added the discussion Discussions related to the design, implementation and operation of the project label Jan 27, 2022
@jku
Copy link
Member

jku commented Jan 27, 2022

Lets decide this before 1.0 release (with the understanding that we might just decide to postpone)

@jku jku added the 1.0.0 blockers To be addressed before 1.0.0 release label Jan 27, 2022
@jku
Copy link
Member

jku commented Jan 27, 2022

Quoting myself in sslib issue:

It is true that we should be more cautious with this part of the metadata (not because they contain signatures but because they are not part of the signed portion of metadata). But: because of TUF design that requires canonicalization we end up parsing even the signed portion before we can check that it has been signed.

Because of this I think allowing unrecognised fields in signature does not significantly increase risk

Oh and the reason why we might want to do this: I believe supporting fulcio certificates/signatures requires extending the metadata. If unrecognised fields are not supported that would mean python-tuf 1.0 might not be able to read metadata written by python-tuf 1.x with fulcio support

@mnm678
Copy link
Contributor

mnm678 commented Jan 27, 2022

cc @asraa who may have insight into the Fulcio signatures.

When using canonical json, it's true that adding an additional field doesn't increase the risk, because we are already doing some somewhat risky parsing. If we switch to a format like DSSE, we'll want to avoid adding additional fields to the signature that could make the implementation vulnerable to a canonicalization attack.

@jku
Copy link
Member

jku commented Feb 2, 2022

To complete the options here: If we allow unrecognised_fields in Signature then there's nothing preventing allowing them in Metadata itself as well.

@asraa
Copy link

asraa commented Feb 2, 2022

cc @asraa who may have insight into the Fulcio signatures.

It was majorly convenient adding a cert field to the signature -- +1

@MVrachev
Copy link
Collaborator Author

MVrachev commented Feb 9, 2022

Together in a discussion with @jku and @lukpueh we agreed that it makes sense to implement unrecognized fields inside Metadata, Metadata.signatures and in the Signatures objects.
You can see read more about my reasoning here: theupdateframework/specification#203 (comment).

MVrachev added a commit to MVrachev/securesystemslib that referenced this issue Feb 9, 2022
The TUF specification says the following regarding unknown fields:
"All of the formats described below include the ability to add more
attribute-value fields to objects for backward-compatible format
changes. Implementers who encounter undefined attribute-value pairs in
the format must include the data when calculating hashes or verifying
signatures and must preserve the data when re-serializing."
From: https://theupdateframework.github.io/specification/latest/#document-formats

This section is the reason ADR0008 was accepted inside python-tuf
(see here: https://github.com/theupdateframework/python-tuf/blob/develop/docs/adr/0008-accept-unrecognised-fields.md)
and we have added support for unrecognized fields for all fields
inside the SIGNED portion of the metadata.
However, this limits what the citation implies or that everywhere there
inside the metadata files including signatures we should accept
unrecognized fields. That's why I made these changes.
These changes have the approval of the community see:
- theupdateframework/specification#203
- theupdateframework/python-tuf#1802

Signed-off-by: Martin Vrachev <[email protected]>
MVrachev added a commit to MVrachev/securesystemslib that referenced this issue Feb 9, 2022
The TUF specification says the following regarding unknown fields:
"All of the formats described below include the ability to add more
attribute-value fields to objects for backward-compatible format
changes. Implementers who encounter undefined attribute-value pairs in
the format must include the data when calculating hashes or verifying
signatures and must preserve the data when re-serializing."
From: https://theupdateframework.github.io/specification/latest/#document-formats

This section is the reason ADR0008 was accepted inside python-tuf
(see here: https://github.com/theupdateframework/python-tuf/blob/develop/docs/adr/0008-accept-unrecognised-fields.md)
and we have added support for unrecognized fields for all fields
inside the SIGNED portion of the metadata.
However, this limits what the citation implies or that everywhere there
inside the metadata files including signatures we should accept
unrecognized fields. That's why I made these changes.
These changes have the approval of the community see:
- theupdateframework/specification#203
- theupdateframework/python-tuf#1802

Signed-off-by: Martin Vrachev <[email protected]>
@jku
Copy link
Member

jku commented Feb 9, 2022

Metadata.signatures: this does not make sense. signatures is an array and the items are clearly defined as Signature objects (Signature objects are not named in the specification but the content is defined).

MVrachev added a commit to MVrachev/securesystemslib that referenced this issue Feb 9, 2022
The TUF specification says the following regarding unknown fields:
"All of the formats described below include the ability to add more
attribute-value fields to objects for backward-compatible format
changes. Implementers who encounter undefined attribute-value pairs in
the format must include the data when calculating hashes or verifying
signatures and must preserve the data when re-serializing."
From: https://theupdateframework.github.io/specification/latest/#document-formats

This section is the reason ADR0008 was accepted inside python-tuf
(see here: https://github.com/theupdateframework/python-tuf/blob/develop/docs/adr/0008-accept-unrecognised-fields.md)
and we have added support for unrecognized fields for all fields
inside the SIGNED portion of the metadata.
However, this limits what the citation implies or that everywhere there
inside the metadata files including signatures we should accept
unrecognized fields. That's why I made these changes.
These changes have the approval of the community see:
- theupdateframework/specification#203
- theupdateframework/python-tuf#1802

Another change I had to do, so I can add unrecognized fields support
inside Signature is to make "Signature.from_dict()" to behave the same
way as the rest of the "from_dict()" functions inside TUF or destroy
the input dictionary.
This was necessary, as that provides me with an easy way to pass the
unrecognized fields to the constructor.

Signed-off-by: Martin Vrachev <[email protected]>
MVrachev added a commit to MVrachev/securesystemslib that referenced this issue Feb 9, 2022
The TUF specification says the following regarding unknown fields:
"All of the formats described below include the ability to add more
attribute-value fields to objects for backward-compatible format
changes. Implementers who encounter undefined attribute-value pairs in
the format must include the data when calculating hashes or verifying
signatures and must preserve the data when re-serializing."
From: https://theupdateframework.github.io/specification/latest/#document-formats

This section is the reason ADR0008 was accepted inside python-tuf
(see here: https://github.com/theupdateframework/python-tuf/blob/develop/docs/adr/0008-accept-unrecognised-fields.md)
and we have added support for unrecognized fields for all fields
inside the SIGNED portion of the metadata.
However, this limits what the citation implies or that everywhere there
inside the metadata files including signatures we should accept
unrecognized fields. That's why I made these changes.
These changes have the approval of the community see:
- theupdateframework/specification#203
- theupdateframework/python-tuf#1802

Another change I had to do, so I can add unrecognized fields support
inside Signature is to make "Signature.from_dict()" to behave the same
way as the rest of the "from_dict()" functions inside TUF or destroy
the input dictionary.
This was necessary, as that provides me with an easy way to pass the
unrecognized fields to the constructor.

Signed-off-by: Martin Vrachev <[email protected]>
@MVrachev
Copy link
Collaborator Author

MVrachev commented Feb 9, 2022

Okay, my bad, I misunderstood.
Thanks for the clarification.

@MVrachev
Copy link
Collaborator Author

MVrachev commented Feb 9, 2022

I will close this issue as it's related to a securesystemslib code and I will create a new one for Metadata unrecognized fields.

@MVrachev MVrachev closed this as completed Feb 9, 2022
MVrachev added a commit to MVrachev/securesystemslib that referenced this issue Feb 16, 2022
The TUF specification says the following regarding unknown fields:
"All of the formats described below include the ability to add more
attribute-value fields to objects for backward-compatible format
changes. Implementers who encounter undefined attribute-value pairs in
the format must include the data when calculating hashes or verifying
signatures and must preserve the data when re-serializing."
From: https://theupdateframework.github.io/specification/latest/#document-formats

This section is the reason ADR0008 was accepted inside python-tuf
(see here: https://github.com/theupdateframework/python-tuf/blob/develop/docs/adr/0008-accept-unrecognised-fields.md)
and we have added support for unrecognized fields for all fields
inside the SIGNED portion of the metadata.
However, this limits what the citation implies or that everywhere there
inside the metadata files including signatures we should accept
unrecognized fields. That's why I made these changes.
These changes have the approval of the community see:
- theupdateframework/specification#203
- theupdateframework/python-tuf#1802

Another change I had to do, so I can add unrecognized fields support
inside Signature is to make "Signature.from_dict()" to behave the same
way as the rest of the "from_dict()" functions inside TUF or destroy
the input dictionary.
This was necessary, as that provides me with an easy way to pass the
unrecognized fields to the constructor.

Signed-off-by: Martin Vrachev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0.0 blockers To be addressed before 1.0.0 release discussion Discussions related to the design, implementation and operation of the project
Projects
None yet
Development

No branches or pull requests

4 participants