Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Extract and validate client challenge #83

Merged
Show file tree
Hide file tree
Changes from 2 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
21 changes: 16 additions & 5 deletions account-integrations/safe/README.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,20 @@
# Webauthn plugin
# Safe plugins

# Getting Started

1. (from this directory)
1. `cd account-integrations/safe`
2. Run `yarn` to install hardhat dependencies
3. Run `forge install` to install foundry dependencies

## Integration test
## Forge tests

To run the integration tests:
```bash
forge test
```

## Hardhat tests

To run the hardhat tests, you'll need to run a node and a bundler as some of them are integration tests:

1. Start a geth node, fund accounts and deploy Safe contracts:

Expand All @@ -25,7 +31,12 @@ cp .env.example .env
3. Setup and run an external bundler (make sure the values in `.env` match up with the bundler and node you're running).

```bash
# If using the eth-infinitism bundler (tested with commit #1b154c9)
# If using the eth-infinitism bundler, checkout to this commmit. The latest version of the bundler has started breaking the integration tests. This is a previous commit where the integration tests still pass
git checkout 1b154c9
```

```bash
# If using the eth-infinitism bundler
yarn run bundler
```

Expand Down
9 changes: 6 additions & 3 deletions account-integrations/safe/src/SafeWebAuthnPlugin.sol
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ contract SafeWebAuthnPlugin is BaseAccount {

struct LocalVarStruct {
bytes1 authenticatorDataFlagMask;
bytes32 clientChallenge;
uint256 clientChallengeDataOffset;
}

Expand All @@ -87,20 +88,22 @@ contract SafeWebAuthnPlugin is BaseAccount {
uint i = 0;
uint dataLen = 32;
uint256 paramLen = abi.decode(userOp.signature[i:i+dataLen], (uint256));
// Fixed-length params (bytes1, (uint256?), uint256, uint256[2], uint256[2]). Expect 8 slots = 256 bytes
// Fixed-length params (bytes1, (uint256?), bytes32, uint256, uint256[2], uint256[2]). Expect 9 slots = 256 bytes
i += dataLen; // advance index

// decode fixed length params (values to memory)
dataLen = 3 * 32; //lenFixedParams - 32; // -32 already read length
dataLen = 4 * 32; //lenFixedParams - 32; // -32 already read length
(
s.authenticatorDataFlagMask,
, // some number
s.clientChallenge,
s.clientChallengeDataOffset
) = abi.decode(
userOp.signature[i:i+dataLen],
(
bytes1,
uint256, //not sure what is encoded here
bytes32,
Copy link
Contributor Author

@JohnGuilding JohnGuilding Sep 15, 2023

Choose a reason for hiding this comment

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

^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:

Screenshot 2023-09-15 at 16 12 30

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

uint256
)
);
Expand Down Expand Up @@ -146,7 +149,7 @@ contract SafeWebAuthnPlugin is BaseAccount {
authenticatorData,
s.authenticatorDataFlagMask,
clientData,
userOpHash,
s.clientChallenge,
s.clientChallengeDataOffset,
signature,
pubKey
Expand Down
105 changes: 105 additions & 0 deletions account-integrations/safe/src/lib/Base64URL.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
// SPDX-License-Identifier: MIT
// from OpenZeppelin Contracts (last updated v4.7.0) (utils/Base64.sol)

pragma solidity ^0.8.0;

/**
* @dev Provides a set of functions to operate with Base64 strings.
*
* _Available since v4.5._
*/
library Base64URL {
/**
* @dev Base64 Encoding/Decoding Table
*/
string internal constant _TABLE =
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_";

/**
* @dev Converts a `bytes` to its Bytes64 `string` representation.
*/
function encode32(bytes memory data) internal pure returns (string memory) {
/**
* Inspired by Brecht Devos (Brechtpd) implementation - MIT licence
* https://github.com/Brechtpd/base64/blob/e78d9fd951e7b0977ddca77d92dc85183770daf4/base64.sol
*/
if (data.length == 0) return "";

// Loads the table into memory
string memory table = _TABLE;

// Encoding takes 3 bytes chunks of binary data from `bytes` data parameter
// and split into 4 numbers of 6 bits.
// The final Base64 length should be `bytes` data length multiplied by 4/3 rounded up
// - `data.length + 2` -> Round up
// - `/ 3` -> Number of 3-bytes chunks
// - `4 *` -> 4 characters for each chunk
//string memory result = new string(4 * ((data.length + 2) / 3));
string memory result = new string(4 * ((data.length + 2) / 3) - 1);

/// @solidity memory-safe-assembly
assembly {
// Prepare the lookup table (skip the first "length" byte)
let tablePtr := add(table, 1)

// Prepare result pointer, jump over length
let resultPtr := add(result, 32)

// Run over the input, 3 bytes at a time
for {
let dataPtr := data
let endPtr := add(data, mload(data))
} lt(dataPtr, endPtr) {

} {
// Advance 3 bytes
dataPtr := add(dataPtr, 3)
let input := mload(dataPtr)

// To write each character, shift the 3 bytes (18 bits) chunk
// 4 times in blocks of 6 bits for each character (18, 12, 6, 0)
// and apply logical AND with 0x3F which is the number of
// the previous character in the ASCII table prior to the Base64 Table
// The result is then added to the table to get the character to write,
// and finally write it in the result pointer but with a left shift
// of 256 (1 byte) - 8 (1 ASCII char) = 248 bits

mstore8(
resultPtr,
mload(add(tablePtr, and(shr(18, input), 0x3F)))
)
resultPtr := add(resultPtr, 1) // Advance

mstore8(
resultPtr,
mload(add(tablePtr, and(shr(12, input), 0x3F)))
)
resultPtr := add(resultPtr, 1) // Advance

mstore8(
resultPtr,
mload(add(tablePtr, and(shr(6, input), 0x3F)))
)
resultPtr := add(resultPtr, 1) // Advance

mstore8(resultPtr, mload(add(tablePtr, and(input, 0x3F))))
resultPtr := add(resultPtr, 1) // Advance
}

/*
// When data `bytes` is not exactly 3 bytes long
// it is padded with `=` characters at the end
switch mod(mload(data), 3)
case 1 {
mstore8(sub(resultPtr, 1), 0x3d)
mstore8(sub(resultPtr, 2), 0x3d)
}
case 2 {
mstore8(sub(resultPtr, 1), 0x3d)
}
*/
}

return result;
}
}
46 changes: 43 additions & 3 deletions account-integrations/safe/src/lib/FCL_Webauthn.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.12;

import {Base64} from "openzeppelin-contracts/contracts/utils/Base64.sol";
import {Base64URL} from "./Base64URL.sol";
import {FCL_Elliptic_ZZ} from "./FCL_elliptic.sol";

library FCL_WebAuthn {
Expand All @@ -43,12 +43,13 @@ library FCL_WebAuthn {
revert InvalidAuthenticatorData();
}
// Verify that clientData commits to the expected client challenge
string memory challengeEncoded = Base64.encode(abi.encodePacked(clientChallenge));
string memory challengeEncoded = Base64URL.encode32(abi.encodePacked(clientChallenge));
bytes memory challengeExtracted = new bytes(
bytes(challengeEncoded).length
);

// TODO: wax#44 Extract webauthn challenge from clientData and compare to clientChallenge
// TODO: wax#68 Calldata decoding and stack limit
// Remove use of copyBytes function, and use commented inline assembly instead to extract the client challenge
// assembly {
// calldatacopy(
// add(challengeExtracted, 32),
Expand All @@ -65,6 +66,21 @@ library FCL_WebAuthn {
// if (keccak256(abi.encodePacked(bytes(challengeEncoded))) != moreData) {
// revert InvalidClientData();
// }

copyBytes(
clientData,
clientChallengeDataOffset,
challengeExtracted.length,
challengeExtracted,
0
);

if (
keccak256(abi.encodePacked(bytes(challengeEncoded))) !=
keccak256(abi.encodePacked(challengeExtracted))
) {
revert InvalidClientData();
}
} //avoid stack full

// Verify the signature over sha256(authenticatorData || sha256(clientData))
Expand All @@ -74,6 +90,30 @@ library FCL_WebAuthn {
return sha256(verifyData);
}

// TODO: wax#68 Calldata decoding and stack limit - remove this function once #68 is complete
/* The following function has been written by Alex Beregszaszi (@axic), use it under the terms of the MIT license */
function copyBytes(
bytes memory _from,
uint _fromOffset,
uint _length,
bytes memory _to,
uint _toOffset
) internal pure returns (bytes memory _copiedBytes) {
uint minLength = _length + _toOffset;
require(_to.length >= minLength); // Buffer too small. Should be a better way?
uint i = 32 + _fromOffset; // NOTE: the offset 32 is added to skip the `size` field of both bytes variables
uint j = 32 + _toOffset;
while (i < (32 + _fromOffset + _length)) {
assembly {
let tmp := mload(add(_from, i))
mstore(add(_to, j), tmp)
}
i += 32;
j += 32;
}
return _to;
}

/** @notice Modified from original Fresh Crypto Lib code to use memory instead of calldata */
function checkSignature(
bytes calldata authenticatorData,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ contract SafeWebAuthnPluginTest is TestHelper {
bytes memory authenticatorData,
bytes1 authenticatorDataFlagMask,
bytes memory clientData,
,
bytes32 clientChallenge,
uint256 clientChallengeDataOffset,

) = getWebAuthnSignatureValues();
Expand All @@ -63,6 +63,7 @@ contract SafeWebAuthnPluginTest is TestHelper {
authenticatorData,
authenticatorDataFlagMask,
clientData,
clientChallenge,
clientChallengeDataOffset,
invalidSignature,
publicKey
Expand Down
4 changes: 2 additions & 2 deletions account-integrations/safe/test/forge/WebAuthn.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ contract WebauthnTest is TestHelper {
bytes memory authenticatorData,
bytes1 authenticatorDataFlagMask,
bytes memory clientData,
bytes32 messageHash,
bytes32 clientChallenge,
uint256 clientChallengeDataOffset,
uint256[2] memory signature
) = getWebAuthnSignatureValues();
Expand All @@ -33,7 +33,7 @@ contract WebauthnTest is TestHelper {
authenticatorData,
authenticatorDataFlagMask,
clientData,
messageHash,
clientChallenge,
clientChallengeDataOffset,
signature,
publicKey
Expand Down
19 changes: 10 additions & 9 deletions account-integrations/safe/test/forge/utils/TestHelper.sol
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ abstract contract TestHelper is Test {
returns (uint256[2] memory publicKey)
{
publicKey = [
84983235508986227069118519878575107221347312419117152995971180204793547896817,
65342283463016373417889909198331793507107976344099657471582098851386861908802
114874632398302156264159990279427641021947882640101801130664833947273521181002,
32136952818958550240756825111900051564117520891182470183735244184006536587423
];
}

Expand All @@ -79,19 +79,19 @@ abstract contract TestHelper is Test {
bytes memory authenticatorData,
bytes1 authenticatorDataFlagMask,
bytes memory clientData,
bytes32 messageHash,
bytes32 clientChallenge,
uint256 clientChallengeDataOffset,
uint256[2] memory signature
)
{
authenticatorData = hex"1584482fdf7a4d0b7eb9d45cf835288cb59e55b8249fff356e33be88ecc546d11d00000000";
authenticatorData = hex"f8e4b678e1c62f7355266eaa4dc1148573440937063a46d848da1e25babbd20b010000004d";
authenticatorDataFlagMask = 0x01;
clientData = hex"7b2274797065223a22776562617574686e2e676574222c226368616c6c656e6765223a22efbfbd22efbfbd5f21efbfbd1b113e63efbfbdefbfbd6defbfbd4fefbfbdefbfbd11efbfbd11efbfbd40efbfbdefbfbdefbfbd64efbfbdefbfbd3cefbfbd58222c226f726967696e223a2268747470733a2f2f646576656c6f706d656e742e666f72756d64616f732e636f6d227d";
messageHash = keccak256("test");
clientData = hex"7b2274797065223a22776562617574686e2e676574222c226368616c6c656e6765223a224e546f2d3161424547526e78786a6d6b61544865687972444e5833697a6c7169316f776d4f643955474a30222c226f726967696e223a2268747470733a2f2f66726573682e6c65646765722e636f6d222c2263726f73734f726967696e223a66616c73657d";
clientChallenge = hex"353a3ed5a0441919f1c639a46931de872ac3357de2ce5aa2d68c2639df54189d";
clientChallengeDataOffset = 36;
signature = [
36788204816852931931532076736929768488646494203674172515272861180041446565109,
60595451626159535380360537025565143491223093262105891867977188941268073626113
45847212378479006099766816358861726414873720355505495069909394794949093093607,
55835259151215769394881684156457977412783812617123006733908193526332337539398
];
}

Expand All @@ -104,7 +104,7 @@ abstract contract TestHelper is Test {
bytes memory authenticatorData,
bytes1 authenticatorDataFlagMask,
bytes memory clientData,
,
bytes32 clientChallenge,
uint256 clientChallengeDataOffset,
uint256[2] memory signature
) = getWebAuthnSignatureValues();
Expand All @@ -114,6 +114,7 @@ abstract contract TestHelper is Test {
authenticatorData,
authenticatorDataFlagMask,
clientData,
clientChallenge,
clientChallengeDataOffset,
signature,
publicKey
Expand Down
Loading