bcgit / bc-java

Bouncy Castle Java Distribution (Mirror)
https://www.bouncycastle.org/java.html
MIT License
2.28k stars 1.13k forks source link

TrustAllX509TrustManager - lint error #1306

Closed daniel-tailored closed 7 months ago

daniel-tailored commented 1 year ago

Hi, I write concerning below lint errors we got for our android project.

Ofc adding to baseline would resolve the issue but that's not really the best approach I guess (implementation could change in the future etc.). Any suggestions or awareness for this already?

Thx in advance.

<issue
    id="TrustAllX509TrustManager"
    message="`checkClientTrusted` is empty, which could cause insecure network traffic due to trusting arbitrary TLS/SSL certificates presented by peers">
    <location
        file="org/bouncycastle/jsse/BCX509ExtendedTrustManager.class"/>
</issue>

<issue
    id="TrustAllX509TrustManager"
    message="`checkClientTrusted` is empty, which could cause insecure network traffic due to trusting arbitrary TLS/SSL certificates presented by peers">
    <location
        file="org/bouncycastle/jsse/BCX509ExtendedTrustManager.class"/>
</issue>

<issue
    id="TrustAllX509TrustManager"
    message="`checkServerTrusted` is empty, which could cause insecure network traffic due to trusting arbitrary TLS/SSL certificates presented by peers">
    <location
        file="org/bouncycastle/jsse/BCX509ExtendedTrustManager.class"/>
</issue>

<issue
    id="TrustAllX509TrustManager"
    message="`checkServerTrusted` is empty, which could cause insecure network traffic due to trusting arbitrary TLS/SSL certificates presented by peers">
    <location
        file="org/bouncycastle/jsse/BCX509ExtendedTrustManager.class"/>
</issue>

<issue
    id="TrustAllX509TrustManager"
    message="`checkClientTrusted` is empty, which could cause insecure network traffic due to trusting arbitrary TLS/SSL certificates presented by peers">
    <location
        file="org/bouncycastle/est/jcajce/JcaJceUtils$1.class"/>
</issue>

<issue
    id="TrustAllX509TrustManager"
    message="`checkServerTrusted` is empty, which could cause insecure network traffic due to trusting arbitrary TLS/SSL certificates presented by peers">
    <location
        file="org/bouncycastle/est/jcajce/JcaJceUtils$1.class"/>
</issue>

<issue
    id="TrustAllX509TrustManager"
    message="`checkClientTrusted` is empty, which could cause insecure network traffic due to trusting arbitrary TLS/SSL certificates presented by peers">
    <location
        file="org/bouncycastle/est/jcajce/JcaJceUtils$2.class"/>
</issue>
francescocervone commented 1 year ago

This lint issue is pretty serious. Any Android app containing unsafe implementations of X509TrustManager may be removed from the Play Store.

https://support.google.com/faqs/answer/6346016?hl=en

dghgit commented 1 year ago

Yes, I'd say it's serious, BCX509ExtendedTrustManager is an abstract class, there's obviously a bug in the lint checker. Is there a directive that turns it off for a specific method?

So with JcaJceUtils - section 4.1.1 of RFC 7030 actually requires support for connecting to an unknown server as part of it's boot strapping process. These things shouldn't ever have to evaluate client auth though - we've added illegal state exceptions for the client auth side.

daniel-tailored commented 1 year ago

@dghgit

Yes, I'd say it's serious, BCX509ExtendedTrustManager is an abstract class, there's obviously a bug in the lint checker. Is there a directive that turns it off for a specific method?

I'm not aware of any, no.

So with JcaJceUtils - section 4.1.1 of RFC 7030 actually requires support for connecting to an unknown server as part of it's boot strapping process. These things shouldn't ever have to evaluate client auth though - we've added illegal state exceptions for the client auth side.

So with that added do you not get the lint error any more? Can you reproduce it when running lint? If so, fixed, it would then probably be in the next release?

Thx for the reply and looking into it!

daniel-tailored commented 1 year ago

Also: with that change, as you mentioned, checkServerTrusted at least for getTrustAllTrustManager would still be empty. This needs to be according to RFC 7030 4.1.1, right? Anything that can be done to meet the security requirements (from google), to not have that?

change your code in the checkServerTrusted method of your custom X509TrustManager interface to raise .. exceptions .. whenever the certificate presented by the server does not meet your expectations. https://support.google.com/faqs/answer/6346016?hl=en

ATM there is no way to use this dependency without the custom TrustManager implementations, even though they are not used.. for example: https://github.com/appmattus/certificatetransparency/issues/18

Maybe that could also offer a way to resolve this for a library like the above from appmattus.

skylerreimer commented 1 year ago

Hey there. My team's app is also dealing with this. Is there any update here?

dghgit commented 1 year ago

I'm not aware of anyone fixing the Android lint checker as yet.

fvermeulen commented 11 months ago

Hey any news on this?

daniel-tailored commented 11 months ago

As for the certificate transparency library from app mattus, they removed the dependency.. therefore resolving the issue. https://github.com/appmattus/certificatetransparency/issues/50

artyomdeynega commented 7 months ago

Any update?

dghgit commented 7 months ago

Apparently it's now possible to add:

@SuppressLint("TrustAllX509TrustManager")

to the source to disable this check.

I'd also note that:

https://github.com/plaid/plaid-link-android/issues/231

provides other directions for how to suppress the error.