Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement RFC4880bis-04 #691

Merged
merged 51 commits into from Apr 30, 2018

Conversation

Projects
None yet
4 participants
@twiss
Copy link
Contributor

commented Apr 25, 2018

Fixes #627. Previous discussion: twiss#1.

Implementation Notes

Native AES-EAX

Native Web Crypto and Node Crypto do not support AES-EAX, but they do support AES-CTR, which AES-EAX is built upon. Therefore, I've implemented EAX in a way that is agnostic to the underlying CTR implementation, and automatically selects native CTR whenever available.

Similarly, EAX also makes use of CMAC, which in turn makes use of CBC, which is supported by Web and Node Crypto, so I've made use of that as well.

EAX mode itself (as opposed to AES-EAX) does not require AES and is flexible enough to be used with any block cipher, but RFC4880bis-04 only mandates support for AES-128. We support AES-{128, 192, 256} (although AES-192 is not supported by Web Crypto in Chrome).

A note on implementing a draft specification

RFC4880bis-04 is a work in progress and may change at any time, and any implementation of it should be considered as such as well. The OpenPGP.js documentation already did a good job of making users aware that enabling AEAD may break compatibility with other implementations, but I have now also added a warning that future versions of OpenPGP.js may break compatibility with it, as well.

Backwards compatibility

Unfortunately, RFC4880bis-04 is already incompatible with our current implementation of the previous draft: for example, the AEAD Encrypted Data Packet has gained fields for the cipher algorithm, AEAD algorithm, and chunk size.

A look at GitHub code search reveals that aead_protect is actually used quite a lot, even in apps that store encrypted PGP messages on disk or present them to the user (luckily no large projects seem to do this). This is probably because our first example of setting up OpenPGP.js includes it (which is modified now).

Therefore, we have decided to maintain support for the previous version, and only enable support for the new version when setting openpgp.config.aead_protect_version = 4.

AEAD Support Feature Flags

The new draft adds feature flags for AEAD support in public keys. We add those flags when support for the new draft is enabled. When encrypting using a public key, we only use AEAD when the target key claims support for it. (This only applies for the new draft — the old draft didn't support feature flags so we don't look for them.)

GCM in the new draft

Because GCM is faster than EAX and OCB using Web Crypto (although EAX doesn't lag too far behind, it's not 100% native as GCM is, and so still a bit slower), we have implemented it in the new draft as well, as a "private/experimental algorithm" (with algorithm ID 100). Since GCM is not defined by the spec, we had to decide on a few implementation details ourselves, most notably the computation of the nonce from the IV and chunk counter. A future version of the standard may define GCM mode differently, hopefully under a different ID (e.g. ID 3) so that we can maintain backwards compatibility.

You can enable GCM by setting openpgp.config.aead_mode = openpgp.enums.aead.gcm.

Preferred AEAD Algorithms

The new draft also adds a packet containing the AEAD algorithms that the user of the public key prefers. When encrypting using a public key, those preferences have precedence over the aead_mode setting. When generating a public key, we claim preference for EAX over OCB, since EAX is for a large part implemented using native crypto.

The aead_mode setting currently does not influence the generated preferences, causing it to have no effect when solely using public-key cryptography. This restriction also applies to the other algorithm options (prefer_hash_algorithmencryption_cipher and compression) and could be fixed together with those in the future.

Version 5 Keys

The draft also specifies V5 Public/Private Key packets and V5 Symmetric-Key Encrypted Session Key packets. The main difference with their V4 counterparts is that they encrypt their contents with an AEAD algorithm. We generate these V5 packets when support for this draft version is enabled.

V5 Public/Private Keys also have some other minor improvements. As per the spec:

The version 5 format is similar to the version 4 format except for the addition of a count for the key material. This count helps parsing secret key packets (which are an extension of the public key packet format) in the case of an unknown algoritm. In addition, fingerprints of version 5 keys are calculated differently from version 4 keys.

@twiss twiss force-pushed the twiss:draft04 branch 3 times, most recently from b2565ca to a410177 Apr 25, 2018

@twiss twiss force-pushed the twiss:draft04 branch 2 times, most recently from fe3ac72 to 8429a4f Apr 25, 2018

@bartbutler
Copy link
Member

left a comment

Looks great, see comments

this.data = util.str_to_Uint8Array(util.encode_utf8(text));
Literal.prototype.setText = function(text, format='utf8') {
this.format = format;
this.text = text;

This comment has been minimized.

Copy link
@bartbutler

bartbutler Apr 26, 2018

Member

Zero out this.data here as well? Also should probably set this.text to null in setBytes if not done already.

This comment has been minimized.

Copy link
@twiss

twiss Apr 27, 2018

Author Contributor

Good idea 👍 done

const algo_enum = new Uint8Array([enums.write(enums.symmetric, this.sessionKeyAlgorithm)]);

const private_key = util.concatUint8Array([algo_enum, this.sessionKey]);

This comment has been minimized.

Copy link
@bartbutler

bartbutler Apr 26, 2018

Member

Remove some newlines?

@twiss twiss force-pushed the twiss:draft04 branch 2 times, most recently from 667fa39 to 9afbda8 Apr 30, 2018

twiss added some commits Apr 29, 2018

Lower chunk_size_byte to 12 (256KiB)
- In anticipation of streaming decryption
- Firefox 34 does not support chunk_size_byte > 24

256KiB is almost as fast as no chunks (although both of those can be up to
~1.5x slower than optimally using threads for very large message sizes).
The optimal chunk size would be something like:

    max(data.length / navigator.hardwareConcurrency, 128KiB)

But we don't do so currently because

- We don't know the hardwareConcurrency of the decrypting machine
- Smaller chunk sizes are better for streaming decryption

@twiss twiss force-pushed the twiss:draft04 branch 2 times, most recently from b7e8651 to 9afcf84 Apr 30, 2018

@twiss twiss force-pushed the twiss:draft04 branch from 9afcf84 to a16d1a6 Apr 30, 2018

@sanjanarajan sanjanarajan merged commit d562c14 into openpgpjs:master Apr 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@twiss twiss deleted the twiss:draft04 branch Apr 30, 2018

| brainpoolP384r1 | ECDH | ECDSA | Yes | Yes* | No |
| brainpoolP512r1 | ECDH | ECDSA | Yes | Yes* | No |
| curve25519 | ECDH | N/A | Yes | No | No |
| ed25519 | N/A | EdDSA | Yes | No | No |

This comment has been minimized.

Copy link
@mahrud

mahrud Apr 30, 2018

Contributor

Just out of curiosity, is there a specific reason why X25519 with Node Crypto doesn't work?

This comment has been minimized.

Copy link
@twiss

twiss Apr 30, 2018

Author Contributor

Yeah, see #680 (comment)

@@ -182,14 +182,14 @@ Curve.prototype.keyFromPublic = function (pub) {

Curve.prototype.genKeyPair = async function () {
let keyPair;
if (webCrypto && this.web) {

This comment has been minimized.

Copy link
@mahrud

mahrud Apr 30, 2018

Contributor

Was this a big fix? If so and webCrypto and nodeCrypto variables aren't used anymore you should delete them from the top of the file too.

This comment has been minimized.

Copy link
@twiss

twiss Apr 30, 2018

Author Contributor

The fix was that, since getWebCrypto() checks config.use_native, caching the result of that function breaks that config value. webCrypto and nodeCrypto are still used later on in the file in web/nodeGenKeyPair.

@@ -45,7 +45,7 @@ function KeyPair(curve, options) {
}

KeyPair.prototype.sign = async function (message, hash_algo) {
if (webCrypto && this.curve.web) {
if (this.curve.web && util.getWebCrypto()) {

This comment has been minimized.

Copy link
@mahrud

mahrud Apr 30, 2018

Contributor

Same here. If webCrypto and nodeCrypto aren't used they can be deleted above.

@@ -46,7 +46,7 @@ function buildEcdhParam(public_algo, oid, cipher_algo, hash_algo, fingerprint) {
new Uint8Array([public_algo]),
kdf_params.write(),
util.str_to_Uint8Array("Anonymous Sender "),
fingerprint
fingerprint.subarray(0, 20)

This comment has been minimized.

Copy link
@mahrud

mahrud Apr 30, 2018

Contributor

Wait, is this supposed to be the first 20 bytes of the fingerprint or the first 10 bytes? If the input is a hex string here then this takes the first 10 bytes. Right?

This comment has been minimized.

Copy link
@twiss

twiss Apr 30, 2018

Author Contributor

fingerprint is an Uint8Array here.

This comment has been minimized.

Copy link
@mahrud

mahrud Apr 30, 2018

Contributor

Oh, I see. I saw the hex_to_Uint8Array line removed a few lines below which threw me off.

This comment has been minimized.

Copy link
@twiss

twiss Apr 30, 2018

Author Contributor

Ah, yeah. I added an getFingerprintBytes function instead of that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.