From c27725782c9f74c67f234e225bc2cd18f516fa0e Mon Sep 17 00:00:00 2001 From: Tom James Holub Date: Fri, 21 Jul 2017 10:13:33 -0700 Subject: [PATCH 1/6] do not fail when missing armor checksum | #563 --- src/config/config.js | 1 + src/encoding/armor.js | 10 ++++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/config/config.js b/src/config/config.js index 78bb1f96..131205d3 100644 --- a/src/config/config.js +++ b/src/config/config.js @@ -40,6 +40,7 @@ export default { aead_protect: false, // use Authenticated Encryption with Additional Data (AEAD) protection for symmetric encryption integrity_protect: true, // use integrity protection for symmetric encryption ignore_mdc_error: false, // fail on decrypt if message is not integrity protected + checksum_required: false, // do not throw error when armor is missing a checksum rsa_blinding: true, use_native: true, // use native node.js crypto and Web Crypto apis (if available) zero_copy: false, // use transferable objects between the Web Worker and main thread diff --git a/src/encoding/armor.js b/src/encoding/armor.js index 9dcbe891..70eb8545 100644 --- a/src/encoding/armor.js +++ b/src/encoding/armor.js @@ -247,6 +247,9 @@ function splitChecksum(text) { if (lastEquals >= 0) { body = text.slice(0, lastEquals); checksum = text.slice(lastEquals + 1); + if (checksum.substr(0, 6) === '\n-----') { + checksum = ''; // missing armor checksum + } } return { body: body, checksum: checksum }; @@ -311,10 +314,9 @@ function dearmor(text) { checksum = checksum.substr(0, 4); - if (!verifyCheckSum(result.data, checksum)) { - throw new Error("Ascii armor integrity check on message failed: '" + - checksum + - "' should be '" + + if (!verifyCheckSum(result.data, checksum) && (checksum || config.checksum_required)) { + // will NOT throw error if checksum is empty AND checksum is not required (GPG compatibility) + throw new Error("Ascii armor integrity check on message failed: '" + checksum + "' should be '" + getCheckSum(result.data) + "'"); } From 80742bdfbe9a451070805530411bc820182122d0 Mon Sep 17 00:00:00 2001 From: Tom James Holub Date: Fri, 21 Jul 2017 10:15:49 -0700 Subject: [PATCH 2/6] tests extended - armor checksum validation | #563 --- test/general/armor.js | 95 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 91 insertions(+), 4 deletions(-) diff --git a/test/general/armor.js b/test/general/armor.js index f5ceb2e3..7988df98 100644 --- a/test/general/armor.js +++ b/test/general/armor.js @@ -131,7 +131,7 @@ describe("ASCII armor", function() { expect(msg).to.throw(Error, /Unknown ASCII armor type/); }); - it('Armor checksum validation', function () { + it('Armor checksum validation - mismatch', function () { var privKey = ['-----BEGIN PGP PRIVATE KEY BLOCK-----', 'Version: OpenPGP.js v0.3.0', @@ -152,9 +152,96 @@ describe("ASCII armor", function() { '=wJN@', '-----END PGP PRIVATE KEY BLOCK-----'].join('\n'); - var result = openpgp.key.readArmored(privKey); - expect(result.err).to.exist; - expect(result.err[0].message).to.match(/Ascii armor integrity check on message failed/); + // try with default config + var result_1 = openpgp.key.readArmored(privKey); + expect(result_1.err).to.exist; + expect(result_1.err[0].message).to.match(/Ascii armor integrity check on message failed/); + + // try opposite config + openpgp.config.checksum_required = !openpgp.config.checksum_required; + var result_2 = openpgp.key.readArmored(privKey); + expect(result_2.err).to.exist; + expect(result_2.err[0].message).to.match(/Ascii armor integrity check on message failed/); + + // back to default + openpgp.config.checksum_required = !openpgp.config.checksum_required; + }); + + it('Armor checksum validation - valid', function () { + var privKey = + ['-----BEGIN PGP PRIVATE KEY BLOCK-----', + 'Version: OpenPGP.js v0.3.0', + 'Comment: http://openpgpjs.org', + '', + 'xbYEUubX7gEBANDWhzoP+Tr/IyRSv++vl5jBesQIPTYGQBdzF4YDnGEBABEB', + 'AAH+CQMIfzdw4/PKNl5gVXdtfDFdSIN8yJT2rbeg3+SsWexXZNNdRaONWaiB', + 'Z5cG9Q6+BoXKsEshIdcYOgwsAgRxlPpRA34Vvmg2QBk7PhdrkbK7aqENsJ1w', + 'dIlLD6p9GmLE20yVff58/fMiUtPRgsD83SpKTAX6EM1ulpkuQQNjmrVc5qc8', + '7AMdF80JdW5kZWZpbmVkwj8EEAEIABMFAlLm1+4JEBD8MASZrpALAhsDAAAs', + 'QgD8CUrwv7Hrp/INR0/UvAvzS52VztREQwQWTJMrgTNHBGjHtgRS5tfuAQEA', + 'nys9SaSgR+l6iZc/M8hGIUmbuahE2/+mtw+/l0RO+WcAEQEAAf4JAwjr39Yi', + 'FzjxImDN1IoYVsonA9M+BtIIJHafuQUHjyEr1paJJK5xS6KlyGgpMTXTD6y/', + 'qxS3ZSPPzHGRrs2CmkVEiPmurn9Ed05tb0y9OnJkWtuh3z9VVq9d8zHzuENa', + 'bUfli+P/v+dRaZ+1rSOxUFbFYbFB5XK/A9b/OPFrv+mb4KrtLxugwj8EGAEI', + 'ABMFAlLm1+4JEBD8MASZrpALAhsMAAC3IgD8DnLGbMnpLtrX72RCkPW1ffLq', + '71vlXMJNXvoCeuejiRw=', + '=wJNM', + '-----END PGP PRIVATE KEY BLOCK-----'].join('\n'); + + // try with default config + var result_1 = openpgp.key.readArmored(privKey); + expect(result_1.err).to.not.exist; + + // try opposite config + openpgp.config.checksum_required = !openpgp.config.checksum_required; + var result_2 = openpgp.key.readArmored(privKey); + expect(result_2.err).to.not.exist; + + // back to default + openpgp.config.checksum_required = !openpgp.config.checksum_required; + }); + + it('Armor checksum validation - missing', function () { + var privKeyNoCheckSum = + ['-----BEGIN PGP PRIVATE KEY BLOCK-----', + 'Version: OpenPGP.js v0.3.0', + 'Comment: http://openpgpjs.org', + '', + 'xbYEUubX7gEBANDWhzoP+Tr/IyRSv++vl5jBesQIPTYGQBdzF4YDnGEBABEB', + 'AAH+CQMIfzdw4/PKNl5gVXdtfDFdSIN8yJT2rbeg3+SsWexXZNNdRaONWaiB', + 'Z5cG9Q6+BoXKsEshIdcYOgwsAgRxlPpRA34Vvmg2QBk7PhdrkbK7aqENsJ1w', + 'dIlLD6p9GmLE20yVff58/fMiUtPRgsD83SpKTAX6EM1ulpkuQQNjmrVc5qc8', + '7AMdF80JdW5kZWZpbmVkwj8EEAEIABMFAlLm1+4JEBD8MASZrpALAhsDAAAs', + 'QgD8CUrwv7Hrp/INR0/UvAvzS52VztREQwQWTJMrgTNHBGjHtgRS5tfuAQEA', + 'nys9SaSgR+l6iZc/M8hGIUmbuahE2/+mtw+/l0RO+WcAEQEAAf4JAwjr39Yi', + 'FzjxImDN1IoYVsonA9M+BtIIJHafuQUHjyEr1paJJK5xS6KlyGgpMTXTD6y/', + 'qxS3ZSPPzHGRrs2CmkVEiPmurn9Ed05tb0y9OnJkWtuh3z9VVq9d8zHzuENa', + 'bUfli+P/v+dRaZ+1rSOxUFbFYbFB5XK/A9b/OPFrv+mb4KrtLxugwj8EGAEI', + 'ABMFAlLm1+4JEBD8MASZrpALAhsMAAC3IgD8DnLGbMnpLtrX72RCkPW1ffLq', + '71vlXMJNXvoCeuejiRw=', + '-----END PGP PRIVATE KEY BLOCK-----'].join('\n'); + + // try with default config + var result_1 = openpgp.key.readArmored(privKeyNoCheckSum); + if(openpgp.config.checksum_required) { + expect(result_1.err).to.exist; + expect(result_1.err[0].message).to.match(/Ascii armor integrity check on message failed/); + } else { + expect(result_1.err).to.not.exist; + } + + // try opposite config + openpgp.config.checksum_required = !openpgp.config.checksum_required; + var result_2 = openpgp.key.readArmored(privKeyNoCheckSum); + if(openpgp.config.checksum_required) { + expect(result_2.err).to.exist; + expect(result_2.err[0].message).to.match(/Ascii armor integrity check on message failed/); + } else { + expect(result_2.err).to.not.exist; + } + + // back to default + openpgp.config.checksum_required = !openpgp.config.checksum_required; }); it('Accept header with trailing whitespace', function () { From 841b03d6cd5fae491a8b8549384e999dd6e93d8a Mon Sep 17 00:00:00 2001 From: Tom James Holub Date: Fri, 21 Jul 2017 15:35:27 -0700 Subject: [PATCH 3/6] improved armor behavior - trailing newline --- src/encoding/armor.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/encoding/armor.js b/src/encoding/armor.js index 70eb8545..14909608 100644 --- a/src/encoding/armor.js +++ b/src/encoding/armor.js @@ -246,10 +246,7 @@ function splitChecksum(text) { if (lastEquals >= 0) { body = text.slice(0, lastEquals); - checksum = text.slice(lastEquals + 1); - if (checksum.substr(0, 6) === '\n-----') { - checksum = ''; // missing armor checksum - } + checksum = text.slice(lastEquals + 1).trim().substr(0, 4); } return { body: body, checksum: checksum }; @@ -271,6 +268,7 @@ function dearmor(text) { var type = getType(text); + text = text.trim() + "\n"; var splittext = text.split(reSplit); // IE has a bug in split with a re. If the pattern matches the beginning of the @@ -312,8 +310,6 @@ function dearmor(text) { checksum = sig_sum.checksum; } - checksum = checksum.substr(0, 4); - if (!verifyCheckSum(result.data, checksum) && (checksum || config.checksum_required)) { // will NOT throw error if checksum is empty AND checksum is not required (GPG compatibility) throw new Error("Ascii armor integrity check on message failed: '" + checksum + "' should be '" + From 10896c2cbfa00816f34ac9739bd39a2d4d82cf78 Mon Sep 17 00:00:00 2001 From: Tom James Holub Date: Fri, 21 Jul 2017 15:37:25 -0700 Subject: [PATCH 4/6] added missing checksum test with traling armor newline --- test/general/armor.js | 44 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/test/general/armor.js b/test/general/armor.js index 7988df98..073c35a5 100644 --- a/test/general/armor.js +++ b/test/general/armor.js @@ -244,6 +244,50 @@ describe("ASCII armor", function() { openpgp.config.checksum_required = !openpgp.config.checksum_required; }); + it('Armor checksum validation - missing - trailing newline', function () { + var privKeyNoCheckSumWithTrailingNewline = + ['-----BEGIN PGP PRIVATE KEY BLOCK-----', + 'Version: OpenPGP.js v0.3.0', + 'Comment: http://openpgpjs.org', + '', + 'xbYEUubX7gEBANDWhzoP+Tr/IyRSv++vl5jBesQIPTYGQBdzF4YDnGEBABEB', + 'AAH+CQMIfzdw4/PKNl5gVXdtfDFdSIN8yJT2rbeg3+SsWexXZNNdRaONWaiB', + 'Z5cG9Q6+BoXKsEshIdcYOgwsAgRxlPpRA34Vvmg2QBk7PhdrkbK7aqENsJ1w', + 'dIlLD6p9GmLE20yVff58/fMiUtPRgsD83SpKTAX6EM1ulpkuQQNjmrVc5qc8', + '7AMdF80JdW5kZWZpbmVkwj8EEAEIABMFAlLm1+4JEBD8MASZrpALAhsDAAAs', + 'QgD8CUrwv7Hrp/INR0/UvAvzS52VztREQwQWTJMrgTNHBGjHtgRS5tfuAQEA', + 'nys9SaSgR+l6iZc/M8hGIUmbuahE2/+mtw+/l0RO+WcAEQEAAf4JAwjr39Yi', + 'FzjxImDN1IoYVsonA9M+BtIIJHafuQUHjyEr1paJJK5xS6KlyGgpMTXTD6y/', + 'qxS3ZSPPzHGRrs2CmkVEiPmurn9Ed05tb0y9OnJkWtuh3z9VVq9d8zHzuENa', + 'bUfli+P/v+dRaZ+1rSOxUFbFYbFB5XK/A9b/OPFrv+mb4KrtLxugwj8EGAEI', + 'ABMFAlLm1+4JEBD8MASZrpALAhsMAAC3IgD8DnLGbMnpLtrX72RCkPW1ffLq', + '71vlXMJNXvoCeuejiRw=', + '-----END PGP PRIVATE KEY BLOCK-----', + ''].join('\n'); + + // try with default config + var result_1 = openpgp.key.readArmored(privKeyNoCheckSumWithTrailingNewline); + if(openpgp.config.checksum_required) { + expect(result_1.err).to.exist; + expect(result_1.err[0].message).to.match(/Ascii armor integrity check on message failed/); + } else { + expect(result_1.err).to.not.exist; + } + + // try opposite config + openpgp.config.checksum_required = !openpgp.config.checksum_required; + var result_2 = openpgp.key.readArmored(privKeyNoCheckSumWithTrailingNewline); + if(openpgp.config.checksum_required) { + expect(result_2.err).to.exist; + expect(result_2.err[0].message).to.match(/Ascii armor integrity check on message failed/); + } else { + expect(result_2.err).to.not.exist; + } + + // back to default + openpgp.config.checksum_required = !openpgp.config.checksum_required; + }); + it('Accept header with trailing whitespace', function () { var privKey = ['-----BEGIN PGP PRIVATE KEY BLOCK-----\t \r', From ac055d69d2c60988f14b2f233958491b3399bae1 Mon Sep 17 00:00:00 2001 From: Tom James Holub Date: Fri, 21 Jul 2017 15:39:06 -0700 Subject: [PATCH 5/6] fixed outdated annotations in armor.js --- src/encoding/armor.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/encoding/armor.js b/src/encoding/armor.js index 14909608..1a6f0c04 100644 --- a/src/encoding/armor.js +++ b/src/encoding/armor.js @@ -191,8 +191,7 @@ function createcrc24(input) { /** * Splits a message into two parts, the headers and the body. This is an internal function * @param {String} text OpenPGP armored message part - * @returns {(Boolean|Object)} Either false in case of an error - * or an object with attribute "headers" containing the headers and + * @returns {Object} An object with attribute "headers" containing the headers * and an attribute "body" containing the body. */ function splitHeaders(text) { @@ -234,8 +233,7 @@ function verifyHeaders(headers) { /** * Splits a message into two parts, the body and the checksum. This is an internal function * @param {String} text OpenPGP armored message part - * @returns {(Boolean|Object)} Either false in case of an error - * or an object with attribute "body" containing the body + * @returns {Object} An object with attribute "body" containing the body * and an attribute "checksum" containing the checksum. */ function splitChecksum(text) { From 3f40a36081e88f51f8152cbec254a778dad5668b Mon Sep 17 00:00:00 2001 From: Tom James Holub Date: Fri, 21 Jul 2017 17:39:19 -0700 Subject: [PATCH 6/6] do not remove equal sign at the end of armored body when missing checksum --- src/encoding/armor.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/encoding/armor.js b/src/encoding/armor.js index 1a6f0c04..bc7abea4 100644 --- a/src/encoding/armor.js +++ b/src/encoding/armor.js @@ -237,14 +237,15 @@ function verifyHeaders(headers) { * and an attribute "checksum" containing the checksum. */ function splitChecksum(text) { + text = text.trim(); var body = text; var checksum = ""; var lastEquals = text.lastIndexOf("="); - if (lastEquals >= 0) { + if (lastEquals >= 0 && lastEquals !== text.length - 1) { // '=' as the last char means no checksum body = text.slice(0, lastEquals); - checksum = text.slice(lastEquals + 1).trim().substr(0, 4); + checksum = text.slice(lastEquals + 1).substr(0, 4); } return { body: body, checksum: checksum };