Extract and validate client challenge #83
Conversation
- Use base64url encoding - Add copyBytes function to replace assembly calldatacopy logic - Update webauthn sig values - Use updated clientChallenge
JohnGuilding
force-pushed
the
44-extract-webauthn-challenge-from-clientData-and-compare-to-clientChallenge
branch
from
a86e783 to
dee5171
Compare
| s.clientChallengeDataOffset | ||
| ) = abi.decode( | ||
| userOp.signature[i:i+dataLen], | ||
| ( | ||
| bytes1, | ||
| uint256, //not sure what is encoded here | ||
| bytes32, |
There was a problem hiding this comment.
^I'm not sure why this is the order for the decoding, I originally assumed it would have been bytes before uints like so:
(
bytes1,
bytes32,
uint256,
uint256
)The screenshot below shows the difference between the two signatures. The older signature without the clientChallenge is on the left, and the newer signature with the clientChallenge is on the right:
You can see where the clientChallenge was added, look for 353a3ed5a0441919f1c639a46931de872ac3357de2ce5aa2d68c2639df54189d. There were some other changes which I'm not too sure about. It looks like the mystery uint256 changed. The value in the first 32 bytes was also changed, again not sure what that change means - @jzaki any ideas?
There was a problem hiding this comment.
The ordering of these fixed length vars is as they were encoded.
The additional 0x20 (32) on the length is the additional 32 bytes added.
The mystery number seems to be a length, since it also increased by 0x20. Will create an issue to get to the bottom of why and how that gets encoded in.
| s.clientChallengeDataOffset | ||
| ) = abi.decode( | ||
| userOp.signature[i:i+dataLen], | ||
| ( | ||
| bytes1, | ||
| uint256, //not sure what is encoded here | ||
| bytes32, |
There was a problem hiding this comment.
The ordering of these fixed length vars is as they were encoded.
The additional 0x20 (32) on the length is the additional 32 bytes added.
The mystery number seems to be a length, since it also increased by 0x20. Will create an issue to get to the bottom of why and how that gets encoded in.
| @@ -135,4 +132,4 @@ library FCL_WebAuthn { | |||
|
|
|||
| return result; | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
I assume this file is now back to it's original version?
JohnGuilding
deleted the
44-extract-webauthn-challenge-from-clientData-and-compare-to-clientChallenge
branch
This PR properly extracts the client challenge and validates it. The reason this wasn't working originally is because the code actually needed to use
base64urlencoding instead ofbase64encoding to encode theclientChallengeinFCL_WebAuthn.Base64URL encoding
The
clientChallengewith base64url encoding is equal toNTo-1aBEGRnxxjmkaTHehyrDNX3izlqi1owmOd9UGJ0. Whereas the base64 encoding of that isNTo+1aBEGRnxxjmkaTHehyrDNX3izlqi1owmOd9UGJ0=. This resulted in the hash being incorrect which the extracted challenge was compared to. Therefore the assertion failed and an error was thrown.Why did the original ledger implementation not use base64URL encoding?
It turns out the implementation located in
src/is different to the implementation that is tested against intests/. If you look at the Fresh Crypto Lib (FCL) implementation, you can see that it does indeed usebase64encoding. Since I copied that file originally, I expected that to work. It turns out that the FCL tests do actually usebase64urlencoding instead! See this link here.Does that mean we always have to use
base64urlencoding now?TLDR: Not sure. Need to refresh my knowledge of how webauthn handles encoding
clientData, and whether that is something we can manage. Ideally the aim is to not edit the FCL library contract at all. Added an issue to investigate this further - #84### copyBytes functionSince we haven't implemented #68 yet, we couldn't use the originalclientChallengevalidation logic still, as that relied on the arguments being stored ascalldata. As a temporary solution until we fix that, I've chosen to use a function calledcopyBytes. This is taken from the original webauthn solidity implementation whichFCL_WebAuthnis based on. You can see the original code for that here.^ #68 has been merged so we don't need to use the
copyBytesfunction anymoreThe PR also updates the webauthn sig values used throughout the tests to use a valid clientChallenge (I was using incorrect values such as the userOp hash before)