New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(agent): Check subnet canister ranges #580
Changes from 2 commits
d67164e
298b5b5
2c360c8
8adffe9
8212233
d452ce7
dc6229e
76ac41c
af999e4
4203c2e
d4d11dc
cd997df
5e98356
5755d35
6889f72
839a6ea
e7f9eb6
7cd23c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,8 +87,8 @@ export const request = async (options: { | |
const response = await agent.readState(canisterId, { | ||
paths: [encodedPaths[index]], | ||
}); | ||
const cert = new Certificate(response, agent); | ||
const verified = await cert.verify(); | ||
const cert = new Certificate(response.certificate, agent.fetchRootKey()); | ||
const verified = await cert.verify(canisterId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. quick check here - this changes the interface of also, is there anywhere else in the codebase that calls There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have the same concern. Not all certificates contain a path with canister_id. How do we verify the certificate if we only want to read non canister related paths, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point - I'll bump the version up. We could also consider rejigging the type of For the other point (other places calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For @chenyan-dfinity 's point: any call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Nope. TS type system is NOT sound. Never rely on its type checker.
Good point. Then we could use Also, can this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think the interface spec is actually wrong on this, as it says that any canister is an effective canister ID for
Since we're fixing a security flaw, I think we have to make a breaking change (callers that don't change the call would stay vulnerable). After discussing with Jason, since it's a breaking change already, I've changed the API to perform the verification immediately in the constructor - we couldn't find a use case where you'd want to delay the verification (do you maybe see one?). Also, I unified the error reporting to always use exceptions, instead of a mixture of exceptions and Booleans as was previously the case for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think the spec is fine. We just need to disable the check when the effective canister id is
Not sure it has to be a breaking change. Callers that didn't upgrade agent-js to the fixed version stays vulnerable. It doesn't have to be a breaking change to fix a vulnerability.
There might be a use case that the caller doesn't want to do a verification. If we verify in the constructor, it can be harder to skip verification? The problem is that bls verify is written in Wasm, some platforms don't support wasm execution, e.g. mobile platform, chrome plugins. In those environments, they either have to use a slower JS implementation or skip the verification completely. I know a grant awardee is writing a Chrome plugin to inspect all IC traffic, and cannot run verify in the plugin setting. Since it's a debugging tool, it's not too bad to disable the verification. |
||
if (!verified) { | ||
throw new Error( | ||
'There was a problem certifying the response data. Please verify your connection to the mainnet, or be sure to call fetchRootKey on your agent if you are developing locally', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,12 @@ | |
* an instance of ArrayBuffer). | ||
* @jest-environment node | ||
*/ | ||
import { fromHexString } from '@dfinity/candid'; | ||
import * as cbor from './cbor'; | ||
import * as Cert from './certificate'; | ||
import { fromHex, toHex } from './utils/buffer'; | ||
import { Principal } from '@dfinity/principal'; | ||
import { verify } from 'crypto'; | ||
|
||
function label(str: string): ArrayBuffer { | ||
return new TextEncoder().encode(str); | ||
|
@@ -15,6 +18,13 @@ function pruned(str: string): ArrayBuffer { | |
return fromHex(str); | ||
} | ||
|
||
// Root public key for the IC main net, encoded as hex | ||
const IC_ROOT_KEY = | ||
'308182301d060d2b0601040182dc7c0503010201060c2b0601040182dc7c05030201036100814' + | ||
'c0e6ec71fab583b08bd81373c255c3c371b2e84863c98a4f1e08b74235d14fb5d9c0cd546d968' + | ||
'5f913a0c0b2cc5341583bf4b4392e467db96d65b9bb4cb717112f8472e0d5a4d14505ffd7484' + | ||
'b01291091c5f87b98883463f98091a0baaae'; | ||
|
||
test('hash tree', async () => { | ||
const cborEncode = fromHex( | ||
'8301830183024161830183018302417882034568656c6c6f810083024179820345776f726c64' + | ||
|
@@ -124,3 +134,38 @@ test('lookup', () => { | |
expect(toText(Cert.lookup_path([fromText('d')], tree))).toEqual('morning'); | ||
expect(Cert.lookup_path([fromText('e')], tree)).toEqual(undefined); | ||
}); | ||
|
||
// The sample certificate for testing delegation is extracted from the response used in agent-rs tests, where they were taken | ||
// from an interaction with the IC mainnet. | ||
const SAMPLE_CERT: string = | ||
'd9d9f7a364747265658301830182045820250f5e26868d9c1ea7ab29cbe9c15bf1c47c0d7605e803e39e375a7fe09c6ebb830183024e726571756573745f7374617475738301820458204b268227774ec77ff2b37ecb12157329d54cf376694bdd59ded7803efd82386f83025820edad510eaaa08ed2acd4781324e6446269da6753ec17760f206bbe81c465ff528301830183024b72656a6563745f636f64658203410383024e72656a6563745f6d6573736167658203584443616e69737465722069766733372d71696161612d61616161622d61616167612d63616920686173206e6f20757064617465206d6574686f64202772656769737465722783024673746174757382034872656a65637465648204582097232f31f6ab7ca4fe53eb6568fc3e02bc22fe94ab31d010e5fb3c642301f1608301820458203a48d1fc213d49307103104f7d72c2b5930edba8787b90631f343b3aa68a5f0a83024474696d65820349e2dc939091c696eb16697369676e6174757265583089a2be21b5fa8ac9fab1527e041327ce899d7da971436a1f2165393947b4d942365bfe5488710e61a619ba48388a21b16a64656c65676174696f6ea2697375626e65745f6964581dd77b2a2f7199b9a8aec93fe6fb588661358cf12223e9a3af7b4ebac4026b6365727469666963617465590231d9d9f7a26474726565830182045820ae023f28c3b9d966c8fb09f9ed755c828aadb5152e00aaf700b18c9c067294b483018302467375626e6574830182045820e83bb025f6574c8f31233dc0fe289ff546dfa1e49bd6116dd6e8896d90a4946e830182045820e782619092d69d5bebf0924138bd4116b0156b5a95e25c358ea8cf7e7161a661830183018204582062513fa926c9a9ef803ac284d620f303189588e1d3904349ab63b6470856fc4883018204582060e9a344ced2c9c4a96a0197fd585f2d259dbd193e4eada56239cac26087f9c58302581dd77b2a2f7199b9a8aec93fe6fb588661358cf12223e9a3af7b4ebac402830183024f63616e69737465725f72616e6765738203581bd9d9f781824a000000000020000001014a00000000002fffff010183024a7075626c69635f6b657982035885308182301d060d2b0601040182dc7c0503010201060c2b0601040182dc7c050302010361009933e1f89e8a3c4d7fdcccdbd518089e2bd4d8180a261f18d9c247a52768ebce98dc7328a39814a8f911086a1dd50cbe015e2a53b7bf78b55288893daa15c346640e8831d72a12bdedd979d28470c34823b8d1c3f4795d9c3984a247132e94fe82045820996f17bb926be3315745dea7282005a793b58e76afeb5d43d1a28ce29d2d158583024474696d6582034995b8aac0e4eda2ea16697369676e61747572655830ace9fcdd9bc977e05d6328f889dc4e7c99114c737a494653cb27a1f55c06f4555e0f160980af5ead098acc195010b2f7'; | ||
|
||
test('delegation works for canisters within the subnet range', async () => { | ||
const canisterId = Principal.fromText('ivg37-qiaaa-aaaab-aaaga-cai'); | ||
const cert = new Cert.Certificate(fromHex(SAMPLE_CERT), Promise.resolve(fromHex(IC_ROOT_KEY))); | ||
const result = await cert.verify(canisterId); | ||
expect(result).toEqual(true); | ||
}); | ||
|
||
test('delegation check fails for canisters outside of the subnet range', async () => { | ||
// Use a different principal than the happy path, which isn't in the delegation ranges. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I would have to check the allowed ranges myself here. Would it help to provide the allowed range(s) as a comment here? also, since it would be cheap: would it make sense to add tests for the boundary cases at the (closed) ends of a range? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TBH I don't even know what the subnet ranges in this certificate are - I just copied these over from the Rust tests. But even if I did know what they were, I don't know if our textual encoding preserves the ordering? Boundary cases totally make sense, I'll have a look at those. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can actually decode this (it's cbor, https://cbor.me comes in handy). So the first level of decoding reveals
The included delegation cert then specifies for the subnet (in addition to it's public key) the canister_ranges
Which are the following principals: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @frederikrothenberger ! I now realized that the interface spec also provides handy tools for decoding the text representation (which indeed doesn't preserve the lexicographic ordering, due to the use of crc-32). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the checks now, switched to a hex representation of the principals as that makes the ranges clearly visible There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great, thanks for addressing this @oggy-dfin ! |
||
const canisterId = Principal.fromText('ryjl3-tyaaa-aaaaa-aaaba-cai'); | ||
const cert = new Cert.Certificate(fromHex(SAMPLE_CERT), Promise.resolve(fromHex(IC_ROOT_KEY))); | ||
// This is a bit crufty; verify returns a boolean, but can also indicate verification errors | ||
// by throwing exceptions | ||
try { | ||
const result = await cert.verify(canisterId); | ||
expect(result).toEqual(false); | ||
} catch (e) {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question about this test - it sounds like you want to test that an exception gets thrown. shouldn't you put the on the other hand, if all you want to check is that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem here is that the interface of IMO we should clean it up. I think an event better alternative would be to have the constructor take the |
||
}); | ||
|
||
// The only situation in which one can read state of the IC management canister | ||
// is when the user calls provisional_create_canister_with_cycles. In this case, | ||
// we shouldn't check the delegations. | ||
test('delegation check succeeds for the management canister', async () => { | ||
const cert = new Cert.Certificate(fromHex(SAMPLE_CERT), Promise.resolve(fromHex(IC_ROOT_KEY))); | ||
// This is a bit crufty; verify returns a boolean, but can also indicate verification errors | ||
// by throwing exceptions | ||
const result = await cert.verify(Principal.managementCanister()); | ||
expect(result).toEqual(true); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,9 @@ | ||
import { Agent, getDefaultAgent, ReadStateResponse } from './agent'; | ||
import * as cbor from './cbor'; | ||
import { AgentError } from './errors'; | ||
import { hash } from './request_id'; | ||
import { blsVerify } from './utils/bls'; | ||
import { concat, fromHex, toHex } from './utils/buffer'; | ||
import { Principal } from '@dfinity/principal'; | ||
|
||
/** | ||
* A certificate needs to be verified (using {@link Certificate.prototype.verify}) | ||
|
@@ -102,20 +102,19 @@ function isBufferEqual(a: ArrayBuffer, b: ArrayBuffer): boolean { | |
export class Certificate { | ||
private readonly cert: Cert; | ||
private verified = false; | ||
private _rootKey: ArrayBuffer | null = null; | ||
|
||
constructor(response: ReadStateResponse, private _agent: Agent = getDefaultAgent()) { | ||
this.cert = cbor.decode(new Uint8Array(response.certificate)); | ||
constructor(certificate: ArrayBuffer, private _rootKey: Promise<ArrayBuffer>) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we change the interface to stick the |
||
this.cert = cbor.decode(new Uint8Array(certificate)); | ||
} | ||
|
||
public lookup(path: Array<ArrayBuffer | string>): ArrayBuffer | undefined { | ||
this.checkState(); | ||
return lookup_path(path, this.cert.tree); | ||
} | ||
|
||
public async verify(): Promise<boolean> { | ||
public async verify(canisterId: Principal): Promise<boolean> { | ||
const rootHash = await reconstruct(this.cert.tree); | ||
const derKey = await this._checkDelegation(this.cert.delegation); | ||
const derKey = await this._checkDelegationAndGetKey(canisterId, this.cert.delegation); | ||
const sig = this.cert.signature; | ||
const key = extractDER(derKey); | ||
const msg = concat(domain_sep('ic-state-root'), rootHash); | ||
|
@@ -130,28 +129,41 @@ export class Certificate { | |
} | ||
} | ||
|
||
private async _checkDelegation(d?: Delegation): Promise<ArrayBuffer> { | ||
private async _checkDelegationAndGetKey( | ||
canisterId: Principal, | ||
d?: Delegation, | ||
): Promise<ArrayBuffer> { | ||
if (!d) { | ||
if (!this._rootKey) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why removing this logic? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for catching this - my fix had made things worse! However, I do think that this is the wrong place for this logic - there's no real need for I changed the code now so that the callers use the root key if available. We can probably extract a convenience method on |
||
if (this._agent.rootKey) { | ||
this._rootKey = this._agent.rootKey; | ||
return this._rootKey; | ||
} | ||
|
||
throw new Error(`Agent does not have a rootKey. Do you need to call 'fetchRootKey'?`); | ||
} | ||
return this._rootKey; | ||
} | ||
const cert: Certificate = new Certificate(d as any, this._agent); | ||
if (!(await cert.verify())) { | ||
const cert: Certificate = new Certificate(d.certificate, this._rootKey); | ||
if (!(await cert.verify(canisterId))) { | ||
throw new Error('fail to verify delegation certificate'); | ||
} | ||
|
||
const lookup = cert.lookup(['subnet', d.subnet_id, 'public_key']); | ||
if (!lookup) { | ||
if (canisterId.compareTo(Principal.managementCanister()) != 'eq') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. javascript is weird, so this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this post is correct, there'd be no coercion in TS (it'd be a compile-time error instead) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's not rely on TS for correctness. If one side the operand is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, changed to |
||
const range_lookup = cert.lookup(['subnet', d.subnet_id, 'canister_ranges']); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a nit, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for catching this - I changed all the |
||
if (!range_lookup) { | ||
throw new Error(`Could not find canister ranges for subnet 0x${toHex(d.subnet_id)}`); | ||
} | ||
const ranges_arr: Array<[Uint8Array, Uint8Array]> = cbor.decode(range_lookup); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same nit here re: camelCase |
||
const ranges: Array<[Principal, Principal]> = ranges_arr.map(v => [ | ||
Principal.fromUint8Array(v[0]), | ||
Principal.fromUint8Array(v[1]), | ||
]); | ||
|
||
const canister_in_range = ranges.some(r => r[0].ltEq(canisterId) && r[1].gtEq(canisterId)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same nit here re: camelCase |
||
if (!canister_in_range) { | ||
throw new Error( | ||
`Canister ${canisterId} not in range of delegations for subnet 0x${toHex(d.subnet_id)}`, | ||
); | ||
} | ||
} | ||
const public_key_lookup = cert.lookup(['subnet', d.subnet_id, 'public_key']); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same nit here re: camelCase |
||
if (!public_key_lookup) { | ||
throw new Error(`Could not find subnet key for subnet 0x${toHex(d.subnet_id)}`); | ||
} | ||
return lookup; | ||
return public_key_lookup; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,8 +29,8 @@ export async function pollForResponse( | |
): Promise<ArrayBuffer> { | ||
const path = [new TextEncoder().encode('request_status'), requestId]; | ||
const state = await agent.readState(canisterId, { paths: [path] }); | ||
const cert = new Certificate(state, agent); | ||
const verified = await cert.verify(); | ||
const cert = new Certificate(state.certificate, agent.fetchRootKey()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs an const rootKey = await agent.fetchRootKey();
const cert = new Certificate(state.certificate, rootKey);
const verified = await cert.verify(canisterId); |
||
const verified = await cert.verify(canisterId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need a function to compute the effective canister id, especially when the client is calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have sufficient information at this point in the code to compute this (there's no request); per the comment, the function already expects an effective canister ID. I don't see any special computation of the effective ID, but I may have missed something. So I think you're right and we should add it, but I don't think this PR is breaking anything beyond what's broken already. In particular, to read the status of calls to the management canister other than |
||
if (!verified) { | ||
throw new Error('Fail to verify certificate'); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,4 +24,21 @@ describe('Principal', () => { | |
it('errors out on parsing invalid characters', () => { | ||
expect(() => Principal.fromText('Hello world!')).toThrow(); | ||
}); | ||
|
||
it('compares principals correctly', () => { | ||
const anonymous = Principal.anonymous(); | ||
const principal1 = Principal.fromText('ryjl3-tyaaa-aaaaa-aaaba-cai'); | ||
const principal2 = Principal.fromText('ivg37-qiaaa-aaaab-aaaga-cai'); | ||
for (let p of [anonymous, principal1, principal2]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lint error here: the tests are failing because this is a |
||
expect(p.compareTo(p)).toBe('eq'); | ||
expect(p.ltEq(p)).toBe(true); | ||
expect(p.gtEq(p)).toBe(true); | ||
} | ||
expect(principal1.compareTo(principal2)).toBe('lt'); | ||
expect(principal1.compareTo(anonymous)).toBe('lt'); | ||
expect(principal2.compareTo(principal1)).toBe('gt'); | ||
expect(principal2.compareTo(anonymous)).toBe('lt'); | ||
expect(anonymous.compareTo(principal1)).toBe('gt'); | ||
expect(anonymous.compareTo(principal2)).toBe('gt'); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agent.fetchRootKey()
is asynchronous, so this would need anawait
- otherwise, the value that gets passed to the Certificate constructor is a Promise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
Certiificate
constructor takes aPromise
, it's later awaited on inCertificate.verify
, where it's used - I didn't want to block before the point where it was needed (not that it makes much of a difference in practice, as the user pretty much has to callverify
to do anything with the certificate).