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

Minor improvements #1418

Merged
merged 8 commits into from
Mar 23, 2022
Merged

Minor improvements #1418

merged 8 commits into from
Mar 23, 2022

Conversation

jpstotz
Copy link
Collaborator

@jpstotz jpstotz commented Mar 22, 2022

A bunch of minor improvements to:

  • Preferences dialog -> left side got larger and larger (larger than my screen height) while the right side had plenty of space -> moved plugins group to right side and made sure the plugins list is initialized before the first file is loaded.
  • Variable naming "getInstance" special handling
  • Do not ask for project save if nothing had been changed (e.g. start Jadx do not load a project and directly close it).
  • A lot of minor improvements to the debugger especially for error handling and logging for the case where there are problems.

@skylot
Copy link
Owner

skylot commented Mar 22, 2022

@jpstotz looks good to me, but CodeQL found possible log injections and suggesting filtering input to allow only alphanumeric characters to pass:

if (input.matches("\\w*")) {

Or maybe we can just concat raw strings without using logger args pattern ({}).

@jpstotz
Copy link
Collaborator Author

jpstotz commented Mar 23, 2022

@skylot Sorry but I am still not sure how to react on this CodeQL error. Especially as the CodeQL error is not on testing that the input is non-ASCII but on replacing new lines for avoiding forged log lines.

First I thought of an early Aprils fool because using log injection as an "attack" seems to me not like something an attacker would ever consider. Especially as in my opinion it is the dedicated purpose of a log system to record received data in a way the developer can see the actual unmodified data.

Even considering that log files could be processed by an automatic system later and based on certain log entries trigger actions (a scenario not really relevant for jadx but let's just consider it). In such a case in my opinion it is the job of the logging system to save the data in a way that makes injections impossible (e.g. save the data not in plain text files but in JSON, XML or directly in a database). So the only risk I see is a potential DOS attack by injecting large data into the log file consuming storage space.

Checking the linked OWASP page mentions possible attacks:

  1. Injection of new/bogus log events (log forging via log injection)
  2. Injection of XSS attacks, hoping that the malicious log event is viewed in a vulnerable web application
  3. Injection of commands that parsers (like PHP parsers) could execute

In my opinion 2. and 3. are outside of an web application not really relevant and anyway it would be a vulnerability of the log viewer application (log files are user defined arbitrary and potential insecure data by definition). So it comes back to 1. injection of log messages an possible "attack" in deed but so useless that it is not relevant in my opinion besides for script kiddies who what to say "helle I was here".

@skylot
Copy link
Owner

skylot commented Mar 23, 2022

@jpstotz well, I agree that this vulnerability is not really related to jadx-gui, but due to the latest issues with log4j, I want to correctly handle such warnings. Mostly to get used to correct code practices and prevent possible issues in other parts of jadx. For example, in jadx-core such issues should be handled very carefully because it can be used as library in any kind of software.

So I will add simple log escape utility method in jadx-core with suggested checks. It is not hard, but will allow making first step to prevent future issues.

@jpstotz
Copy link
Collaborator Author

jpstotz commented Mar 23, 2022

@skylot OK, the LogUtils seem to be a good approach but please don't use vulnerable input as alternative message because this can frighten users and is actually not true. I would rather call it "non-ASCII". Or even better just replace non-matching characters by . or ?. That would still allow to read the text content.

@skylot
Copy link
Owner

skylot commented Mar 23, 2022

Looks like CodeQL not too smart. After applying suggested fix, it is still throwing warnings 😢
Anyway, I think we have done enough, so I'm merging this PR.

@skylot skylot merged commit 909cf0a into skylot:master Mar 23, 2022
@jpstotz jpstotz deleted the variableNaming branch March 23, 2022 15:17
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.

None yet

2 participants