amplitude / Amplitude-Android

Native Android SDK for Amplitude
MIT License
164 stars 90 forks source link

Usage of weak crypto algorithms like MD5 in Amplitude-Android SDK #314

Closed nidhi88 closed 1 year ago

nidhi88 commented 2 years ago

Summary

Our Penetration testing team has identified usage of weak crypto algorithms like MD5 in Amplitude-Android SDK and logged security vulnerability. What are the plans to migrate to the latest crypto algorithms? Can you please migrate to the latest crypto algorithms to mitigate this?

Recommendation: Utilize cryptographic hashing algorithms that are considered secure and advocated for in best practice recommendations. Guidance can be found for Android For more guidance on best practices in picking strong cryptography, please see OWASP's Cryptographic Storage Cheat Sheet.

Motivations

Security Vulnerability.

nidhi88 commented 1 year ago

Hi @qingzhuozhen Can you please check this issue?

below function from AmplitudeClient in package com.amplitude.api uses MD5 which is considered a weak crypto algorigthm.

protected void makeEventUploadPostRequest(OkHttpClient client, String events, final long maxEventId, final long maxIdentifyId) {
        String apiVersionString = "" + Constants.API_VERSION;
        String timestampString = "" + getCurrentTimeMillis();

        String checksumString = "";
        try {
            String preimage = apiVersionString + apiKey + events + timestampString;

            // MessageDigest.getInstance(String) is not threadsafe on Android.
            // See https://code.google.com/p/android/issues/detail?id=37937
            // Use MD5 implementation from http://org.rodage.com/pub/java/security/MD5.java
            // This implementation does not throw NoSuchAlgorithm exceptions.
            MessageDigest messageDigest = new MD5();
            checksumString = bytesToHexString(messageDigest.digest(preimage.getBytes("UTF-8")));
        } catch (UnsupportedEncodingException e) {
            // According to
            // http://stackoverflow.com/questions/5049524/is-java-utf-8-charset-exception-possible,
            // this will never be thrown
            logger.e(TAG, e.toString());
        }
qingzhuozhen commented 1 year ago

Hi @nidhi88, thanks for raising this. The MD5 is used for our checksum field, actually, you don't need to worry about the security concern for that. We switched to a different endpoint in https://github.com/amplitude/Amplitude-Kotlin, and that doesn't include the MD5 cypto algorithm.

nidhi88 commented 1 year ago

Thanks, @qingzhuozhen for the update.

justin-fiedler commented 1 year ago

MD5 usage has been removed in v2.39.6