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

Validate key sizes and allow signing with empty public keys #159

Merged
merged 3 commits into from
Jul 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 65 additions & 31 deletions key.go
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,15 @@ func NewKeyFromPrivate(priv crypto.PrivateKey) (*Key, error) {
}
}

var (
// The following errors are used multiple times
// in Key.validate. We declare them here to avoid
// duplication. They are not considered public errors.
errCoordOverflow = fmt.Errorf("%w: overflowing coordinate", ErrInvalidKey)
errReqParamsMissing = fmt.Errorf("%w: required parameters missing", ErrInvalidKey)
errInvalidCurve = fmt.Errorf("%w: curve not supported for the given key type", ErrInvalidKey)
)

// Validate ensures that the parameters set inside the Key are internally
// consistent (e.g., that the key type is appropriate to the curve).
// It also checks that the key is valid for the requested operation.
Expand All @@ -432,14 +441,20 @@ func (k Key) validate(op KeyOp) error {
}
}
if crv == CurveInvalid || (len(x) == 0 && len(y) == 0 && len(d) == 0) {
return ErrInvalidKey
return errReqParamsMissing
}
if size := curveSize(crv); size > 0 {
// RFC 8152 Section 13.1.1 says that x and y leading zero octets MUST be preserved,
// but the Go crypto/elliptic package trims them. So we relax the check
// here to allow for omitted leading zero octets, we will add them back
// when marshaling.
if len(x) > size || len(y) > size || len(d) > size {
return errCoordOverflow
}
}
switch crv {
case CurveX25519, CurveX448, CurveEd25519, CurveEd448:
return fmt.Errorf(
"Key type mismatch for curve %q (must be OKP, found EC2)",
crv.String(),
)
return errInvalidCurve
default:
// ok -- a key may contain a currently unsupported curve
// see https://www.rfc-editor.org/rfc/rfc8152#section-13.1.1
Expand All @@ -457,25 +472,25 @@ func (k Key) validate(op KeyOp) error {
}
}
if crv == CurveInvalid || (len(x) == 0 && len(d) == 0) {
return ErrInvalidKey
return errReqParamsMissing
}
if (len(x) > 0 && len(x) != ed25519.PublicKeySize) || (len(d) > 0 && len(d) != ed25519.SeedSize) {
return errCoordOverflow
}
switch crv {
case CurveP256, CurveP384, CurveP521:
return fmt.Errorf(
"Key type mismatch for curve %q (must be EC2, found OKP)",
crv.String(),
)
return errInvalidCurve
default:
// ok -- a key may contain a currently unsupported curve
// see https://www.rfc-editor.org/rfc/rfc8152#section-13.2
}
case KeyTypeSymmetric:
k := k.Symmetric()
if len(k) == 0 {
return ErrInvalidKey
return errReqParamsMissing
}
case KeyTypeInvalid:
return errors.New("invalid kty value 0")
return fmt.Errorf("%w: kty value 0", ErrInvalidKey)
default:
// Unknown key type, we can't validate custom parameters.
}
Expand Down Expand Up @@ -540,6 +555,18 @@ func (k *Key) MarshalCBOR() ([]byte, error) {
existing[lbl] = struct{}{}
tmp[lbl] = v
}
if k.KeyType == KeyTypeEC2 {
// If EC2 key, ensure that x and y are padded to the correct size.
crv, x, y, _ := k.EC2()
if size := curveSize(crv); size > 0 {
if 0 < len(x) && len(x) < size {
tmp[KeyLabelEC2X] = append(make([]byte, size-len(x), size), x...)
}
if 0 < len(y) && len(y) < size {
tmp[KeyLabelEC2Y] = append(make([]byte, size-len(y), size), y...)
}
}
}
return encMode.Marshal(tmp)
}

Expand Down Expand Up @@ -672,14 +699,6 @@ func (k *Key) PrivateKey() (crypto.PrivateKey, error) {

switch alg {
case AlgorithmES256, AlgorithmES384, AlgorithmES512:
_, x, y, d := k.EC2()
// RFC8152 allows omitting X and Y from private keys;
// crypto.PrivateKey assumes they are available.
// see https://www.rfc-editor.org/rfc/rfc8152#section-13.1.1
if len(x) == 0 || len(y) == 0 {
return nil, ErrEC2NoPub
}

var curve elliptic.Curve

switch alg {
Expand All @@ -691,22 +710,24 @@ func (k *Key) PrivateKey() (crypto.PrivateKey, error) {
curve = elliptic.P521()
}

priv := &ecdsa.PrivateKey{
PublicKey: ecdsa.PublicKey{Curve: curve, X: new(big.Int), Y: new(big.Int)},
D: new(big.Int),
_, x, y, d := k.EC2()
var bx, by *big.Int
if len(x) == 0 || len(y) == 0 {
bx, by = curve.ScalarBaseMult(d)
} else {
bx = new(big.Int).SetBytes(x)
by = new(big.Int).SetBytes(y)
}
priv.X.SetBytes(x)
priv.Y.SetBytes(y)
priv.D.SetBytes(d)
bd := new(big.Int).SetBytes(d)

return priv, nil
return &ecdsa.PrivateKey{
PublicKey: ecdsa.PublicKey{Curve: curve, X: bx, Y: by},
D: bd,
}, nil
case AlgorithmEd25519:
_, x, d := k.OKP()
// RFC8152 allows omitting X from private keys;
// crypto.PrivateKey assumes it is available.
// see https://www.rfc-editor.org/rfc/rfc8152#section-13.2
if len(x) == 0 {
return nil, ErrOKPNoPub
return ed25519.NewKeyFromSeed(d), nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a general question here: What does d mean for Ed25519 in terms of OKP? Is it private key or seed?

RFC 8152 13.2 says that

d: This contains the private key.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d is a private key component of a crv, but my understanding of the specs is that it can be anything sufficiently random... so in the case that you derive a seed from a mnemonic or other details, you might use the seed as the private key... thats all super dangerous stuff... and I don't think any of it is relevant to COSE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name is a little misleading. Its documentation is more interesting:

NewKeyFromSeed calculates a private key from a seed. It will panic if len(seed) is not SeedSize. This function is provided for interoperability with RFC 8032. RFC 8032's private keys correspond to seeds in this package.

RFC8152 8.2 says that EdDSA curves should be implemented following RFC8032, so my understanding is that it is safe to use NewKeyFromSeed here.

}

buf := make([]byte, ed25519.PrivateKeySize)
Expand Down Expand Up @@ -820,6 +841,19 @@ func algorithmFromEllipticCurve(c elliptic.Curve) Algorithm {
}
}

func curveSize(crv Curve) int {
var bitSize int
switch crv {
case CurveP256:
bitSize = elliptic.P256().Params().BitSize
case CurveP384:
bitSize = elliptic.P384().Params().BitSize
case CurveP521:
bitSize = elliptic.P521().Params().BitSize
}
return (bitSize + 7) / 8
}

func decodeBytes(dic map[interface{}]interface{}, lbl interface{}) (b []byte, ok bool, err error) {
val, ok := dic[lbl]
if !ok {
Expand Down
Loading