kryptein cryptography package for Node.js

Question

I have written a cryptography NPM package called kruptein. The code is also hosted on GitHub.

Having used multiple "best practices" with regards to key management, algorithm selection, key size, iv size etc. I could use someone with experience in this area to help validate the implementation.

I could use some help determining if I have flaws in any of the following:

1. Key sizes: _matrix(algo) - Are the appropriate key, iv & authentication tag sizes being identified and used correctly based on the algorithm selection? i.e. AES-128-CBC or AES-256-XTS

    _matrix(algo) {
    let obj = {
    at_size: 16,
    iv_size: 16,
    key_size: 32
    };
    if (algo.match(/ccm|ocb|gcm/i))
    obj.iv_size = 12;
    if (algo.match(/aes/) && algo.match(/ecb/))
    obj.iv_size = 0;
    if (algo.match(/aes/) && algo.match(/128/))
    obj.key_size = 16;
    if (algo.match(/aes/) && algo.match(/192/))
    obj.key_size = 24;
    if (algo.match(/aes/) && algo.match(/xts/))
    obj.key_size = 32;
    if (algo.match(/aes/) && algo.match(/xts/) && algo.match(/256/))
    obj.key_size = 64;
    return obj;
    }
   }
   

2. Key derivation: _derive_key(secret) - When deriving keyed material from a provided secret are enough iterations used? Is there a problem with the salt generation used for the resulting keyed material? What about the resulting key? For both the IV and key I am concerned about possible weakening of the material when converting to a buffer and slicing to the appropriate sizes.

    _derive_key(secret) {
    let key, hash, salt, result, derived_key;
    try {
    hash = this.crypto.createHash(this.hashing);
    hash.update(secret);
    salt = hash.digest();
    } catch(err) {
    throw err;
    }
    salt = (Buffer.isBuffer(salt)) ?
    salt.slice(0, 16) : salt.substr(0, 16);
    key = this.crypto.pbkdf2Sync(secret, salt, 10000, 64, this.hashing);
    return Buffer.from(key.toString(this.encodeas)).slice(0, this.key_size);
    }
   

3. IV generation: _iv(iv_size) - When generating a new IV is there a problem with slicing the results. Most of the node.js crypto API functions use buffers and the sizes must match but I can't help but think it is weakening the IV.

    _iv(iv_size) {
    let iv = this.crypto.randomBytes(iv_size);
    return Buffer.from(iv.toString(this.encodeas),
    this.encodeas).slice(0, iv_size);
    }
   

• Jörg W Mittag: I hate to break it to you, but that is not how crypto-review works. Basically, you have to already be a well-known, respected cryptographer to begin with. Then, you publish your work in a reputable, peer-reviewed cryptography journal and/or submit your work to a competition. Step #3: wait 10 years while the crypto community at large reviews your work.
• jas-: The question posed isn’t about algorithm design, nor is the question without merit. Please read again, as the question is specifically about an engineered solution soliciting review of implementation according to cited best practices.
• Vogel612: Hey there. I've reverted your rollback for two reasons: 1. the link to github that your old post contained pointed to master, which is not a defined deeplink, and therefore may point to a revision different from the one you're presenting here. To avoid confusion a community member linked to a specfic revision. And secondly they also edited the title to be more in line with the title format this community expects. As such the edit was IMO a definitive improvement and should remain applied.

Answer 1

think I should prefix this by saying I'm not a JS or Crypto nerd, but am interested in both! to other people seeing this question, this code/module seems to mostly reimplement the CCM demo at the bottom of the Crypto docs: https://nodejs.org/api/crypto.html#crypto_ccm_mode

anyway on with a few issues I noticed:

1. why isn't _iv just returning crypto.randomBytes(iv_size)? encoding to a string and then getting bytes seems counterproductive at best, it just seems to throw away entropy

2. why use PBKDF2 over just hashing the secret? it's not being saved anywhere, and by using the secret as the salt you've just opened it wide up to rainbow table exploitation again. further, by doing your string encoding things you've just thrown away even more entropy

3. your use of regexes in _matrix looks suspect. why not just match against a few common patterns (e.g. /^\w+-\w+-\w+/) and handle the groups explicitly. allowing modes like ECB at all here looks like a mistake

4. catching exceptions just to immediately rethrow doesn't seem very useful

5. can you think of a better name for the member variable flag? maybe something like is_aead_mode

update:

another big issue just occurred to me; that you're not including any cryptographic details in the structure. this allows the suite used to be upgraded (e.g. NIST update their recommendations) while maintaining backwards compatibility. i.e. you should be saying that you're hashing the secret with SHA512, which algorithm was used for encryption, what you've used as a HMAC (if you've used it, CCM already includes its own MAC so HMAC isn't needed).

• jas-: The module tries to handle every cipher type and mode, including ECB. Regarding _iv(iv_size), the node.js crypto API uses buffers extensively and although specifying the size as x,the result is a UTF-8 which is 1-4 bytes per element returned from .getRandomBytes(), slicing the UTF-8 encoded buffer is necessary to maintain the iv size for all algorithms. So am I really throwing away entropy? Thanks for pointing out the variable name and the salt, I will adjust and generate a new random set of bytes for the salt. Use of PBKDF conforms to the recommended key derivation.
• Sam Mason: what's getRandomBytes? crypto.randomBytes(4) gives me (node v12) a Buffer of length 4, as expected. not sure why you're talking about utf8. the randomBytes code had a big rework for v10 where Buffers which introduced the non-callback functionality, but it's always returned a Buffer
• jas-: You are right, that was a typo and the buffer does return the correct size. Can you explain to me how this project is similar to the code for the CCM module? Use of .randomBytes() for a salt; if I do it this way I have to include the salt in the returned object in order to decrypt correct? Can you explain how this opens it up to rainbow table exploitation?
• Sam Mason: by "rainbow table" I mean that the value returned from _derive_key is (assuming the hash algorithm is fixed) uniquely determined by the "secret", an attacker can easily precalculate/reuse the output of this function. wikipedia explains cryptographic salt reasonably well, and also reiterates that this is used for storing hashed passwords, which you're not doing
• jas-: Thanks! How is that different than the crypt() function and it’s use of a randomly generated salt embedded in the resulting hash for sha512 etc? I ask because that is always a fixed length value which can then be reused to crack the original value applied to the key derivation could it not? NIST SP 800-132 recommends a random value of 128 bits so I suppose I need to refactor and pass the salt along like other implementations. Regarding the format of crypt output and the salt being included in the hash: tunnelsup.com/hash-analyzer