sal0max / currencies

An exchange rates currency converter for Android
GNU General Public License v3.0
227 stars 23 forks source link

Cryptographic APIs misuses #19

Closed misterAnderson90 closed 2 years ago

misterAnderson90 commented 2 years ago

I'm a PhD student interested in finding security vulnerabilities in open source projects.

We found a total of 5 warnings (indicating potential vulnerabilities) when running the CogniCrypt static analyzer (*) on currencies (or its library dependencies). We documented each one of these issues in private gists for the sake of confidentiality (non-disclosure).

Can you please let us know whether we can share these gists with you? We are eager to evaluate the perception of developers (e.g. severity of these warnings) and improve currencies's security, and the quality of the reports of static analysis tools. (*) https://github.com/CROSSINGTUD/CryptoAnalysis

sal0max commented 2 years ago

Sure, just send me the links and I'll have a look. Email is on my profile page.

sal0max commented 2 years ago

Still waiting, @misterAnderson90 ...

sal0max commented 2 years ago

Just checked my spam filter. There it was, all along. 🙈 Thanks for the mail.

Will have a look into it.

sal0max commented 2 years ago

Alright, here's the gists:

  1. https://gist.github.com/misterAnderson90/5384ef894051b997bf3fe26925e1ae4d
  2. https://gist.github.com/misterAnderson90/e67849b965aef456d8279e222ed40a3d
  3. https://gist.github.com/misterAnderson90/490e7d4e2eb6f8e3deb1526861bf8c03
  4. https://gist.github.com/misterAnderson90/162aa1de47b9b1cd63c09f284cb724d0
  5. https://gist.github.com/misterAnderson90/500cb2180ca5a595f8a84000c85febc1

All evolve around this code snipped in the third-party-library kittinunf/fuel.

var socketFactory: SSLSocketFactory by readWriteLazy {
    keystore?.let {
        val trustFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm())
        trustFactory.init(it)
        val sslContext = SSLContext.getInstance("SSL")
        sslContext.init(null, trustFactory.trustManagers, null)
        sslContext.socketFactory
    } ?: HttpsURLConnection.getDefaultSSLSocketFactory()
}
sal0max commented 2 years ago

Here a summary of the single issues:

  1. sslContext.init(null, trustFactory.trustManagers, null) "Second parameter was not properly generated as generated Trust Managers."

  2. val sslContext = SSLContext.getInstance("SSL") "First parameter (with value \SSL) should be any of {TLSv1.2, TLSv1.3}."

  3. sslContext.init(null, trustFactory.trustManagers, null) "First parameter was not properly generated as generated Key Managers."

  4. sslContext.init(null, trustFactory.trustManagers, null) "Third parameter was not properly generated as randomized."

  5. trustFactory.init(it) where it is var keystore: KeyStore? = null "First parameter was not properly generated as generated Key Store."

sal0max commented 2 years ago

Now for my conclusion, @misterAnderson90 .

This should be no problem for my app at all, as all issued code is inside the default initialization of the socketFactory variable.

You could open an issue in kittinunf/fuel, if you think, the default values used in that code block are a security risk.

misterAnderson90 commented 2 years ago

Dear sal0max,

Thanks a lot for your contribution. Based on your responses, these might be false positives and do not cause vulnerabilities to your Application. If necessary, I get in touch with you later. I'll open an issue on kittinunf/fuel to get their perceptions on these warnings.

Best regards.