Add private key decrypt validation by martgil · Pull Request #1390 · FlowCrypt/flowcrypt-ios

martgil · 2022-02-21T08:42:39Z

This PR adds a private key validation test.

close #1346 // if this PR closes an issue

issue #0000 // if it doesn't close the issue yet

Tests (delete all except exactly one):

Tests will be added later (issue #...)
Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

addresses the issue it closes (if any)
code is readable and understandable
is accompanied with tests, or tests are not needed
is free of vulnerabilities
is documented clearly and usefully, or doesn't need documentation

…le use)

…rypt-validation

martgil · 2022-02-21T11:05:35Z

Hello sir, @IvanPizhenko,

I have added the initial validation to PgpKey.decrypt(). The bad private key from the test was now flagged as an invalid key.

I also think of adding the validation to PgpKey.parse() through PgpKey.normalize() but when the throw triggered on .validate() function of the Openpgp.js the return from PgpKey.normalization kinda overrides the error. (4cd6cb6)

So If I check the error message through throwAsync it receives the json debug data from normalized and not getting the "Error: key is invalid" native error.

Lastly, I feel like I'm still missing some place where to add the validation. My goal is to add it on PgpKey.decrypt and PgpKey.parse for the moment. If I missed anywhere other than those, I appreciate any suggestion.

Many Thanks!

tomholub · 2022-02-21T11:50:29Z

On normalize you can let the error be swallowed and test for it returning no keys.

When you add to parse, does it have the same issue?

IvanPizhenko

@martgil please see my remarks

IvanPizhenko · 2022-02-21T23:29:26Z

Core/source/core/pgp-key.ts

+  public static validateAllDecryptedPackets = async (key: Key): Promise<void> => {
+    const packets = key.toPacketList() as PacketList<SecretKeyPacket>;
+    for (const prvPacket of packets.filter(PgpKey.isPacketPrivate).filter(packet => packet.isDecrypted())) {
+      await (prvPacket as SecretKeyPacket).validate(); // gnu-dummy never raises an exception, invalid keys raise exceptions


should be type cast to BaseSecretKeyPacket

updated, thanks!

sir, I made a recent change that would work without further typecasting but by checking if the prvPacket is an instance of SecretKeyPacket. would that be ok?

IvanPizhenko · 2022-02-21T23:31:43Z

Core/source/core/pgp-key.ts

  }

+  public static validateAllDecryptedPackets = async (key: Key): Promise<void> => {
+    const packets = key.toPacketList() as PacketList<SecretKeyPacket>;


Please remove type cast to PacketList<SecretKeyPacket>, not all packets will be of type SecretKeyPacket, return type of the toPacketList() is PacketList<AllowedKeyPackets>, that's what we need to operate on.

thank you, sir, I revert the change to use PacketList<AllowedKeyPackets> and check if the instance of prvPacket is SecretKeyPacket and if the prvPacket is already decrypted because I don't have much idea of how I could make the .filter(PgpKey.isPacketPrivate) to work.

What's the problem with .filter(PgpKey.isPacketPrivate)? Why doesn't it work?
This code losing checks for SecretSubkeyPacket. I assumed it should be like this:

for (const packet of (key.toPacketList().filter(PgpKey.isPacketPrivate) as BaseSecretKeyPacket[])) { await packet.validate(); }

or even

I'm not sure but i'm getting the following type error when doing so:

No overload matches this call. Overload 1 of 2, '(predicate: (value: AllowedKeyPackets, index: number, array: AllowedKeyPackets[]) => value is PrvPacket, thisArg?: any): PrvPacket[]', gave the following error. Argument of type '(p: AnyKeyPacket) => p is PrvPacket' is not assignable to parameter of type '(value: AllowedKeyPackets, index: number, array: AllowedKeyPackets[]) => value is PrvPacket'. Types of parameters 'p' and 'value' are incompatible. Type 'AllowedKeyPackets' is not assignable to type 'BasePublicKeyPacket'. Type 'SignaturePacket' is missing the following properties from type 'BasePublicKeyPacket': algorithm, getAlgorithmInfo, getFingerprint, getFingerprintBytes, and 6 more. Overload 2 of 2, '(predicate: (value: AllowedKeyPackets, index: number, array: AllowedKeyPackets[]) => unknown, thisArg?: any): AllowedKeyPackets[]', gave the following error. Argument of type '(p: AnyKeyPacket) => p is PrvPacket' is not assignable to parameter of type '(value: AllowedKeyPackets, index: number, array: AllowedKeyPackets[]) => unknown'. Types of parameters 'p' and 'value' are incompatible. Type 'AllowedKeyPackets' is not assignable to type 'BasePublicKeyPacket'.ts(2769)

ok, I'll try to fix this myself a bit later.

thank you sir, I'll have my eyes on them so that I would learn from them and digest the idea as much as I can so that I can handle it myself the next time.

Maybe just change parameter type of isPrvPacket() to AllowedKeyPackets?

@martgil I've changed parameter type of the isPacketPrivate() to AllowedKeyPackets and it worked. I've pushed this change.

IvanPizhenko · 2022-02-21T23:43:48Z

I also think of adding the validation to PgpKey.parse() through PgpKey.normalize() but when the throw triggered on .validate() function of the Openpgp.js the return from PgpKey.normalization kinda overrides the error.

@martgil You can extend return type of normalize() with additional optional field called like exception or error, and store exception there if it has occurred and then inspect it in the test. Regarding parse() - it actually calls normalize(), so you can do the same - propagate error from normalize() to the object returned by parse().

…rypt-validation

martgil · 2022-02-22T06:47:23Z

@martgil You can extend return type of normalize() with additional optional field called like exception or error, and store exception there if it has occurred and then inspect it in the test. Regarding parse() - it actually calls normalize(), so you can do the same - propagate error from normalize() to the object returned by parse().

Thank you sir, it helps a lot. I tried to apply it the better way I know of - please correct me if any of the changes look awful. Thanks again!

martgil · 2022-02-22T07:01:01Z

On normalize you can let the error be swallowed and test for it returning no keys.

When you add to parse, does it have the same issue?

Thank you, sir, I also find this helpful with help of the additional field exception on normalize() and parse() - the test looks much clearer.

IvanPizhenko · 2022-02-22T12:24:42Z

Core/source/core/pgp-key.ts

  }

-  public static normalize = async (armored: string): Promise<{ normalized: string, keys: Key[] }> => {
+  public static normalize = async (armored: string): Promise<{ normalized: string, keys: Key[], exception: string }> => {


should be optional, i.e. exception?: string, i.e. it can be undefined.

name exception is a bit misleading. It could be exception if you would return here the whole exception object, but only error message is returned, as I can see later in the code. So I suggest to rename it into error, (and moreover, this will be more aligned with what we already have in this codebase).

oh, okay sir. I've rename it appropriately to error field, making it optional and removing the error from the returning data. thanks!

IvanPizhenko · 2022-02-22T12:25:20Z

Core/source/core/pgp-key.ts

        }
      }
-      return { normalized: keys.map(k => k.armor()).join('\n'), keys };
+      return { normalized: keys.map(k => k.armor()).join('\n'), keys, exception: '' };


no need to return exception here if it is declared optional

updated, thanks!

IvanPizhenko · 2022-02-22T12:25:59Z

Core/source/core/pgp-key.ts

-  public static parse = async (armored: string): Promise<{ original: string, normalized: string, keys: KeyDetails[] }> => {
-    const { normalized, keys } = await PgpKey.normalize(armored);
-    return { original: armored, normalized, keys: await Promise.all(keys.map(PgpKey.details)) };
+  public static parse = async (armored: string): Promise<{ original: string, normalized: string, keys: KeyDetails[], exception: string }> => {


same: exception should be optional

updated, thanks!

IvanPizhenko · 2022-02-22T12:28:46Z

@martgil Some more comments, please check.

…rypt-validation

IvanPizhenko · 2022-02-22T12:52:14Z

@martgil Generally looks good, I will try later today to solve this issue with isPrvPacket().

martgil added 2 commits February 21, 2022 16:04

remove debug console.log

8ec467a

add working a test for decrypting bad private key (for decrypt - sing…

c0b7935

…le use)

martgil requested a review from sosnovsky as a code owner February 21, 2022 08:42

martgil marked this pull request as draft February 21, 2022 08:42

martgil self-assigned this Feb 21, 2022

martgil added 2 commits February 21, 2022 19:03

add validation on PgpKey.parse validation (needs help)

4cd6cb6

Merge remote-tracking branch 'origin/master' into add-private-key-dec…

eee46e3

…rypt-validation

IvanPizhenko reviewed Feb 21, 2022

View reviewed changes

martgil added 3 commits February 22, 2022 12:39

Merge remote-tracking branch 'origin/master' into add-private-key-dec…

26af89f

…rypt-validation

add additional fields for reading exception + apply requested change

414f1d1

add checks if prvPacket is also decrypted

ab28d6a

IvanPizhenko reviewed Feb 22, 2022

View reviewed changes

martgil added 2 commits February 22, 2022 20:32

Merge remote-tracking branch 'origin/master' into add-private-key-dec…

dd71f62

…rypt-validation

apply requested change making exception field to error field optional

1245c87

Ivan Pizhenko added 2 commits February 22, 2022 23:17

Filter with isPacketPrivate()

f1b7671

variable name

fb1c30b

martgil marked this pull request as ready for review February 23, 2022 03:39

sosnovsky approved these changes Feb 23, 2022

View reviewed changes

sosnovsky merged commit 5c2bea5 into master Feb 23, 2022

sosnovsky deleted the add-private-key-decrypt-validation branch February 23, 2022 08:37

Conversation

martgil commented Feb 21, 2022

To be filled by reviewers

Uh oh!

martgil commented Feb 21, 2022

Uh oh!

tomholub commented Feb 21, 2022

Uh oh!

IvanPizhenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martgil Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanPizhenko Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanPizhenko commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martgil commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martgil commented Feb 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanPizhenko commented Feb 22, 2022

Uh oh!

IvanPizhenko commented Feb 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

martgil Feb 22, 2022 •

edited

Loading

IvanPizhenko Feb 22, 2022 •

edited

Loading

IvanPizhenko commented Feb 21, 2022 •

edited

Loading

martgil commented Feb 22, 2022 •

edited

Loading