Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[PR] Enable TLS 1.3 by default #1282

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

Open
thanhminhmr wants to merge 1 commit into QuantumBadger:master
base: master
Choose a base branch
Loading
from thanhminhmr:thanhminhmr-TLS1.3

Conversation

Copy link

@thanhminhmr thanhminhmr commented Mar 29, 2025

Fix #1278

@thanhminhmr thanhminhmr force-pushed the thanhminhmr-TLS1.3 branch 2 times, most recently from 99a8a3a to 110244b Compare April 3, 2025 05:20
@thanhminhmr thanhminhmr force-pushed the thanhminhmr-TLS1.3 branch 2 times, most recently from 80d5a00 to 81b7d85 Compare April 5, 2025 12:41
Copy link
Author

@QuantumBadger I fixed all CI problems that my new code introduces, but there are some problem came from updating target Android to 33 (A bunch of error when run gradle lint). This is a bit outside of my Android knowledge, so I hope that you can either point me to the right direction with those, or just ignore the dependency updates altogether. Thanks!

Copy link
Owner

Hi @thanhminhmr, thank you for the PR! I'm currently undecided whether it's a good idea to use Conscrypt in RedReader.

It would be a fundamental change to the security model -- previously we've relied on the system APIs for TLS (and other native operations like image/video decoding), with all the app's logic written in Java/Kotlin. As a result there's never been a security issue discovered in RedReader, and we've never had to release an update for security reasons.

Putting Conscrypt (and hence the native BoringSSL library) inside RedReader means that:

  • If there are vulnerabilities in Conscrypt, RedReader would be vulnerable
  • We have to release a new version of RedReader very quickly after Conscrypt gets updated, to ensure any security fixes get pushed to users

Conscrypt seems to get updated very infrequently -- it's only had one release in the last three years.

I can't find records of any BoringSSL vulnerabilities since 2018, which is either a good sign or a bad sign 😂 It's a large C/C++ project, with the notoriously vulnerable OpenSSL as the base, and frankly I'd be impressed if they had precisely zero vulnerabilities in seven years of development (when OpenSSL has had a vast number in the same time).

So, I'm not yet decided, and open to arguments either way!

Copy link
Owner

On the topic of the PR itself -- it would be good to remove all the dependency updates, as these are best handled separately (and I think there's another PR open for this already). It's possible to run Lint locally using ./gradlew lint, and an HTML report gets generated in build/reports/.

thanhminh-vht reacted with heart emoji

Copy link
Author

thanhminhmr commented May 1, 2025
edited
Loading

I kind of agree with your points. Introducing a security related library that are not that trustworthy is not a good thing to do. For now I think restrict OkHttp to TLSv1.2 and TLSv1.3 is a better way to do it.

There is a few nuance though:

  1. Android only supported TLSv1.3 natively in API level 29 (Android 10), so installing RedReader on older Android will still work, but without TLSv1.3. This is why I introduce Conscrypt in the first place: to support TLSv1.3 on older Android.
  2. This also prevent OkHttp from accessing any HTTP website (which by the previous configuration it can). I cannot think of any problem introducing by this, as HTTP-only sites are nearly gone for good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature Request] Enable TLS 1.3 by default

AltStyle によって変換されたページ (->オリジナル) /