-
Notifications
You must be signed in to change notification settings - Fork 295
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
Normalize salt, iv, uuid params of .toV3() before encrypting #95
Changes from all commits
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 |
---|---|---|
@@ -0,0 +1 @@ | ||
package-lock = false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
language: node_js | ||
node_js: | ||
- "6" | ||
- "8" | ||
- "10" | ||
addons: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,19 @@ const uuidv4 = require('uuid/v4') | |
// parameters for the toV3() method | ||
|
||
interface V3Params { | ||
kdf: string | ||
cipher: string | ||
salt: string | Buffer | ||
iv: string | Buffer | ||
uuid: string | Buffer | ||
dklen: number | ||
c: number | ||
n: number | ||
r: number | ||
p: number | ||
} | ||
|
||
interface V3ParamsStrict { | ||
kdf: string | ||
cipher: string | ||
salt: Buffer | ||
|
@@ -21,8 +34,43 @@ interface V3Params { | |
p: number | ||
} | ||
|
||
function mergeToV3ParamsWithDefaults(params?: Partial<V3Params>): V3Params { | ||
const v3Defaults: V3Params = { | ||
function validateHexString(paramName: string, str: string, length?: number) { | ||
if (str.toLowerCase().startsWith('0x')) { | ||
str = str.slice(2) | ||
} | ||
if (!str && !length) { | ||
return str | ||
} | ||
if ((length as number) % 2) { | ||
throw new Error(`Invalid length argument, must be an even number`) | ||
} | ||
if (typeof length === 'number' && str.length !== length) { | ||
throw new Error(`Invalid ${paramName}, string must be ${length} hex characters`) | ||
} | ||
if (!/^([0-9a-f]{2})+$/i.test(str)) { | ||
const howMany = typeof length === 'number' ? length : 'empty or a non-zero even number of' | ||
throw new Error(`Invalid ${paramName}, string must be ${howMany} hex characters`) | ||
} | ||
return str | ||
} | ||
|
||
function validateBuffer(paramName: string, buff: Buffer, length?: number) { | ||
if (!Buffer.isBuffer(buff)) { | ||
const howManyHex = | ||
typeof length === 'number' ? `${length * 2}` : 'empty or a non-zero even number of' | ||
const howManyBytes = typeof length === 'number' ? ` (${length} bytes)` : '' | ||
throw new Error( | ||
`Invalid ${paramName}, must be a string (${howManyHex} hex characters) or buffer${howManyBytes}`, | ||
) | ||
} | ||
if (typeof length === 'number' && buff.length !== length) { | ||
throw new Error(`Invalid ${paramName}, buffer must be ${length} bytes`) | ||
} | ||
return buff | ||
} | ||
|
||
function mergeToV3ParamsWithDefaults(params?: Partial<V3Params>): V3ParamsStrict { | ||
const v3Defaults: V3ParamsStrict = { | ||
cipher: 'aes-128-ctr', | ||
kdf: 'scrypt', | ||
salt: randomBytes(32), | ||
|
@@ -38,17 +86,30 @@ function mergeToV3ParamsWithDefaults(params?: Partial<V3Params>): V3Params { | |
if (!params) { | ||
return v3Defaults | ||
} | ||
|
||
if (typeof params.salt === 'string') { | ||
params.salt = Buffer.from(validateHexString('salt', params.salt), 'hex') | ||
} | ||
if (typeof params.iv === 'string') { | ||
params.iv = Buffer.from(validateHexString('iv', params.iv, 32), 'hex') | ||
} | ||
if (typeof params.uuid === 'string') { | ||
params.uuid = Buffer.from(validateHexString('uuid', params.uuid, 32), 'hex') | ||
} | ||
|
||
if (params.salt) { | ||
validateBuffer('salt', params.salt) | ||
} | ||
if (params.iv) { | ||
validateBuffer('iv', params.iv, 16) | ||
} | ||
if (params.uuid) { | ||
validateBuffer('uuid', params.uuid, 16) | ||
} | ||
|
||
return { | ||
cipher: params.cipher || 'aes-128-ctr', | ||
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 can be simplified as: 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. Done, but it needed to be: |
||
kdf: params.kdf || 'scrypt', | ||
salt: params.salt || randomBytes(32), | ||
iv: params.iv || randomBytes(16), | ||
uuid: params.uuid || randomBytes(16), | ||
dklen: params.dklen || 32, | ||
c: params.c || 262144, | ||
n: params.n || 262144, | ||
r: params.r || 8, | ||
p: params.p || 1, | ||
...v3Defaults, | ||
...(params as V3ParamsStrict), | ||
} | ||
} | ||
|
||
|
@@ -60,6 +121,14 @@ const enum KDFFunctions { | |
} | ||
|
||
interface ScryptKDFParams { | ||
dklen: number | ||
n: number | ||
p: number | ||
r: number | ||
salt: Buffer | ||
} | ||
|
||
interface ScryptKDFParamsOut { | ||
dklen: number | ||
n: number | ||
p: number | ||
|
@@ -68,27 +137,35 @@ interface ScryptKDFParams { | |
} | ||
|
||
interface PBKDFParams { | ||
c: number | ||
dklen: number | ||
prf: string | ||
salt: Buffer | ||
} | ||
|
||
interface PBKDFParamsOut { | ||
c: number | ||
dklen: number | ||
prf: string | ||
salt: string | ||
} | ||
|
||
type KDFParams = ScryptKDFParams | PBKDFParams | ||
type KDFParamsOut = ScryptKDFParamsOut | PBKDFParamsOut | ||
|
||
function kdfParamsForPBKDF(opts: V3Params): PBKDFParams { | ||
function kdfParamsForPBKDF(opts: V3ParamsStrict): PBKDFParams { | ||
return { | ||
dklen: opts.dklen, | ||
salt: opts.salt.toString('hex'), | ||
salt: opts.salt, | ||
c: opts.c, | ||
prf: 'hmac-sha256', | ||
} | ||
} | ||
|
||
function kdfParamsForScrypt(opts: V3Params): ScryptKDFParams { | ||
function kdfParamsForScrypt(opts: V3ParamsStrict): ScryptKDFParams { | ||
return { | ||
dklen: opts.dklen, | ||
salt: opts.salt.toString('hex'), | ||
salt: opts.salt, | ||
n: opts.n, | ||
r: opts.r, | ||
p: opts.p, | ||
|
@@ -130,7 +207,7 @@ interface V3Keystore { | |
} | ||
ciphertext: string | ||
kdf: string | ||
kdfparams: KDFParams | ||
kdfparams: KDFParamsOut | ||
mac: string | ||
} | ||
id: string | ||
|
@@ -361,6 +438,7 @@ export default class Wallet { | |
|
||
// public instance methods | ||
|
||
// tslint:disable-next-line | ||
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 is it necessary to ignore tslint errors here and in 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. Without disabling those lines for tslint I get the following when doing
I felt that refactoring that code from the TS rewrite was beyond the scope of this PR, especially since the README documents those |
||
public getPrivateKey(): Buffer { | ||
return this.privKey | ||
} | ||
|
@@ -369,6 +447,7 @@ export default class Wallet { | |
return ethUtil.bufferToHex(this.privKey) | ||
} | ||
|
||
// tslint:disable-next-line | ||
public getPublicKey(): Buffer { | ||
return this.pubKey | ||
} | ||
|
@@ -394,16 +473,16 @@ export default class Wallet { | |
throw new Error('This is a public key only wallet') | ||
} | ||
|
||
const v3Params: V3Params = mergeToV3ParamsWithDefaults(opts) | ||
const v3Params: V3ParamsStrict = mergeToV3ParamsWithDefaults(opts) | ||
|
||
let kdfParams: PBKDFParams | ScryptKDFParams | ||
let kdfParams: KDFParams | ||
let derivedKey: Buffer | ||
switch (v3Params.kdf) { | ||
case KDFFunctions.PBKDF: | ||
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 couldn't comment on the code below, but now 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. Nice catch, I've made the change. |
||
kdfParams = kdfParamsForPBKDF(v3Params) | ||
derivedKey = crypto.pbkdf2Sync( | ||
Buffer.from(password), | ||
v3Params.salt, | ||
kdfParams.salt, | ||
kdfParams.c, | ||
kdfParams.dklen, | ||
'sha256', | ||
|
@@ -414,7 +493,7 @@ export default class Wallet { | |
// FIXME: support progress reporting callback | ||
derivedKey = scryptsy( | ||
Buffer.from(password), | ||
v3Params.salt, | ||
kdfParams.salt, | ||
kdfParams.n, | ||
kdfParams.r, | ||
kdfParams.p, | ||
|
@@ -449,7 +528,10 @@ export default class Wallet { | |
cipherparams: { iv: v3Params.iv.toString('hex') }, | ||
cipher: v3Params.cipher, | ||
kdf: v3Params.kdf, | ||
kdfparams: kdfParams, | ||
kdfparams: { | ||
...kdfParams, | ||
salt: kdfParams.salt.toString('hex'), | ||
}, | ||
mac: mac.toString('hex'), | ||
}, | ||
} | ||
|
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.
I just noticed that in
ethereumjs-util
we pad the string to even length, so that if implicitly e.g. a0x01
byte was converted to0x1
, it would be covered:https://github.com/ethereumjs/ethereumjs-util/blob/3b1085059194b02354177d334f89cd82a5187883/src/bytes.ts#L76
Not sure whether that applies here.
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.
It's a possibility, for sure, though I wonder if it would end up masking legit user errors more than providing shorthand/convenience.