Skip to content

Commit

Permalink
fix(address-hooks): version robustness and hardening (#10683)
Browse files Browse the repository at this point in the history 
closes: #10681

## Description

Be more robust for Address Hook version compatibility: return an error if the magic bytes match but the version number is unsupported.  If the magic bytes don't match, it is not an address hook, so pass through the specimen as the `baseAddress` with empty `hookData`.

Also, `harden` the exports and returned objects if running under HardenedJS.

### Security Considerations

Stronger Address Hook classification helps prevent accidental collisions.  Hardened return objects and exported functions have fewer mutability concerns.

### Scaling Considerations

n/a

### Documentation Considerations

n/a

### Testing Considerations

Some unhappy-path testing has been added to verify at least these issues have been addressed.

### Upgrade Considerations

n/a
  • Loading branch information
@mergify
mergify[bot] authored Dec 13, 2024
2 parents 4feddb0 + 80fee60 commit ddff762
Show file tree
Hide file tree
Showing 4 changed files with 177 additions and 48 deletions.
20 changes: 14 additions & 6 deletions golang/cosmos/types/address_hooks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ const (
BaseAddressLengthBytes = 2
)

// AddressHookMagic is a magic byte prefix that identifies a hooked address.
// AddressHookBytePrefix is a magic prefix that identifies a hooked address.
// Chosen to make bech32 address hooks that look like "agoric10rch..."
var AddressHookMagic = []byte{0x78, 0xf1, 0x70 | AddressHookVersion}
var AddressHookBytePrefix = []byte{0x78, 0xf1, 0x70 /* | AddressHookVersion */}

func init() {
if AddressHookVersion&0x0f != AddressHookVersion {
Expand All @@ -51,12 +51,19 @@ func SplitHookedAddress(addr string) (string, []byte, error) {
return "", []byte{}, err
}

bz := bytes.TrimPrefix(payload, AddressHookMagic)
if len(bz) == len(payload) {
lastPrefixHighNibble := AddressHookBytePrefix[len(AddressHookBytePrefix)-1]
bz := bytes.TrimPrefix(payload, AddressHookBytePrefix[:len(AddressHookBytePrefix)-1])
if len(bz) == len(payload) || len(bz) == 0 || bz[0]&0xf0 != lastPrefixHighNibble {
// Return an unhooked address.
return addr, []byte{}, nil
}

version := bz[0] & 0x0f
bz = bz[1:]
if version != AddressHookVersion {
return "", []byte{}, fmt.Errorf("unsupported address hook version %d", version)
}

if len(bz) < BaseAddressLengthBytes {
return "", []byte{}, fmt.Errorf("hooked address must have at least %d bytes", BaseAddressLengthBytes)
}
Expand Down Expand Up @@ -97,8 +104,9 @@ func JoinHookedAddress(baseAddr string, hookData []byte) (string, error) {
return "", fmt.Errorf("base address length 0x%x is longer than the maximum 0x%x", b, maxB)
}

payload := make([]byte, 0, len(AddressHookMagic)+b+len(hookData)+BaseAddressLengthBytes)
payload = append(payload, AddressHookMagic...)
payload := make([]byte, 0, len(AddressHookBytePrefix)+b+len(hookData)+BaseAddressLengthBytes)
payload = append(payload, AddressHookBytePrefix...)
payload[len(payload)-1] |= byte(AddressHookVersion)
payload = append(payload, bz...)
payload = append(payload, hookData...)
baLen := make([]byte, BaseAddressLengthBytes)
Expand Down
47 changes: 47 additions & 0 deletions golang/cosmos/types/address_hooks_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,53 @@ import (
"github.com/Agoric/agoric-sdk/golang/cosmos/types"
)

func TestSplitHookedAddress(t *testing.T) {
cases := []struct {
name string
hook string
baseAddr string
hookData []byte
err string
}{
{"empty", "", "", []byte{}, "decoding bech32 failed: invalid bech32 string length 0"},
{"no hook", "agoric1qqp0e5ys", "agoric1qqp0e5ys", []byte{}, ""},
{"Fast USDC", "agoric10rchp4vc53apxn32q42c3zryml8xq3xshyzuhjk6405wtxy7tl3d7e0f8az423padaek6me38qekget2vdhx66mtvy6kg7nrw5uhsaekd4uhwufswqex6dtsv44hxv3cd4jkuqpqvduyhf",
"agoric16kv2g7snfc4q24vg3pjdlnnqgngtjpwtetd2h689nz09lcklvh5s8u37ek",
[]byte("?EUD=osmo183dejcnmkka5dzcu9xw6mywq0p2m5peks28men"),
""},
{"version 0",
"agoric10rchqqqpqgpsgpgxquyqjzstpsxsurcszyfpxpqrqgqsq9qx0p9wp",
"agoric1qqqsyqcyq5rqwzqfpg9scrgwpugpzysn3tn9p0",
[]byte{4, 3, 2, 1},
""},
{"version 1 reject",
"agoric10rchzqqpqgpsgpgxquyqjzstpsxsurcszyfpxpqrqgqsq9q04n2fg",
"",
[]byte{},
"unsupported address hook version 1"},
{"version 15 reject",
"agoric10rch7qqpqgpsgpgxquyqjzstpsxsurcszyfpxpqrqgqsq9q25ez2d",
"",
[]byte{},
"unsupported address hook version 15"},
}

for _, tc := range cases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
baseAddr, hookData, err := types.SplitHookedAddress(tc.hook)
if len(tc.err) > 0 {
require.Error(t, err)
require.Equal(t, tc.err, err.Error())
} else {
require.NoError(t, err)
require.Equal(t, tc.baseAddr, baseAddr)
require.Equal(t, string(tc.hookData), string(hookData))
}
})
}
}

func TestExtractBaseAddress(t *testing.T) {
bases := []struct {
name string
Expand Down
102 changes: 60 additions & 42 deletions packages/cosmic-proto/src/address-hooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,18 @@
* const decoded = decodeAddressHook(addressHook);
* // {
* // baseAddress: 'agoric1qqp0e5ys',
* // query: [Object: null prototype] { foo: [ 'bar', 'baz' ], key: 'value' }
* // query: { foo: [ 'bar', 'baz' ], key: 'value' }
* // }
*/

/* eslint-disable no-bitwise */
import { bech32 } from 'bech32';
import queryString from 'query-string';

/* global globalThis */
/** @type {<T>(x: T) => T} */
const harden = globalThis.harden || Object.freeze;

// ADDRESS_HOOK_VERSION is the version of the address hook format used in this
// module.
const ADDRESS_HOOK_VERSION = 0;
Expand All @@ -39,13 +43,14 @@ if ((ADDRESS_HOOK_VERSION & 0x0f) !== ADDRESS_HOOK_VERSION) {
throw Error(`ADDRESS_HOOK_VERSION ${ADDRESS_HOOK_VERSION} exceeds 0x0f`);
}

// AddressHookMagic is a magic byte prefix that identifies a hooked address.
// ADDRESS_HOOK_BYTE_PREFIX is a magic prefix that identifies a hooked address.
// Chosen to make bech32 address hooks that look like "agoric10rch..."
const ADDRESS_HOOK_MAGIC = new Uint8Array([
const ADDRESS_HOOK_BYTE_PREFIX = [
0x78,
0xf1,
0x70 | ADDRESS_HOOK_VERSION,
]);
0x70, // | ADDRESS_HOOK_VERSION
];
harden(ADDRESS_HOOK_BYTE_PREFIX);

/**
* The default maximum number of characters in a bech32-encoded hooked address.
Expand All @@ -63,6 +68,9 @@ export const DEFAULT_HOOKED_ADDRESS_CHAR_LIMIT = 1024;
* { key: ['value1', null, 'value3'] } // '?key=value1&key&key=value3'
*/

/**
* How many bytes are used to store the length of the base address.
*/
export const BASE_ADDRESS_LENGTH_BYTES = 2;

/**
Expand All @@ -78,8 +86,9 @@ export const decodeBech32 = (
const rawBytes = bech32.fromWords(words);

const bytes = new Uint8Array(rawBytes);
return { prefix, bytes };
return harden({ prefix, bytes });
};
harden(decodeBech32);

/**
* @param {string} humanReadablePart
Expand All @@ -95,6 +104,7 @@ export const encodeBech32 = (
const words = bech32.toWords(bytes);
return bech32.encode(humanReadablePart, words, charLimit);
};
harden(encodeBech32);

/**
* Join raw base address bytes and hook data into a bech32-encoded hooked
Expand Down Expand Up @@ -140,13 +150,14 @@ export const joinHookedAddress = (
throw RangeError(`Hook data length ${hd} is not a non-negative integer`);
}

const magicLength = ADDRESS_HOOK_MAGIC.length;
const prefixLength = ADDRESS_HOOK_BYTE_PREFIX.length;
const hookBuf = new Uint8Array(
magicLength + b + hd + BASE_ADDRESS_LENGTH_BYTES,
prefixLength + b + hd + BASE_ADDRESS_LENGTH_BYTES,
);
hookBuf.set(ADDRESS_HOOK_MAGIC, 0);
hookBuf.set(bytes, magicLength);
hookBuf.set(hookData, magicLength + b);
hookBuf.set(ADDRESS_HOOK_BYTE_PREFIX, 0);
hookBuf[prefixLength - 1] |= ADDRESS_HOOK_VERSION;
hookBuf.set(bytes, prefixLength);
hookBuf.set(hookData, prefixLength + b);

// Append the address length bytes, since we've already ensured these do not
// exceed maxBaseAddressLength above. These are big-endian because the length
Expand All @@ -161,6 +172,7 @@ export const joinHookedAddress = (

return encodeBech32(prefix, hookBuf, charLimit);
};
harden(joinHookedAddress);

/**
* @param {string} baseAddress
Expand All @@ -174,6 +186,7 @@ export const encodeAddressHook = (baseAddress, query, charLimit) => {
const hookData = te.encode(`?${queryStr}`);
return joinHookedAddress(baseAddress, hookData, charLimit);
};
harden(encodeAddressHook);

/**
* @param {string} addressHook
Expand All @@ -187,64 +200,69 @@ export const decodeAddressHook = (addressHook, charLimit) => {
throw Error(`Hook data does not start with '?': ${hookStr}`);
}

/** @type {HookQuery} */
const query = queryString.parse(hookStr);
return { baseAddress, query };
const parsedQuery = queryString.parse(hookStr);

/**
* @type {HookQuery}
*/
const query = harden({ ...parsedQuery });
return harden({ baseAddress, query });
};
harden(decodeAddressHook);

/**
* @param {string} specimen
* @param {number} [charLimit]
* @returns {string | { baseAddress: string; hookData: Uint8Array }}
* @returns {{ baseAddress: string; hookData: Uint8Array }}
*/
export const splitHookedAddressUnsafe = (
export const splitHookedAddress = (
specimen,
charLimit = DEFAULT_HOOKED_ADDRESS_CHAR_LIMIT,
) => {
const { prefix, bytes } = decodeBech32(specimen, charLimit);

const magicLength = ADDRESS_HOOK_MAGIC.length;
for (let i = 0; i < magicLength; i += 1) {
if (bytes[i] !== ADDRESS_HOOK_MAGIC[i]) {
return { baseAddress: specimen, hookData: new Uint8Array() };
const prefixLength = ADDRESS_HOOK_BYTE_PREFIX.length;
let version = 0xff;
for (let i = 0; i < prefixLength; i += 1) {
let maybeMagicByte = bytes[i];
if (i === prefixLength - 1) {
// Final byte has a low version nibble and a high magic nibble.
version = maybeMagicByte & 0x0f;
maybeMagicByte &= 0xf0;
}
if (maybeMagicByte !== ADDRESS_HOOK_BYTE_PREFIX[i]) {
return harden({ baseAddress: specimen, hookData: new Uint8Array() });
}
}

if (version !== ADDRESS_HOOK_VERSION) {
throw TypeError(`Unsupported address hook version ${version}`);
}

let len = 0;
for (let i = BASE_ADDRESS_LENGTH_BYTES - 1; i >= 0; i -= 1) {
const byte = bytes.at(-i - 1);
if (byte === undefined) {
return `Cannot get base address length from byte ${-i - 1} of ${bytes.length}`;
throw TypeError(
`Cannot get base address length from byte ${-i - 1} of ${bytes.length}`,
);
}
len <<= 8;
len |= byte;
}

const b = len;
if (b > bytes.length - BASE_ADDRESS_LENGTH_BYTES - magicLength) {
return `Base address length 0x${b.toString(16)} is longer than specimen length ${bytes.length - BASE_ADDRESS_LENGTH_BYTES - magicLength}`;
if (b > bytes.length - BASE_ADDRESS_LENGTH_BYTES - prefixLength) {
throw TypeError(
`Base address length 0x${b.toString(16)} is longer than specimen length ${bytes.length - BASE_ADDRESS_LENGTH_BYTES - prefixLength}`,
);
}

const baseAddressBuf = bytes.subarray(magicLength, magicLength + b);
const baseAddressBuf = bytes.subarray(prefixLength, prefixLength + b);
const baseAddress = encodeBech32(prefix, baseAddressBuf, charLimit);

const hookData = bytes.subarray(magicLength + b, -BASE_ADDRESS_LENGTH_BYTES);
const hookData = bytes.subarray(prefixLength + b, -BASE_ADDRESS_LENGTH_BYTES);

return { baseAddress, hookData };
};

/**
* @param {string} specimen
* @param {number} [charLimit]
* @returns {{
* baseAddress: string;
* hookData: Uint8Array;
* }}
*/
export const splitHookedAddress = (specimen, charLimit) => {
const result = splitHookedAddressUnsafe(specimen, charLimit);
if (typeof result === 'object') {
return result;
}
throw Error(result);
return harden({ baseAddress, hookData });
};
harden(splitHookedAddress);
56 changes: 56 additions & 0 deletions packages/cosmic-proto/test/address-hooks.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,62 @@ test.before(async t => {
t.context = await makeTestContext();
});

/**
* @type {import('ava').Macro<
* [addressHook: string, baseAddress: string, hookDataStr: string, error?: any],
* { addressHooks: import('../src/address-hooks.js') }
* >}
*/
const splitMacro = test.macro({
title(providedTitle = '', addressHook, _baseAddress, _hookDataStr, _error) {
return `${providedTitle} split ${addressHook}`;
},
exec(t, addressHook, baseAddress, hookDataStr, error) {
const { splitHookedAddress } = t.context.addressHooks;
if (error) {
t.throws(() => splitHookedAddress(addressHook), error);
return;
}
const { baseAddress: ba, hookData: hd } = splitHookedAddress(addressHook);
t.is(ba, baseAddress);
const hookData = new TextEncoder().encode(hookDataStr);
t.deepEqual(hd, hookData);
},
});

test('empty', splitMacro, '', '', '', { message: ' too short' });
test('no hook', splitMacro, 'agoric1qqp0e5ys', 'agoric1qqp0e5ys', '', '');
test(
'Fast USDC',
splitMacro,
'agoric10rchp4vc53apxn32q42c3zryml8xq3xshyzuhjk6405wtxy7tl3d7e0f8az423padaek6me38qekget2vdhx66mtvy6kg7nrw5uhsaekd4uhwufswqex6dtsv44hxv3cd4jkuqpqvduyhf',
'agoric16kv2g7snfc4q24vg3pjdlnnqgngtjpwtetd2h689nz09lcklvh5s8u37ek',
'?EUD=osmo183dejcnmkka5dzcu9xw6mywq0p2m5peks28men',
);
test(
'version 0',
splitMacro,
'agoric10rchqqqpqgpsgpgxquyqjzstpsxsurcszyfpxpqrqgqsq9qx0p9wp',
'agoric1qqqsyqcyq5rqwzqfpg9scrgwpugpzysn3tn9p0',
'\x04\x03\x02\x01',
);
test(
'version 1 reject',
splitMacro,
'agoric10rchzqqpqgpsgpgxquyqjzstpsxsurcszyfpxpqrqgqsq9q04n2fg',
'',
'',
{ message: 'Unsupported address hook version 1' },
);
test(
'version 15 reject',
splitMacro,
'agoric10rch7qqpqgpsgpgxquyqjzstpsxsurcszyfpxpqrqgqsq9q25ez2d',
'',
'',
{ message: 'Unsupported address hook version 15' },
);

/**
* @type {import('ava').Macro<
* [string, ArrayLike<number> | undefined, ArrayLike<number>, string],
Expand Down

0 comments on commit ddff762

Please sign in to comment.