issue #1057 Parsing/decrypting of complex PGP messages in Kotlin by IvanPizhenko · Pull Request #1331 · FlowCrypt/flowcrypt-android

IvanPizhenko · 2021-07-04T17:17:47Z

This PR introduces native Kotlin-base processing of the complex email messages

close #1057

Tests added

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

IvanPizhenko · 2021-07-04T17:34:26Z

@tomholub I can see some Snyk tests failed, however I can't see what exactly. Please check.

seisvelas · 2021-07-05T07:20:42Z

@IvanPizhenko Sorry to answer a question not directed at me, but I checked on Snyk and it appears this PR introduces a vulnerable dependency. Specifically, com.googlecode.owasp-java-html-sanitizer:owasp-java-html-sanitizer:20200713.1, which is introduced here:

flowcrypt-android/FlowCrypt/build.gradle

Line 459 in 0c43360

    
           implementation 'com.googlecode.owasp-java-html-sanitizer:owasp-java-html-sanitizer:20200713.1'

Depends on a version of com.google.guava which suffers an Information Disclosure vulnerability (you can learn more about how this specific vuln works / can be exploited in Guava here: https://snyk.io/vuln/SNYK-JAVA-COMGOOGLEGUAVA-1015415)

The codebase on https://github.com/OWASP/java-html-sanitizer has actually updated to a non-vulnerable Guava version (here: OWASP/java-html-sanitizer@ad287c3), but there hasn't been a release in quite some time which is why we're having this problem.

The actual vulnerability is that files used in the temp dir created via Guava's createTempDir are readable by all users on the system. Upon deeper inspection, it appears owasp-java-html-sanitizer doesn't even use the vulnerable method, so in practice we should be safe.

Consequently, I've told Snyk to skip this check and mark it as passed.

tomholub · 2021-07-05T10:12:56Z

Thank you Alex for looking into this 👍

tomholub

Thanks! So far only reviewed the tests. Sorry if the answer is obvious.

tomholub · 2021-07-05T11:22:55Z

FlowCrypt/src/test/java/com/flowcrypt/email/security/pgp/PgpMsgTest.kt

    }
  }
+
+  // -------------------------------------------------------------------------------------------


Approximately 14 test files (.txt) were added below, but this test is using only a handful of them. Where are they being loaded and used?

All these files should be used in the tests here.

Looks like mime-email-*.txt are not used. Will remove them

DenBond7

@IvanPizhenko Please look at my comments. I didn't try your realization yet but it will be a start point of the final migration. On the current stage it's hard to say what is good or not good.

FlowCrypt/src/main/java/com/flowcrypt/email/core/msg/MimeUtils.kt

FlowCrypt/src/main/java/com/flowcrypt/email/extensions/java/io/InputStreamExt.kt

FlowCrypt/src/main/java/com/flowcrypt/email/extensions/kotlin/StringExt.kt

FlowCrypt/src/main/java/com/flowcrypt/email/api/retrofit/response/model/node/MsgBlock.kt

DenBond7 · 2021-07-05T15:27:28Z

FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt

    val type: PgpDecrypt.DecryptionErrorType,
    val message: String,
    val cause: Throwable? = null
-  )
+  ) : Parcelable {


unfortunately, we can't use Throwable in Parcelable implementation in data classes. We can't compare Throwable instance here. That's why some of JUnit tests were failed.

FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt

tomholub

This PR is pretty massive 👀 which reflects the issue is a complicated one. It looks good. On my end, I mainly want to make sure that all provided test cases are indeed tested (see earlier question) and if not, that we file an issue to test all soon.

tomholub · 2021-07-05T19:30:58Z

FlowCrypt/src/main/java/com/flowcrypt/email/core/msg/MimeUtils.kt

+    val firstChars = msg.copyOfRange(0, msg.size.coerceAtMost(1000))
+      .toString(StandardCharsets.US_ASCII)
+      .toLowerCase(Locale.ROOT)


I wonder if the conversion to String can fail with an exception if we give it arbitrary bytes that are non-ascii. If it can fail by throwing, we should catch the error and return false.

Did the small test:

import java.nio.charset.StandardCharsets import java.util.Locale fun main() { val a = ByteArray(256) for (i in 0..255) a[i] = i.toByte() val s = a.toString(StandardCharsets.US_ASCII).toLowerCase(Locale.ROOT) println(s) }

Works for all source byte values.

FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpKey.kt

FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt

tomholub · 2021-07-05T19:53:16Z

@IvanPizhenko Please look at my comments. I didn't try your realization yet but it will be a start point of the final migration. On the current stage it's hard to say what is good or not good.

As long as it passes the supplied test cases, then it's well good enough. Of course, some adjustments will be needed once you start integrating, but if we confirm that supplied test cases pass the way they do on TypeScript code (and code style stuff gets adjusted) then this is perfectly fine for merging even if we didn't try to integrate it yet.

IvanPizhenko · 2021-07-06T23:00:39Z

@IvanPizhenko Sorry to answer a question not directed at me, but I checked on Snyk and it appears this PR introduces a vulnerable dependency. Specifically, com.googlecode.owasp-java-html-sanitizer:owasp-java-html-sanitizer:20200713.1, which is introduced here:

flowcrypt-android/FlowCrypt/build.gradle

Line 459 in 0c43360

implementation 'com.googlecode.owasp-java-html-sanitizer:owasp-java-html-sanitizer:20200713.1'

Depends on a version of com.google.guava which suffers an Information Disclosure vulnerability (you can learn more about how this specific vuln works / can be exploited in Guava here: https://snyk.io/vuln/SNYK-JAVA-COMGOOGLEGUAVA-1015415)

The codebase on https://github.com/OWASP/java-html-sanitizer has actually updated to a non-vulnerable Guava version (here: OWASP/java-html-sanitizer@ad287c3), but there hasn't been a release in quite some time which is why we're having this problem.

The actual vulnerability is that files used in the temp dir created via Guava's createTempDir are readable by all users on the system. Upon deeper inspection, it appears owasp-java-html-sanitizer doesn't even use the vulnerable method, so in practice we should be safe.

Consequently, I've told Snyk to skip this check and mark it as passed.

Thanks Alex!

DenBond7 · 2021-07-07T11:53:40Z

@IvanPizhenko I've added some changes to fix tests

tomholub

I've reviewed the changes and they look good. I'll still give it a review regarding the test cases, then can merge.

tomholub · 2021-07-07T17:39:01Z

FlowCrypt/src/test/resources/PgpMsgTest/compat/direct-encrypted-text.txt

@@ -0,0 +1,14 @@
+-----BEGIN PGP MESSAGE-----


This file is not used anywhere that I can find

tomholub · 2021-07-07T17:39:42Z

FlowCrypt/src/test/resources/PgpMsgTest/compat/direct-encrypted-pgpmime.txt

@@ -0,0 +1,36 @@
+-----BEGIN PGP MESSAGE-----


This file is not used anywhere that I can find

IvanPizhenko requested review from DenBond7 and tomholub July 4, 2021 17:17

IvanPizhenko requested a review from seisvelas as a code owner July 4, 2021 17:17

issue #1057 Parsing/decrypting of complex PGP messages in Kotlin

0c43360

IvanPizhenko force-pushed the ip-1057 branch from c6f8d5f to 0c43360 Compare July 4, 2021 17:32

seisvelas previously approved these changes Jul 5, 2021

View reviewed changes

tomholub reviewed Jul 5, 2021

View reviewed changes

DenBond7 requested changes Jul 5, 2021

View reviewed changes

tomholub reviewed Jul 5, 2021

View reviewed changes

Rename constants

5304a38

IvanPizhenko dismissed seisvelas’s stale review via 5304a38 July 6, 2021 23:31

IvanPizhenko added 3 commits July 7, 2021 02:42

Rename constants

f67f5ad

Key normalization fix

c37ff1c

Remove unnecessary files

63d1410

DenBond7 added the PR submitted PR is submitted for this issue label Jul 7, 2021

DenBond7 added this to the 1.1.9 milestone Jul 7, 2021

Fixed JUnit tests.| #1331

e529096

DenBond7 approved these changes Jul 7, 2021

View reviewed changes

tomholub reviewed Jul 7, 2021

View reviewed changes

tomholub merged commit 7e1ca93 into master Jul 7, 2021

tomholub deleted the ip-1057 branch July 7, 2021 18:08

Conversation

IvanPizhenko commented Jul 4, 2021 • edited by tomholub Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

close #1057

To be filled by reviewers

Uh oh!

IvanPizhenko commented Jul 4, 2021

Uh oh!

seisvelas commented Jul 5, 2021

Uh oh!

tomholub commented Jul 5, 2021

Uh oh!

tomholub left a comment

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!

DenBond7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomholub left a comment

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!

Uh oh!

Uh oh!

Uh oh!

tomholub commented Jul 5, 2021

Uh oh!

IvanPizhenko commented Jul 6, 2021

Uh oh!

DenBond7 commented Jul 7, 2021

Uh oh!

tomholub left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

IvanPizhenko commented Jul 4, 2021 •

edited by tomholub

Loading