bocoup / community-organization-operations-suite

Applications & tools for Community-Based Organizations (CBOs) to work together more effectively
MIT License
12 stars 7 forks source link

Security Improvements #657

Open mdeitner opened 2 years ago

mdeitner commented 2 years ago

After a security review meeting with Larry Joy (Microsoft) some recommendations were made:

Recommended as ‘best practices’ when doing crypto in the browser:

Use the Web Crypto API when available:

Pros:

Crypto is performed in this OS and not in browser/JavaScript-engine. The keys and intermediate computations cannot be accessed through the browser dev tools. No need for third-party packages Crypto code maintained by OS Better performance Cons:

Can be more difficult to program Not available on all platforms May not have specific required algorithms

Don’t use AES-CBC if possible. It’s not broken per se - but it has weaknesses and is not an authenticated form of encryption.

As discussed, using CBC here in offline mode doesn’t appear to be risk.

Suggestion:

Combine the two-step authentication (hash validation then decrypt validation) into a single step (password directly to encryption key and attempt validation decryption).

The two step works fine, but theoretically provides a larger attack surface to figure out the password. We discussed a possible password-change scenario where the two-step method could make sense, but we couldn’t recall the details during the meeting. Something to consider.

Scenarios we discussed that need to be explored:

Could a user take an offline mode tablet and gain access to the server when it returns online?

While no riskier than taking an ‘online’ device, offline mode authentication may not timeout the same way online mode does, and any offline timeout may be circumvented with the dev tools

When returning to online mode, an expired session cookie ‘should’ prevent the server connection. Testing should be done to make sure this is the case, and that the cookie is not auto-renewed.

What are the security/usability risks when the app rapidly moves in and out of offline/online mode?

This could be a mess and require some testing – or making online/offline mode switches manual.

Things I considered later:

If a user Alice, in offline mode, enters some data that is encrypted – then user Bob, on the same device, also in offline mode, logs in enters additional encrypted data, can the app data store handle data encrypted with multiple user keys?

If two offline devices store conflicting data about ‘Joe Smith’, what data ‘wins’ when these devices return online and the data is sent to the server?

mdeitner commented 2 years ago

Example code for working with Web Crypto API

(async () => {
    const username = "User1";
    const password = "k8FD61mNuh9M";
    const salt = crypto.getRandomValues(new Uint8Array(16));

    const data = {
        user: "Joe Smith",
        data: 123,
    };

    // creates AES-GCM key from a username/password
    const key = await passwordToAESKey(`${username}${password}`, salt);

    // encrypt text with the key
    const encrypted = await encrypt(JSON.stringify(data), key);

    // decrypts back to the original text
    const plain = await decrypt(encrypted, key)

    console.log(plain);  // should be the 'data' as JSON
})()

async function passwordToAESKey(password, salt) {
    const encoder = new TextEncoder();
    const utf8Bytes = encoder.encode(password);

    // import the password as a PBKDF2 key
    const pbkdf2Key = await crypto.subtle
        .importKey("raw", utf8Bytes, { name: "PBKDF2" },
            false, // not exportable (the key data cannot be exported)
            ["deriveKey"]
        )
        .catch((err) => {
            throw err;
        });

    // derive a new crypto key from the pbkdf2 key
    const aesKey = await crypto.subtle
        .deriveKey(
            {
                name: "PBKDF2",
                salt: salt,
                iterations: 2000,  // slows the password hashing down like in bcrypt
                hash: { name: "SHA-256" },
            },
            pbkdf2Key,
            { name: "AES-GCM", length: 256, },
            false,
            ["encrypt", "decrypt"]
        )
        .catch((err) => {
            throw err;
        });

    // this key object is opaque to the JavaScript environment
    // the actual key material and crypto computations using it will be in the OS and not in JavaScript
    return aesKey;
}

async function encrypt(text, key, additionalData = new Uint8Array()) {
    const encoder = new TextEncoder();
    const utf8Bytes = encoder.encode(text);

    // generate a new random IV - we'll need this for decryption
    const iv = crypto.getRandomValues(new Uint8Array(12));
    const cipherBytes = await crypto.subtle
        .encrypt(
            {
                name: "AES-GCM",
                // always generate a new iv every time you encrypt with a given key
                // recommended to use 12 bytes
                iv,
                additionalData,  // optional (if you do provide this, you'll also need it for decryption)
                tagLength: 128, // 128 (default)
            },
            key,
            utf8Bytes
        )
        .catch(function (err) {
            throw err;
        });

    // prepend the iv to the cipher since we'll need this IV to decrypt
    return new Uint8Array([...iv, ...new Uint8Array(cipherBytes)]);
}

async function decrypt(cipher, key, additionalData = new Uint8Array()) {
    let utf8decoder = new TextDecoder();
    // iv is the first 12 bytes - the remainder is the cipher
    const iv = cipher.slice(0, 12);
    cipher = cipher.slice(12);
    const plainBytes = await crypto.subtle
        .decrypt(
            {
                name: "AES-GCM",
                iv,
                additionalData, // only required if you provided this at encryption
                tagLength: 128,
            },
            key,
            cipher
        )
        .catch(function (err) {
            console.log(err);
            return new Uint8Array();
        });

    return utf8decoder.decode(plainBytes);
}