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

Workaround TLS fragmented records #32

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Conversation

aitorvs
Copy link
Collaborator

@aitorvs aitorvs commented Nov 20, 2023

Task/Issue URL: https://app.asana.com/0/1202279501986195/1205874074739543/f

Description

We currently don't have support for TLS fragmented records and we think that lack of support is cause for the crashes/issues we see described in the asana task.

This PR doesn't support fragmented records but it takes them into consideration when parsing the TLS. The main goal would be twofold:

  • avoid issues/crashes due to out of bounds buffer addressing on a fragmented record
  • eagerly check TLS extensions to still block trakcers on fragmented records when the SNI falls into the first record

Steps to test this PR

  • from this branch, publish the library to maven local ie. ./gradlew clean assemble publishToMavenLocal
  • In the DDG android app apply the following path
Subject: [PATCH] Maven local use
---
Index: build.gradle
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build.gradle b/build.gradle
--- a/build.gradle	(revision d11f7491d7ab4b27223fd352f83c26be403e79ed)
+++ b/build.gradle	(revision 3b1fe446b5d33e4d8a7f400137134ea0b5a797d7)
@@ -40,6 +40,7 @@
     repositories {
         google()
         mavenCentral()
+        mavenLocal()
     }
     configurations.all {
         resolutionStrategy.force 'org.objenesis:objenesis:2.6'
Index: versions.properties
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>ISO-8859-1
===================================================================
diff --git a/versions.properties b/versions.properties
--- a/versions.properties	(revision d11f7491d7ab4b27223fd352f83c26be403e79ed)
+++ b/versions.properties	(revision 3b1fe446b5d33e4d8a7f400137134ea0b5a797d7)
@@ -55,7 +55,7 @@
 
 version.com.android.installreferrer..installreferrer=2.2
 
-version.com.duckduckgo.netguard..netguard-android=1.6.0
+version.com.duckduckgo.netguard..netguard-android=1.7.0-SNAPSHOT
 
 version.com.duckduckgo.synccrypto..sync-crypto-android=0.3.0
 
  • build DDG app
  • AppTP smoke tests

@aitorvs
Copy link
Collaborator Author

aitorvs commented Nov 20, 2023

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@@ -180,7 +178,13 @@ static int parse_server_name_extension(const uint8_t *data, size_t data_len, cha
}
strncpy(hostname, (const char *)(data + pos + 3), len);
(hostname)[len] = '\0';
return len;
if (is_valid_utf8(hostname)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a check we had before that got lost when we re-wrote the TLS parsing.
We've seen some instances where the hostname is not UTF-8. The JNI interface (is_domain_blocked) uses NewStringUTF to pass the domain name to JVM, this is just to ensure we never get there with non UTF string.

Copy link

@ngoossens ngoossens left a comment

Choose a reason for hiding this comment

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

I have read the code and run the tests (with some logging enabled) and this change looks good to me. I haven't built the DDG app and tested the whole thing together so approving with that caveat.

@aitorvs aitorvs marked this pull request as ready for review November 20, 2023 21:00
@karlenDimla karlenDimla self-assigned this Nov 21, 2023
@aitorvs aitorvs merged commit 5147bd8 into main Nov 21, 2023
1 check passed
@aitorvs aitorvs deleted the feature/aitor/tls_fragmented branch November 21, 2023 10:06
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.

3 participants