Fix for variable clientDataJSON serialization

[wilsoncusack]

Our key assumption and optimization here turned out to be wrong: there are clients that have varied serialization of clientDataJSON.

E.g. in Mozilla the fields are ordered in the order they appear here.

With this change, our implementation basically mirrors Daimo and FreshCryptoLib, with the addition of the precompile call.

[wilsoncusack]

@mdehoog @rholterhus @xenoliss given all the steps we skip from the official spec, I am wondering if we care to keep this. Do we really care if it is webauthn.get vs. .create not sure.

[mdehoog]

Two questions:

• what's the worse that could happen if we skip?
• how much gas do we save by skipping (seems negligible at first glance)?

[xenoliss]

For gas saving, I won't comment as I haven't tested it.

What's the worst that could happen if we skip?

The specification is quite clear about it.

I guess whether to skip it or not depends on whether we want to restrict its usage to our use case only or make it more generic:

• For our usage only, I think skipping it is ok. In our specific case, we are not really interested in its value and simply assume that if the user signed it, then they must agree with whatever they just signed.

• For public usage (or other usages not related to our ERC4337 implementation), it might make sense to keep this verification to avoid "certain types of signature confusion attacks."

I'm always in favor of security, so if the gas is negligible, I would keep it. If it breaks at some point because an authenticator delivers assertion signatures with a type different than "webauthn.get", we can still upgrade the contract.

[rholterhus]

My opinion would also be to keep the check. I agree with the above analysis - the worst-case scenario would be if someone could reuse a signature from a registration ceremony. For the context of ERC4337, this is very unlikely to matter. Because the challenge in the registration ceremony has no reason to be equal to a hash of a UserOp, so there shouldn't ever be a collision. But for public usage it'd be good to keep the check.

[wilsoncusack]

Yeah I guess the only thought I had was whether we wanted to allow webauthn.create to be used for signing. But I am ok with the conclusion here. Will keep.

[rholterhus]

One thing I found interesting is that the slice() function doesn't revert if the start or end indices are greater than the length of the string. Instead of reverting, it'll just return a shorter string, for example see this part of the Solady code:

let subjectLength := mload(subject)
if iszero(gt(subjectLength, end)) { end := subjectLength }
if iszero(gt(subjectLength, start)) { start := subjectLength }

However I think this is fine. Because the only way that hash(a) == hash(b) is if a and b are equal. So if the user provides some weird values for challengeIndex or typeIndex such that it results in a shorter string, then the hashes aren't going to match, and verification will fail as expected.

So I think this is all okay, just leaving this note

[rholterhus]

I have one thing I've been thinking about recently. Technically, compared to the previous implementation, there's less of a guarantee that the "challenge":"..." and "type":"..." fields are actually occurring in the top level of the JSON structure. For example if a future version of WebAuthn has a field called arbitraryData in the clientDataJSON, then I'm imagining a JSON like this:

{
    "type":"webauthn.get",
    "challenge": "<challenge1>",
    "arbitraryData": "\"challenge\": \"<challenge2>\""
}

With this example, the arbitrary data contains a string that looks like a challenge field in the JSON, but really it's just a random string. In this scenario it might be possible to point the challengeIndex to the arbitrary string to prove a challenge the user wasn't expecting.

I think the implication here is that users need to be careful of everything they're signing, and they need to avoid clientDataJSON that have obvious red flags like the above. Maybe adding a note about this in the comments would be useful.

WDYT? The rest of the PR looks good to me pending this last note

[wilsoncusack]

Thanks for the note. And this sort of gets to why I question verify the type at all. Ultimately, anyone with a p256 key can format the data to get this test to pass. We don't know for sure that what is passed is the result of a proper webauthn authentication. Of course it's also possible that a wallet lies about what the transaction will be, and so the challenge is correct and it was a proper webauthn authentication, but the user is still surprised. We can't cover everything. I think the best we can do is: Did the data that you signed include the userOpHash? And, I guess per other discussion, was type: webauthn.get also present.

[stevieraykatz]

One small nit, otherwise looks good.

[stevieraykatz]

NIT: magic number 21 here should be described in a comment. I.E.

// bytes of string "type":"webauthn.get"

[wilsoncusack]

thanks, updated!

[cb-heimdall]

Review Error for stevieraykatz @ 2024-03-04 21:57:08 UTC
User must have write permissions to review

[cb-heimdall]

Review Error for stevieraykatz @ 2024-03-04 23:12:46 UTC
User must have write permissions to review