missing EC public key validation when creating a new Verifier

[thomas-fossati]

Currently, when we instantiate a Verifier we do not check that the supplied public key EC point is on the supposed curve.

This is not best practice. See for example BCP225 in the JWS/JWT context:

"Some cryptographic operations [...] take inputs that may contain invalid values. This includes points not on the specified elliptic curve or other invalid points (e.g., [Valenta], Section 7.1). The JWS/JWE library itself must validate these inputs before using them, or it must use underlying cryptographic libraries that do so (or both!)."

It'd also result in a panic when the go-cose application is built against go1.19. See go1.19 release notes:

"Operating on invalid curve points (those for which the IsOnCurve method returns false, and which are never returned by Unmarshal or by a Curve method operating on a valid point) has always been undefined behavior and can lead to key recovery attacks. If an invalid point is supplied to Marshal, MarshalCompressed, Add, Double, or ScalarMult, they will now panic."

This can potentially widen the DoS surface of a go-cose application. Users should be shielded from such situation.

[ivarprudnikov]

when we instantiate a Verifier <...> This can potentially widen the DoS surface of a go-cose application

The implementer is passing a public key and currently it is on them to make sure it is a correct one. Nothing prevents the implementer from making the appropriate checks before using their own key. Furthermore, it is a reasonable expectation to be provided with a valid argument to begin with.

It is interesting though that Go went ahead with a panic and not the error in such a case.

[thomas-fossati]

Furthermore, it is a reasonable expectation to be provided with a valid argument to begin with.

I don't know you, but for an old-school derelict like myself input validation has always been no. 1 rule of secure coding practices 😄

It is interesting though that Go went ahead with a panic and not the error in such a case.

I agree with you that was a pretty interesting choice -- which makes upfront validation without crashing the caller even more valuable.

[ivarprudnikov]

input validation has always been no. 1 rule

But you get that key from somewhere, are you saying you do not verify it before using in the library?

[thomas-fossati]

[...] are you saying you do not verify it before using in the library?

at the moment we just check that the underlying type is as expected (see

go-cose/verifier.go

Line 45 in 4dbb9a7

vk, ok := key.(*ecdsa.PublicKey)

). 064ff82 adds the "on curve" test.

[ivarprudnikov]

at the moment we just check that the underlying type is as expected

^^ My question was more about the production code that gets the key from somewhere and passes it to this library for a verification.

Otherwise, I am just trying to understand if it is a good idea to add this curve check to the library, because:

• Currently the library will defer to what the native go implementation does which is either a good idea or not.
• If the library wants to shield from panic, then one needs to be sure to implement the same checks as in native go that result in such a panic which might mean additional burden on maintenance.
• As it stands, folks who use the library need to do the IsOnCurve(x, y) check already because otherwise there might be a panic as the library does not do that check for them.
• Affected methods are Marshal, MarshalCompressed, Add, Double, ScalarMult that would result in a panic. None of these methods are used here it seems