Due to some urgent travel I was not able to update the Week_8 report, this report summarize what I did in both week.
Any latest code will be found in this repository : GitHub - akshajrawat/plugins-test · GitHub
What I did :
Firstly, I focused on running the codeql rule test on all of the 30 plugins listed in Joplin Plugins website. The tests was done etirely locally so that I can dedicate all of my empty cpu cores to run the codeql rules which reduce the scanning time up to 60%.
27 out of 30 plugins passed with 0 findings. Following are the plugins that gave few findings :
The rules were updated respectively so that these plugins also do not give any result. With this the current set of rules if ran on the top 30 plugins will generate no noise.
Then I focused on the pr related to plugin-repo-cli and generator-joplin, updated the code tested them using a dedicated published test package which is a clone of updated plugin-repo-cli and raised 2 prs :
I will be updating the GitHub - joplin/plugins-test · GitHub repository with all of the code shortly since some changes are underway right now, after doing the necessary changes and make sure everything works, I'll update the repository.
The rules were updated respectively so that these plugins also do not give any result. With this the current set of rules if ran on the top 30 plugins will generate no noise.
Please provide a link to the rules that were added along with the tests.
Also there should have been a report on 31 July but there wasn't?
My college started last week so the only thing I did was the pr work, there was not much to add so I though I'll add 2 week report together, Im currently reviewing all the Ts and yaml code in : GitHub - joplin/plugins-test · GitHub, I'll give a report for both week tommorow.
For the update done for the rest of the 10 plugins, no test were updated, only few rules were made tighter and some built in rules were excluded :
Rule 07b – Hardcoded command execution : Previously, any hardcoded string passed to a command-execution sink could be reported. It now ignores: Command-line flags and Common utilities such as git, cp, mv, tar, zip, rsync, pandoc, and ffmpeg.
Rule 18 – Mass data destruction : It now suppresses the finding when the same function contains an explicit Joplin confirmation dialog using joplin.views.dialogs.open().
Rule 19 – Keylogging and surveillance : Generic UI message handlers are no longer treated as keylogging sources, like onMessage
note : Rule 18 & Rule 19 still need little more update to make them more precise on what it should flag, I'll be updating them next
The test were done locally so I added all 30 plugin in the regression test once and ran them : test: top 30 plugins · joplin/plugins-test@5ace573 · GitHub
Everything passed. The one error which is there is due to joplin-plugin-toggle-sidebars being inside the joplin/ repository, which would need a little extra peice of code to run it, though since all of the 20 plugin in the regression test is individually built I did not add that code there.
Though this commit is outdated now, I updated rules and tests and tightened and fixed them a little more in the last 2 weeks, these are the lastest commits where I did those changes :
rm is not in the excluded list.
This does not whitelist these commands globally, there are rules that still flag usage of such commands in certain situation, like if user-data from joplin pass to these command :
Rule 7b (which was edited in the commit) detects hardcoded string passed to os command apis like spawn or exec, earlier it used to detect any hardcoded string, after the commit few of the commands were excluded if they are present alone like :
spawn('unknown', ['--force']);
unknow will be reported --force will be skiped
this will only work for standalone strings not for :
exec('unknown --force');
in this case whole unknown --force will be reported for review.
This is different from what you mentioned earlier since you modified it since we discussed it.
The rule excludes cp and mv which can still be used for maliscious purposes. What was the reason for excluding them? Please point me to the specific lines of the specific plugin(s) that these rules were added for. That should have been in a comment on the rule actually - we need to document exclusions.
Ah yes the explaination differ because I explained the older part of the rule done in the older commit. I thought you were talking about the older commit because it was explicitly for 7b rule
Rule 7b reports the hardcoded command ("cp"), rather than producing separate findings for the argument list ["-r", src, dst]. Due to findings on the test on plugins the later fix was to exclude non-shell (which reduces shell-injection risk) cp and mv.
Direct cp and mv calls might still be used maliciously though other rules catch some cases like :
Rule 02: sensitive settings such as passwords or tokens reach it.
Rule 05c: clipboard contents reach its command or arguments.
Rule 06: downloaded/remote data or cryptomining data reaches it.
Rule 10/10b: it runs from a registered Joplin export callback.
Rule 12: data from userDataGet() reaches command execution.
It contains 2-3 days of work. I updated the tests, ran the CodeQL tests, compared the findings with the intended positive and negative cases, and adjusted the rules when expected cases were missed.
The .expected files are generated automatically by the CodeQL test runner and record the findings produced by test.
I used Codex to implement the changes and cross checking the results, then reviewed the generated code and test results myself.
Ok so that's the Backup Plugin. Now let's say in the future there's an equally useful and legitimate plugin that uses rm, wget and rsync. What would be your approach to handle this plugin?
For such plugins, It should be better to let the scanner flag those per scan and the reviewer do a quick check before accepting the plugin since static analysis cannot in most of the cases determine if a usecase is legitimate or not.
For cases where some sensitive data for which rules are written is reaching rm, wget and rsync, there are other rules to explicitly flag them.
If that specific usecase need to be whitelisted instead of reviewing it every time, we can approve that safe usage once by updating the rules to ignore that specific path. It will then only report if the command or its arguments change.
I've tried to decipher your answer with Claude but it wasn't much more successful than me, and offered me up to three different readings for certain parts.
Everything is vague - "other rules to explicitly flag them" - which ones? how? "ignore that specific path" - a path to what? what does that even mean?
Then "static analysis cannot in most of the cases determine if a usecase is legitimate or not" - how is that even possible? Why do we even have a scanner then?
Here it seems to me that we need to have exceptions per plugin and possibly for specific lines in specific files. What you are saying, based on your current code, is that we need to keep adding global exceptions for commands every time a legitimate plugin uses them. We can't have that - that would weaken the system over time and indeed make it useless. So for example we can have exceptions for cp and rm but they need to be scoped to the Backup Plugin.
What I would expect here are suggestions on how we could manage and maintain per-plugin/file/line exceptions. That would be up to the reviewer to decide what should be added as an exception but there has to be a UI or process to add them.
By “other rules,” I meant Rule 7, which detects user or Joplin data reaching command execution, and Rules 10/10b, which detect exported data reaching child processes, network calls, or unsafe filesystem operations.
I have implemented plugin-scoped approvals:
The scanner reports all findings.
The reviewer checks them and adds status: approved only if every finding is legitimate.
Approved findings are stored in .github/codeql/approved-findings/<plugin-id>.json
Future scans show unchanged findings under Approved Earlier.
Changed commands, arguments, files, or data flows require review again.
This was for update scans where the previous scan result would have been shown as already approved so that reviewer does not have to check those results again and again :
Therefore, approving cp or rm for the Backup Plugin approves only those exact findings for that plugin and does not weaken the rules globally.
Does this address the per-plugin exception?
