Should SignMessage guard against large metadata?

[asraa]

It's not likely to be a large metadata file, but should we watch out for it?

Originally posted by @trishankatdatadog in #357 (comment)

See here:

go-tuf/pkg/keys/keys.go

Line 57 in 4febe4c

SignMessage(message []byte) ([]byte, error)

SignMessge takes in an arbitrary length message bytes and hashes it before signing. In my opinion, sining is never done by the client so it's not as much of a threat.

On the other hand, the general procedure of unmarshalling metadata should come with JSON size limits. Client fetchers may already do this.

[znewman01]

👎 to adding length limits to SignMessage, you shouldn't be signing things you haven't validated or created yourself anyway

👍 to making SignMessage take in an io.Reader (which would enable using a LimitedReader if a caller cares)

On the other hand, the general procedure of unmarshalling metadata should come with JSON size limits. Client fetchers may already do this.

python-tuf has these limits. It enforces them at fetch-time (e.g., for root). I think that makes way more sense than doing it at SignMessage time.

[trishankatdatadog]

👎 to adding length limits to SignMessage, you shouldn't be signing things you haven't validated or created yourself anyway

👍 to making SignMessage take in an io.Reader (which would enable using a LimitedReader if a caller cares)

I don't feel strongly either way, but defence-in-depth is always a good idea.

On the other hand, the general procedure of unmarshalling metadata should come with JSON size limits. Client fetchers may already do this.

python-tuf has these limits. It enforces them at fetch-time (e.g., for root). I think that makes way more sense than doing it at SignMessage time.

Agree, this is where it's more important: during verify, not signing.

[rdimitrov]

Closing since the code base changed and this is no longer valid. Similar to python-tuf we do verify the length of the metadata when loading one.

Thanks for raising this 👍