JavaScript: proper user of SJCL encryption in Google Apps Script

Question

I'm using the minified version of SJCL to encrypt the backup files requested by the users. It is in a Google Apps Script add-on for Google Sheets.

I'm mostly concerned with correct use, exception handling and best practices of adding the encryption part in the script. It is currently working as expected in backup and in restore process.

The Backup (and Encryption)

User enter passphrase

It starts by asking the user to enter and re-enter the passphrase in a prompt. 1) One concern here is that the text field of the prompt doesn't mask the passphrase. To fix that I would show a dialog with HTML and form (password input). But then I would need to work client-server call and, while that is not an issue, the prompt is much simpler, less complicated and it won't be necessary to put a HTML - that could be questionable by the user?

const passphrase1 = ui.prompt(
  'Backup',
  'Enter passphrase:',
  ui.ButtonSet.OK_CANCEL);
if (passphrase1.getSelectedButton() === ui.Button.CANCEL) return 0;

const passphrase2 = ui.prompt(
  'Backup',
  'Please re-enter this passphrase:',
  ui.ButtonSet.OK_CANCEL);
if (passphrase2.getSelectedButton() === ui.Button.CANCEL) return 0;

Test passphrase

Here, the script tests if the passphrases match, and it also tests passphrase length and if it has at least upper, lower, numbers and special characters. It is a very simple test - so pA$$w0rd00 pass the test - and I actually didn't want to test anything like GnuPG does. In this case, is nothing (no test, like GnuPG) better than this simple test?

const passphrase = passphrase1.getResponseText();
if (passphrase !== passphrase2.getResponseText() || passphrase.length < 10 || testPassphrasePolicy(passphrase)) {
  ui.alert(
    'Backup',
    'Invalid passphrase.',
    ui.ButtonSet.OK);
  return 1;
}

function testPassphrasePolicy (passphrase) {
  if (!/[a-z]+/.test(passphrase)) return 1;
  if (!/[A-Z]+/.test(passphrase)) return 1;
  if (!/[0-9]+/.test(passphrase)) return 1;
  if (!/[~!@#$%^*-_=+[{\]}/;:,.?]+/.test(passphrase)) return 1;

  return 0;
}

Encrypt data

The encryption part is pretty straight forward. The backupis a object like backup = { foe: 'abc', bar: 123 }. The steps are as follow:

1. Stringify backup
2. Encode to base64
3. Compute SHA256
4. Concatenate the base64 with : and the SHA256
5. Encrypt with AES-128 in GCM mode and using the SHA256 as authentication data. If there is an error, simply return and show a generic message error.
6. Test decryption. If there is an error, simply return and show a generic message error.
7. Return a blob of the encrypted backup

I suspect steps 3 and 4 are overdoing, but before the addition of encryption that is how the script was doing an simple integrity test (with SHA1).

function encryptBackup_ (backup, passphrase) {
  const string = JSON.stringify(backup);
  const webSafeCode = Utilities.base64EncodeWebSafe(string, Utilities.Charset.UTF_8);

  const sha = computeDigest('SHA_256', webSafeCode, 'UTF_8');
  const data = webSafeCode + ':' + sha;

  let encrypted = '';
  try {
    encrypted = sjcl.encrypt(passphrase, data, { mode: "gcm", adata: sha });
  } catch (err) {
    ConsoleLog.error(err);
    return 0;
  }

  try {
    const decrypted = sjcl.decrypt(passphrase, encrypted);
    const parts = decrypted.split(':');
    const test_sha = computeDigest('SHA_256', parts[0], 'UTF_8');

    if (test_sha !== parts[1]) throw new Error('digestBackup_(): Bad decryption.');
  } catch (err) {
    ConsoleLog.error(err);
    return 0;
  }

  const date = Utilities.formatDate(DATE_NOW, 'GMT', 'yyyy-MM-dd-HH-mm-ss');
  const name = 'data' + date + '.backup';
  const blob = Utilities.newBlob(encrypted, 'application/octet-stream', name);

  return blob;
}

The Restore (and Decryption)

Once the user select a Drive file, the script requests the passphrase and tries to decrypt the file. This part is testing if it is possible to decrypt the file selected by the user.

The passphrase is being cached in a cache instance scoped to the current user and script so the script can retrieve it later to actually decrypt the file and restore the data. The cache expires in 120 seconds (session), and once it is retrieved later, it is immediately removed.

const ui = SpreadsheetApp.getUi();
const passphrase = ui.prompt(
  'Restore',
  'Enter passphrase:',
  ui.ButtonSet.OK_CANCEL);
if (passphrase.getSelectedButton() === ui.Button.CANCEL) return 0;

let decrypted = null;

try {
  decrypted = sjcl.decrypt(passphrase.getResponseText(), data);
} catch (err) {
  ConsoleLog.error(err);
  return 4;
}

const address = computeDigest(
  'SHA_1',
  file.getId() + SpreadsheetApp2.getActiveSpreadsheet().getId(),
  'UTF_8');
CacheService2.put('user', address, 'string', passphrase.getResponseText(), 120);

const parts = decrypted.split(':');
const test_sha = computeDigest('SHA_256', parts[0], 'UTF_8');

if (test_sha !== parts[1]) return 4;

const string = base64DecodeWebSafe(parts[0], 'UTF_8');
return JSON.parse(string);

Answer 1

User enter passphrase

Nobody uses an OS modal dialog prompts anymore. I'd suggest a web page specific modal box such as shown here. You need to click "new user" to have the box show up.

As for the naming, passphrase1 and passphrase1 seem to loose information, why not passphrase and passphraseReentered (or reenteredPassprase)? Now it seems like two different passwords to me.

Test passphrase

You haven't clearly documented which policy is being checked, although that might be in the design documentation or left out for now. But things like policy should not have to be derived from source code. It would be pretty tricky to validate the source code if it is leading so you would not be able to discern bugs.

if (passphrase !== passphrase2.getResponseText() || passphrase.length < 10 || testPassphrasePolicy(passphrase)) {

You're doing too much here, and it shows. First of all, checking if the password was correctly reentered is separate from testing the password policy. If it is a different check then it should be handled separately.

This will quickly become apparent anyway when users start to complain that they first entered the password twice before you check that the password complies with the policy (d'Oh!).

I'd argue that a length requirement should be part of the password policy, which removes another equation from the if.

if (!/[a-z]+/.test(passphrase)) return 1;

No, no no. First of all, at least return false or something similar, not 0 or 1. But shouldn't you indicate which test failed to the user?

After all these checks you perform return 0 which would actually sounds negative to me. However, I haven't seen that it fails on specific characters. That might be fine, but I'd personally make sure that the password doesn't contain any weird characters, control characters and such.

Encrypt data

I suspect steps 3 and 4 are overdoing, but before the addition of encryption that is how the script was doing an simple integrity test (with SHA1).

It is overdoing it, and since you're being incompatible with the original anyway, I'd remove the SHA-256 computation (note the dash in SHA-256).

const webSafeCode = Utilities.base64EncodeWebSafe(string, Utilities.Charset.UTF_8);

Why you would expand your plaintext by encoding it before encryption is beyond me, GCM handles binary just fine, thank you.

const sha = computeDigest('SHA_256', webSafeCode, 'UTF_8');

So what does this return so that you can put it at the end of a textual string? If it returns hexadecimals or base 64 then this should be made clear when looking at the call.

 let encrypted = '';

If you fail to assign you will hit return 0 and otherwise the assignment is performed in the next statement. Don't assign empty strings, or zero or null when it is not required.

encrypted = sjcl.encrypt(passphrase, data, { mode: "gcm", adata: sha });

JCL strengthens your passwords by a factor of 1000

Yeah, that might have been fine when PBKDF2, the password strengthening version was first created. Nowadays you'd want to hit a million at the least, and maybe increase the work factor later. Using (just) passwords for any encryption is tricky, unfortunately.

I'll not comment on the decryption part within the encryption function. I think it is overly cautious because on error I would not expect the code to hit return blob but since it is backup data...

 if (test_sha !== parts[1]) throw new Error('digestBackup_(): Bad decryption.');

But I will comment on badly copied code, we're not in digestBackup and errors already should contain a stacktrace, so there is no need to log it additionally.

const date = Utilities.formatDate(DATE_NOW, 'GMT', 'yyyy-MM-dd-HH-mm-ss');
const name = 'data' + date + '.backup';

A date in seconds and a filename that uses that date to distinguish backups. Even if this would never happen in reality, your unit / application testers are going to despise you. You need a better way of keeping blobs apart. Worse they may not catch it and you may overwrite a previous backup - current computers are fast, you can do a lot in a second, and maybe you want to backup before and after a process was executed.

Note that the encrypt function starting from the (unnecessary) SHA calculation up to the blob creation is completely ignorant of the data type. If you'd spit that out you can test the encryption / decryption separately. EDIT: On the other hand, you could use the file name as additional authenticated data (adata) so an attacker cannot get away with renaming the backups. Still, you could add that data as parameter to the function call.

The Restore (and Decryption)

First of all, where is the decryptBackup_ function? Always make your applications symmetric, even if the result is just a single line method. Imagine the future developer (i.e. you) searching for it. Now you suddenly mix UI code and the code doing the actual work as well.

return 4;

Warning, high octarine levels detected. Don't use magic numbers!

const address = computeDigest(
  'SHA_1',
  file.getId() + SpreadsheetApp2.getActiveSpreadsheet().getId(),
  'UTF_8');
CacheService2.put('user', address, 'string', passphrase.getResponseText(), 120);

Excuse me, what does this do? In here? At all?

if (test_sha !== parts[1]) return 4;

Four! Goofy golf ball error!

I like that you thought about your passphrase caching and management. Key/secret management is very important in crypto.