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.

Sign up for GitHub

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

Jump to bottom

SSlibKey: consider stronger validation of keytype/scheme for keyval #766

Open
lukpueh opened this issue Apr 3, 2024 · 1 comment
Open

SSlibKey: consider stronger validation of keytype/scheme for keyval #766

lukpueh opened this issue Apr 3, 2024 · 1 comment
Labels
discussion Issues that require discussion

Comments

@lukpueh
Copy link
Member

lukpueh commented Apr 3, 2024

The consistency of SSlibKey instances is mainly "validated" at usage time. That is, in SSlibKey.verify_signature, when keyval is deserialised based on scheme, unknown schemes and undeserializable keyvals are filtered out. This validation is incomplete and also happens late.

For comparison, at creation time inputs are hardly validated (see e.g. in from_dict or in the base constructor). Note that an additional safeguard exists in the Key.from_dict deserialisation interface, which filters out unregistered keytype, scheme pairs.

Let's consider:

  • adding more comprehensive validation, most notably check consistency of keytype, scheme, and keyval, and
  • validating earlier, e.g. already in the constructor

See related issues related to invalid SSlibKey instances and validation: #764 #765, #669, #559
@lukpueh lukpueh added the discussion Issues that require discussion label Apr 3, 2024
@lukpueh
Copy link
Member Author

lukpueh commented Apr 3, 2024

An argument against more comprehensive and/or earlier validation is that a key might be deserialized but never used for signature verification. In that case it might not matter if the key is invalid.

Similarly, a key might be inconsistent, but still manage to verify a signature. This is likely the case for ecdsa keys, e.g. if the keyval contains a nistp256 key, but scheme is "ecdsa-sha2-nistp384" (or vice-versa).

I lean towards checking early and comprehensively, and failing hard if an SSlibKey is inconsistent. If we want to support deserialisation of unknown keys, we should implement this in the Key.from_dict interface.

lukpueh added a commit to lukpueh/securesystemslib that referenced this issue Apr 24, 2024
@lukpueh
Fix verify test with ecdsa-sha2-nistp384 key
d3c9ca9 
Fix test, which successfully verifies a signature from an ecdsa
nistp256 key, using a "ecdsa-sha2-nistp384" key.

This only worked because keytype/scheme/keyval combos in an SSlibKey
are not validated sufficiently (see secure-systems-lab#766).

The test data was created using below snippet. Note how the signature is
not created with CryptoSigner, which does not support nistp384.

```
from cryptography.hazmat.primitives.asymmetric.ec import (
    ECDSA,
    SECP384R1,
    EllipticCurvePrivateKey,
)
from cryptography.hazmat.primitives.hashes import SHA384
from cryptography.hazmat.primitives.serialization import load_pem_private_key

from securesystemslib.signer import SSlibKey

with open("tests/data/pems/ecdsa_secp384r1_private.pem", "rb") as f:
    priv: EllipticCurvePrivateKey = load_pem_private_key(f.read(), None)

assert isinstance(priv.curve, SECP384R1)

pub = SSlibKey.from_crypto(priv.public_key())
sig = priv.sign(b"DATA", ECDSA(SHA384()))

print(pub.keyid)
print(sig.hex())
print(repr(pub.keyval["public"]).strip("'"))
```

Signed-off-by: Lukas Puehringer <[email protected]>
@lukpueh lukpueh mentioned this issue Apr 24, 2024
Merged
lukpueh added a commit to lukpueh/securesystemslib that referenced this issue Apr 24, 2024
@lukpueh
Improve SSlibKey validation in _verify
5ec0e4c 
Added checks as requrested in secure-systems-lab#766:

- keytype matches scheme
- scheme matches deserialized key type (only for pem formatted keyvals;
  for ed25519 the check happens implicitly on deserialization)
- scheme matches deserialized key curve (only for ecdsa)

Note that this PR does not move the checks to the constructor as
suggested in secure-systems-lab#766. This may or may not be addressed in a follow-up PR.

failing tests fixed in secure-systems-lab#794

Signed-off-by: Lukas Puehringer <[email protected]>
@lukpueh lukpueh mentioned this issue Apr 24, 2024
Merged
lukpueh added a commit to lukpueh/securesystemslib that referenced this issue Apr 24, 2024
@lukpueh
Improve SSlibKey validation in _verify
b409d6b 
Added checks as requrested in secure-systems-lab#766:

- keytype matches scheme
- scheme matches deserialized key type (only for pem formatted keyvals;
  for ed25519 the check happens implicitly on deserialization)
- scheme matches deserialized key curve (only for ecdsa)

Note that this PR does not move the checks to the constructor as
suggested in secure-systems-lab#766. This may or may not be addressed in a follow-up PR.

failing tests fixed in secure-systems-lab#794

Signed-off-by: Lukas Puehringer <[email protected]>
lukpueh added a commit to lukpueh/securesystemslib that referenced this issue Apr 25, 2024
@lukpueh
Fix verify test with ecdsa-sha2-nistp384 key
c72a946 
Fix test, which successfully verifies a signature from an ecdsa
nistp256 key, using a "ecdsa-sha2-nistp384" key.

This only worked because keytype/scheme/keyval combos in an SSlibKey
are not validated sufficiently (see secure-systems-lab#766).

The test data was created using below snippet. Note how the signature is
not created with CryptoSigner, which does not support nistp384.

```
from cryptography.hazmat.primitives.asymmetric.ec import (
    ECDSA,
    SECP384R1,
    EllipticCurvePrivateKey,
)
from cryptography.hazmat.primitives.hashes import SHA384
from cryptography.hazmat.primitives.serialization import load_pem_private_key

from securesystemslib.signer import SSlibKey

with open("tests/data/pems/ecdsa_secp384r1_private.pem", "rb") as f:
    priv: EllipticCurvePrivateKey = load_pem_private_key(f.read(), None)

assert isinstance(priv.curve, SECP384R1)

pub = SSlibKey.from_crypto(priv.public_key())
sig = priv.sign(b"DATA", ECDSA(SHA384()))

print(pub.keyid)
print(sig.hex())
print(repr(pub.keyval["public"]).strip("'"))
```

Signed-off-by: Lukas Puehringer <[email protected]>
lukpueh added a commit to lukpueh/securesystemslib that referenced this issue Apr 25, 2024
@lukpueh
Improve SSlibKey validation in _verify
3f70df5 
Added checks as requrested in secure-systems-lab#766:

- keytype matches scheme
- scheme matches deserialized key type (only for pem formatted keyvals;
  for ed25519 the check happens implicitly on deserialization)
- scheme matches deserialized key curve (only for ecdsa)

Note that this PR does not move the checks to the constructor as
suggested in secure-systems-lab#766. This may or may not be addressed in a follow-up PR.

failing tests fixed in secure-systems-lab#794

Signed-off-by: Lukas Puehringer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
discussion Issues that require discussion
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@lukpueh