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

Replace strings with integer algorithm identifiers in packet classes #1410

Merged
merged 13 commits into from Nov 22, 2021

Conversation

larabr
Copy link
Collaborator

@larabr larabr commented Aug 25, 2021

In several packet classes, we used to store string identifiers for public-key, aead, cipher or hash algorithms. To make the code consistent and to avoid having to convert to/from string values, we now always store integer values instead, e.g. enums.symmetric.aes128 is used instead of 'aes128'.

This is not expected to be a breaking change for most library users. Note that the type of Key.getAlgorithmInfo() and of the session key objects returned and accepted by top-level functions remain unchanged.

Affected classes (type changes for some properties and method's arguments):

  • PublicKeyPacket, PublicSubkeyPacket, SecretKeyPacket, SecretSubkeyPacket
  • SymEncryptedIntegrityProtectedDataPacket, AEADEncryptedDataPacket, SymmetricallyEncryptedDataPacket
  • LiteralDataPacket, CompressedDataPacket
  • PublicKeyEncryptedSessionKey, SymEncryptedSessionKeyPacket
  • SignaturePacket

Other potentially breaking changes:

  • Removed property AEADEncryptedDataPacket.aeadAlgo, since it was redudant given .aeadAlgorithm.
  • Renamed AEADEncryptedDataPacket.cipherAlgo -> .cipherAlgorithm

TODO:

  • TS

@larabr larabr requested a review from twiss Aug 26, 2021
@@ -117,8 +117,7 @@ class S2K {
if (this.type === 'gnu-dummy') {
return new Uint8Array([101, 0, ...util.stringToUint8Array('GNU'), 1]);
}

const arr = [new Uint8Array([enums.write(enums.s2k, this.type), enums.write(enums.hash, this.algorithm)])];
const arr = [new Uint8Array([enums.write(enums.s2k, this.type), this.algorithm])];

switch (this.type) {
Copy link
Collaborator Author

@larabr larabr Aug 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to change this as well, but I didn't for now since we treat the gnu-dummy key as a special case, and I think it would require re-thinking more logic.

src/crypto/hash/index.js Outdated Show resolved Hide resolved
src/crypto/mode/gcm.js Outdated Show resolved Hide resolved
this.cipherAlgorithm = enums.write(enums.symmetric, await reader.readByte());
this.aeadAlgorithm = enums.write(enums.aead, await reader.readByte());
Copy link
Member

@twiss twiss Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not to get into the habit of using enums.write for checking an integer value. I know it supports it, but I think we should get a bit more strict about the types, and use read only for int -> string, and write only for string -> int. And then we can do validation in some other way - for example, in this case we will already throw below, in the read call in getAEADMode, if the AEAD mode is unknown (and similarly for the cipher, but during decryption).

Copy link
Collaborator Author

@larabr larabr Oct 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly disagree about doing optimistic parsing, I think we should detect wrong enums ASAP and not let this propagate to when we use the packet, with potentially unexpected side effects. It also makes it much easier to debug issues (as opposed to now). I'd rather add an enum function to validate the enum value -- so we also don't need to make exceptions for certain packets (about "where to throw").

Copy link
Member

@twiss twiss Oct 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually speaking, they're not necessarily "wrong enums", it might be that it's an algorithm that we just don't support. In other words, the spec does not forbid any values here, it's just that we might not be able to use all of them. And sometimes it might be useful to parse the packet, e.g. when building a packet listing, and just show it with an "unsupported algorithm", rather than showing the entire packet as "unsupported packet".

with potentially unexpected side effects

What side effects are you thinking of?

(There also shouldn't be a need for explicit exceptions, the "rule" would just be "throw when you use the enum value if we don't support it.")

By the way, in v5 public keys, we don't actually have to throw, as it includes a count of the public key material. We don't really use it today, but that was added exactly for the purpose of being able to parse a key without knowing the public key algorithm (and indeed bring it more in line with the other packets in that way).

Copy link
Collaborator Author

@larabr larabr Oct 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that in some cases it can be useful to allow parsing, but IMO it'll get in the way of a more solid error handling for the standard usage. I fundamentally think that an unknown algo inside a known packet leads to effectively having an unsupported packet, since it cannot be used as intended.
Plus, if we are going to be more strict in terms of typings, then we cannot really pass around values that are not necessarily of the specified enum type.

What side effects are you thinking of?

Nothing specific in mind, but we'd be keeping a potentially invalid value, trusting that the rest of the functions in the code can handle that securely.

Re. the packet listing example, I personally don't consider it a target use case of the library, but I think it could still be implemented by manually adding all the missing enums values and mapping them to 'unknown', for instance.

Copy link
Member

@twiss twiss Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other implications, for example if you have a key block with multiple keys, and one of them has a public key algorithm that we don't support, then it would be nice to still properly parse them as separate keys, and have one of them be a key that we just can't use, (similarly to if it is a DSA key, for example), rather than throwing an error. And then, if you have a message that's signed with both, for example, it can still verify. Of course, then if you try to encrypt a message to both keys, it will still throw, as expected, but then at least the application could choose to encrypt with only one of the keys, if it wants to.

Nothing specific in mind, but we'd be keeping a potentially invalid value, trusting that the rest of the functions in the code can handle that securely.

I don't think there should be any security implications of parsing these packets, since we don't support the algorithm we will always throw when we try to use it.

src/packet/compressed_data.js Outdated Show resolved Hide resolved
src/packet/one_pass_signature.js Outdated Show resolved Hide resolved
src/packet/public_key.js Outdated Show resolved Hide resolved
@larabr larabr added this to the v5.1 milestone Oct 25, 2021
@larabr larabr requested a review from twiss Nov 17, 2021
twiss
twiss approved these changes Nov 17, 2021
@larabr larabr merged commit 6da1c53 into openpgpjs:master Nov 22, 2021
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants