Skip to content
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

Add org.microg.tools.CondLog() for conditional logging (for #251) #804

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Nanolx
Copy link
Contributor

@Nanolx Nanolx commented Jun 2, 2019

Add new CondLog() [Conditional Logging] function to play-services-core. It only prints the Log if the desired Log.Level isLoggable(). Took some inspiration from this gist.

Also includes a test commit within AuthManagerServiceImpl.

@mar-v-in let me know if you think this is suitable for #251

cc-ing @ale5000-git @Lanchon

@Nanolx Nanolx changed the title Add org.microg.tools.CondLog() for conditional logging (fix #251) Add org.microg.tools.CondLog() for conditional logging (for #251) Jun 2, 2019
@ale5000-git
Copy link
Member

ale5000-git commented Jun 3, 2019

Very good idea, but I propose 2 DEBUG levels:

  • DEBUG (standard with all messages)
  • DEBUG_SAFE same as DEBUG but without auth info (that may compromise account if published online)

It could be set at compile time if there isn't any other way.

@Nanolx
Copy link
Contributor Author

Nanolx commented Jun 3, 2019

microG has different log levels already, so we could either do more fine granulatio/re-classification or check whether we're on a debug build, instead of a relase build with something like:

diff --git a/play-services-core/src/main/java/org/microg/tools/CondLog.java b/play-services-core/src/main/java/org/microg/tools/CondLog.java
index 36c0cb5..be20cbb 100644
--- a/play-services-core/src/main/java/org/microg/tools/CondLog.java
+++ b/play-services-core/src/main/java/org/microg/tools/CondLog.java
@@ -42,7 +42,11 @@ public class CondLog
 			tag = tag.substring(0, 22);
 		}
 
-		return Log.isLoggable(tag, level);
+		if (! BuildConfig.DEBUG && level = Log.DEBUG) {
+			return false
+		} else {
+			return Log.isLoggable(tag, level);
+		}
 	}
 
 	/**

@ale5000-git
Copy link
Member

ale5000-git commented Jun 3, 2019

There still can be the case when someone on a debug build need to post the log publicly (to help debugging).

So I think we need a more fine granulation/reclassification of log messages (at least the ones containing sensitive informations or session ids that may allow session hijacking).

@Nanolx
Copy link
Contributor Author

Nanolx commented Jun 3, 2019

See the last commit if it more fits your expectation, I've simply added DEBUG_SAFE (as CondLog.DEBUG_SAFE) as a new, custom log level, which equals to 42 (because it's the meaning of life).

I've not hijacked the logger itself, because catching DEBUG_SAFE and then piping through Log.d() (or not) is enough from my point of view.

Since official log levels are in the range between 2 - 7 that shouldn't cause any issue in the foreseeable future.

Additionally isLoggable() will always return false for DEBUG_SAFE if we're on a release build.

@Lanchon
Copy link

Lanchon commented Jun 3, 2019

logs should never log sensitive information. this applies to every system. i shouldnt be exposed to risks if i choose DEBUG logs; this is a bug. i shouldnt need to know that a DEBUG_SAFE level exists not to be in danger!

DEBUG should be safe, and if needed, a DEBUG_WITH_SENSITIVE_INFO or something similar could be added. (DEBUG_UNSAFE is not a good name; it doesnt protect users from phishing attacks from others posing as giving help.)

more correctly, an independent LOG_SENSITIVE_INFO setting could be added which triggers logging of unsafe stuff at the proper level which probably shouldnt be DEBUG. a WARNING entry should be logged whenever this LOG_SENSITIVE_INFO mode is enabled. even if a DEBUG_WITH_SENSITIVE_INFO level is implemented, it should log a warning when selected. security is a serious thing.

additionally, if DEBUG currently produces sensitive output, then id STRONGLY suggest removing that level completely. maybe replace it with VERBOSE or SAFE_DEBUG or something similar.

the rationale would be avoiding this scenario where someone honest asks for a DEBUG log and is sent a tainted DEBUG from an old version of microg.

@Nanolx
Copy link
Contributor Author

Nanolx commented Jun 3, 2019 via email

@voidstarstar
Copy link
Contributor

If there's going to be 2 logging functions for each logging level, I agree with @Lanchon that the one that includes sensitive data should contain additional verbosity. In other words, we should use DEBUG_SENSITIVE_DATA and DEBUG instead of DEBUG_SAFE and DEBUG. We should assume that anything unsafe will be explicitly (and verbosely) marked.

I would also suggest phrasing this with a preference of "sensitive data" over "sensitive info" to avoid confusion with the INFO logging level.

A global flag, off by default, should be as verbose and obvious as possible. Probably a good idea to put the words "DANGEROUS" and/or "I_KNOW_WHAT_IM_DOING" in there for good measure.

additionally, if DEBUG currently produces sensitive output, then id STRONGLY suggest removing that level completely. maybe replace it with VERBOSE or SAFE_DEBUG or something similar.

the rationale would be avoiding this scenario where someone honest asks for a DEBUG log and is sent a tainted DEBUG from an old version of microg.

Even if we change future versions of microG to not use DEBUG, how will that prevent the old versions from using it? This seems like a pointless change unless we somehow have all users always change their default logging levels before they paste a logcat, which seems unreasonable to me. We could have only affected users change their logging level, but at that point, we might as well just tell users to upgrade to the latest microG before posting logcats (we shouldn't care about fixing bugs in old versions of microG anyway).

@Nanolx
Copy link
Contributor Author

Nanolx commented Jun 3, 2019

I've renamed DEBUG_SAFE to DEBUG_SENSITIVE to be more clear what's going on.

Before going further we should wait for what @mar-v-in has to say regarding the logging.

@ale5000-git
Copy link
Member

ale5000-git commented Jun 4, 2019

@voidstarstar: We must always tell users to upgrade to the latest microG before posting logcat otherwise we will have a lot of issues for already fixed bugs.

@Nanolx: It seems nice but how can be switched on/off DEBUG_SENSITIVE in debug builds?

@Nanolx
Copy link
Contributor Author

Nanolx commented Jun 4, 2019

@ale5000-git in the current state you can't, because it's always enabled in debug builds and always disabled in release builds. It would be possible to either

a) add an optional compile time variable (override-able from local.properties)
b) add a system property (like microg.debug.sensitive using something like this reflection)

@ale5000-git
Copy link
Member

ale5000-git commented Jun 4, 2019

My opinion is that for sensitive info it is safer a compile time variable, also it would be nice to avoid calling Log.isLoggable for DEBUG_SENSITIVE on DEBUG builds.

@ale5000-git
Copy link
Member

ale5000-git commented Jun 4, 2019

Like this:

if (level == DEBUG_SENSITIVE) {
	return BuildConfig.DEBUG && DEBUG_SENSITIVE_IS_ENABLED;
} else {
	return Log.isLoggable(tag, level);
}

@Nanolx
Copy link
Contributor Author

Nanolx commented Jun 4, 2019

Yes, that's probably the best if we go with a).

@ale5000-git ale5000-git requested a review from mar-v-in June 4, 2019 20:34
@Nanolx
Copy link
Contributor Author

Nanolx commented Sep 15, 2020

@mar-v-in any interest in this? If not, I'm going to close it.

@mar-v-in
Copy link
Member

I personally prefer a system that by default removes sensitive information from log or replaces it with stars but keeps the log entries. And allows users to enable logging the sensitive information when needed. However, I'm not sure how to easily realize such system without significant additional burden during development.

@mar-v-in mar-v-in force-pushed the master branch 4 times, most recently from ae17355 to 8078073 Compare April 20, 2023 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants