Keyring JSON with kdf #637
Conversation
|
|
||
| export default function scryptFromU8a (data: Uint8Array): Result { | ||
| const salt = data.subarray(0, 32); | ||
| const N = u8aToBn(data.subarray(32 + 0, 32 + 4), { isLe: true }).toNumber(); |
There was a problem hiding this comment.
I wonder if it would make sense to add some sanity checks here to avoid DoSing by some ridiculously-high numbers.
Probably is not an issue in client-side usage, though, since you can always close the tab.
But if we ever use this code serverside, maybe it would make sense to either explicitly timeout the derivation function or put some reasonable limits (like all defaults + 2 maybe?) on the values here?
There was a problem hiding this comment.
Yes, this is not great an opens up a hole. For now, I'm going to fix it to the defaults until there is a proper plan of attack - need to really look around on best-practices for this. User inputs are always an issue...
// FIXME At this moment we assume these to be fixed params, this is not a great idea since we lose flexibility
// and updates for greater security. However we need some protection against carefully-crafted params that can
// eat up CPU since these are user inputs. So we need to get very clever here, but atm we only allow the defaults
// and if no match, bail out
assert(N === DEFAULT_PARAMS.N && p === DEFAULT_PARAMS.p && r === DEFAULT_PARAMS.r, 'Invalid injected scrypt params found');
Easting the elephant in chunks here since this one has a bit of rollout gymnastics and would rather get it in solid, sooner and slightly less flexible than delayed and flexible in all ways possible.
EDIT: Logged here #638
| @@ -17,8 +17,10 @@ export * from './keccak'; | |||
| export * from './key'; | |||
| export * from './mnemonic'; | |||
| export * from './nacl'; | |||
| export * from './pbkdf2'; | |||
There was a problem hiding this comment.
Do you still want to keep it, even despite PBKDF2 is not being used anymore?
There was a problem hiding this comment.
Yes, since it is actually used elsewhere it should have been exposed already and it wasn't.
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Still TODO in follow-up PRs in the 3 series -