Skip to main content
added 220 characters in body
Source Link

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.

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.

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.

added 12 characters in body
Source Link
const address = computeDigest(
  'SHA_1',
  file.getId() + SpreadsheetApp2.getActiveSpreadsheet().getId(),
  'UTF_8');
CacheService2.put('user', address, 'string', passphrase.getResponseText(), 120);

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

const address = computeDigest(

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

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

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.

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.