Cwa-app-android: Do not log in production

[IndianaDschones]

Current Implementation

The release build contains Log-statements of all levels.

Suggested Enhancement

A production build should not contain any Log-statements, see for example the Android Prepare for release guide.

Log statements can be removed using ProGuard rules.

Expected Benefits

• Compliance to Androids suggestions for release builds
• Security: logs can contain sensitive data which should never be logged
• Behaves like the iOS application will do, see corona-warn-app/cwa-app-ios#22

[IndianaDschones]

Limitations of this fix

While this fix is quick and easy, it unfortunately isn´t the _silver bullet_ to protect from unwanted information disclosure.

A simple Log statement like Log.d("Foo") will reliable be removed by Proguard.

Now assume this: Log.d("Foo value is" + secretValue) will be compiled to Log.d(new StringBuilder().append("Foo value is").append(secretValue).toString())

Proguard will simplify it to new StringBuilder().append("Foo value is").append(secretValue).toString() because the code is to complex for Proguard. It ain´t safe to be removed by Proguard. You can verify this behavior by decompiling the app.

The secret value will show up in memory, which can be accessed using a debugger or memory dumping.

Possible mitigations

Feel free to file a new issue (or pr) based on your projects preferences. Each solution has its pros and cons.

Never log sensitive data

Never use a Log-statement containing sensitive data.

Remove sensitive log statements using compiler

By writing if (BuildConfig.DEBUG) in front of a log containging sensitive data, the compiler will strip this completely.

Own logger accepting only simple values

By using a own logger (wrapping android log) and only using simple arguments, Proguard manages to remove them.

LogHelper.d("Foo value is ", secretValue)

The Proguard rules need to be adjusted by replacing -assumenosideeffects class android.util.Log with -assumenosideeffects class de.rki.coronawarnapp.LogHelper

Source

More information on this topic can be found in this stackoverflow question

[jakobmoellersap]

I think our best case here is going through the code before release and manually replacing the Log statements with empty calls, If checks that get compiled away by if (false) or leaving them in for error reporting. Your proguard solution is working fine for simple Log statements, but we often use complex logging string concatentation that gets compiled by a StringBuilder() class as mentioned and thus most logic actually would not be drawn out.

Let me know what you think and if you agree, we can move forward! 👍

[IndianaDschones]

I think the most future-proof solution would be the usage of a _simple logger_ (third suggested solution) and using Proguard on it to reliable and automated remove those statements.
There is a logging-framework from JakeWharton called timber which could do the job.
So using Timber instead of Log and changing the Proguard rule to Timber should to the trick.

The timber solution should be easier than writing a Log-wrapper. But you have to check if it is okay to use this framework (license compliance and such stuff).

I already have a branch where i tested timber. If you want i can create a PR and you can take over the PR and adjust it to your needs.
Timber can only log "simple" types. You are logging more complex types, like
https://github.com/corona-warn-app/cwa-app-android/blob/360f5812243ddd3bf7f9c290065a16ea6cbdafe4/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/ExposureStateUpdateWorker.kt#L33-L35

You would have to check how to log it using timber (e.g. extracting the relevant things as string and log it).

[jakobmoellersap]

Logs are excluded from compilation starting from 0.8.15 using Timber. Thanks so much for your input on this! It shaped our decision quite a bit!