SecurityGuard: An API to prevent malicious code execution

[Net-0]

Hello guys,

I work as a software developer in Brazil ( I apologize for my english xD ), and the product that I work with my team uses the BeanShell to execute simple Java Snippets, but we were doing security validations of the entire system, and we were minding about how to solve the security problem of malicious code executions, e.g. executing "System.exit(1)", it'd be a really problem to us. Then I read a lot about "eval" functionalities, e.g. the JavaScript eval function, and I understand that it's a concern that "evals" are unsecure when you don't know what are being executed, so I propose of creating an API that solve this problem ( at least to Java :P ), and I called it SecurityGuard.

"Ok, ok, but how does it work?", the idea is really simple, the SecurityGuard is just an interface with methods that validate if some specific thing can be executed, e.g. the interface has a method called canInvokeStaticMethod that you can implement to validate the variables to call a method and return a boolean saying if it can be executed or not. E.g. of a code implementation of SecurityGuard that prevent the execution of "System.exit(1)":

"And after that?", then you just need execute "Interpreter.mainSecurityGuard.add(mySecurityGuard)", to add your guard to prevent the execution of some dangerous code for your application.

"What occurs when my SecurityGuard says that something can't be executed?", then the Interpreter just throws a SecurityError ( it's a new error that extends UtilEvalError, and is used at the same way in the code )

"And it impacts performance?", my team did this question when I present the POC of this idea, and the answer is quite simple: it dependes of how much SecurityGuards the Interpreter has and the validation of each of them, but with a small amount of SecurityGuards that aren't over complex, it'll be a small performance change. E.g. with 50 simple SecurityGuards, the performance impact is about 2%, I can redo the tests and send you the results if you wanna :).

Disclaimers

• Exists an implementation of SecurityGuard called BasicSecurityGuard, that just prevent that the code being executed by BeanShell add/remove SecurityGuards from the Interpreter.mainSecurityGuard, because it'd create another security problems xD.
• Reflection results at normal validations, e.g. if the code call some method using Reflection, the method canInvokeMethod is called yet!!
• I'd rather that the 'mainSecurityGuard' be for each instance of Interpreter, but in the v3 it's really hard to implement it ( at least to me :V ) because of how BeanShell works, and it'll also create a problem that the code being executed by BeanShell is able to create an instance of Interpreter to execute the malicious code, and there is an example saying that is possible to create an instance of Interpreter in BeanShell page, so it's a hard way to do it.
• I'm a Jr. Software Developer yet, and it's my first contribution at open source project, so any feedback is welcome ;D
• My team didn't use SecurityGuard, this solution is powerful and amazing to our case, but my tech leader said that we mustn't maintain a local fork of an entire project just because we need some small changes in the source code, so if this fork be approved, we should use it, otherwise no :P, meanwhile we're using another validation that isn't as precise but is better than nothing.

[nickl-]

Sounds interesting.

You will need to roll this against fresh HEAD, we had to do some maintenance on the repo at #750, I believe your changes were done on a previous copy which is why it shows so many changes.

Please redo the PR with only the changes for the SecurityGuard for a proper revue.

Apologies for the inconvenience!

[nickl-]

May relate to #645

[Net-0]

Hello, sorry for being late xD, I just saw your comment yesterday.

I rebased my fork's branch with the original master, and it seems ok now ;D

[Net-0]

Hello @nickl- , does the pull request have some issue 😬 ? It wasn't approved yet...

[nickl-]

Hello @nickl- , does the pull request have some issue 😬 ? It wasn't approved yet...

Been swamped, will make some time to look into this, thank you for nudging me.

One note which I thought about, not based on code review, does this provide a means to disable BeanShell commands?

https://github.com/beanshell/beanshell/tree/master/src/main/resources/bsh/commands

This may be a security risk as well.

[Net-0]

Hello nick,

No, it doesn't have a mean to prevent commands, to be honest, I guessed that it didn't need it xD, but reading the all available commands, I think that it can be interesting for some specific cases, I'm going to do a implementation for this purpose :).

[Net-0]

Hello bro,

Now the SecurityGuard have a mean to prevent the execution of local methods/commands :)

Good Night ✌️

[nickl-]

Awesome!!

We have a public holiday on Thursday, will have time then to give my full attention.

[nickl-]

Please also see Scrutinizer exceptions to rectify.

[nickl-]

Implementation otherwise looks good and this will be a very useful addition. I should have time again Monday or Tuesday if this also works for you.

[Net-0]

Please also see Scrutinizer exceptions to rectify.

Abou this, I fixed the code style issues of Scrutinizer.

The tests results was really weird, but it was a mistake in the SecurityGuardTest that I implemented, and it was even weird because it works in my PC xD. But basically there was a test that execute clear(), and it removed the assert(boolean) local method, and it cause some problems at some another tests 😅.

If there is some issue yet, I can see it in the next weekend probably, Good Night ✌️

[nickl-]

Please also see Scrutinizer exceptions to rectify.

Abou this, I fixed the code style issues of Scrutinizer.

The tests results was really weird, but it was a mistake in the SecurityGuardTest that I implemented, and it was even weird because it works in my PC xD. But basically there was a test that execute clear(), and it removed the assert(boolean) local method, and it cause some problems at some another tests 😅.

If there is some issue yet, I can see it in the next weekend probably, Good Night ✌️

Yes these things can be a pain, thank you.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 70.18349% with 65 lines in your changes are missing coverage. Please review.

Project coverage is 74.26%. Comparing base (2ffb804) to head (0ed20ed). Report is 10 commits behind head on master.

:exclamation: Current head 0ed20ed differs from pull request most recent head 91b5b4c. Consider uploading reports for the commit 91b5b4c to get more accurate results

Files	Patch %	Lines
src/main/java/bsh/MainSecurityGuard.java	62.13%	33 Missing and 6 partials :warning:
src/main/java/bsh/SecurityError.java	71.11%	12 Missing and 1 partial :warning:
src/main/java/bsh/BSHPrimarySuffix.java	73.91%	6 Missing :warning:
src/main/java/bsh/SecurityGuard.java	66.66%	3 Missing :warning:
src/main/java/bsh/BshMethod.java	60.00%	2 Missing :warning:
src/main/java/bsh/Name.java	85.71%	1 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #751 +/- ## ============================================ + Coverage 74.24% 74.26% +0.02% - Complexity 3042 3114 +72 ============================================ Files 108 112 +4 Lines 9357 9572 +215 Branches 1857 1880 +23 ============================================ + Hits 6947 7109 +162 - Misses 2070 2115 +45 - Partials 340 348 +8 ``` | [Flag](https://app.codecov.io/gh/beanshell/beanshell/pull/751/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=beanshell) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/beanshell/beanshell/pull/751/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=beanshell) | `74.26% <70.18%> (+0.02%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=beanshell#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[nickl-]

Implementation looks good, but we still need better code coverage. Especially for the new classes, please.

[Net-0]

Hello @nickl- , I'm improving the code coverage, I really forget to do some tests 😅.

I also make some code improvements while doing the new tests for SecurityGuardTest:

• Unwrap the Primitive values in MainSecurityGuard before give them to the SecurityGuard, to the logic validate the "pure" value.
• Reflection to get the length of an Array will also trigger the SecurityGuard#canGetField

I'm going to see the other tests tomorrow, good night ✌️

[Net-0]

I guess that I finished the tests, I made all missed tests for the implementation of SecurityGuard, but I ignored the files that have missed tests that the code wasn't implemented in this PR.

Can I run the codecov to see the code coverage or only you can ? I didn't find this option.... 🫠

After all, I guess that now everything looks good to me, but if it's not 😬, I probably can see any issues in the next weekend.

Good afternoon ✌️.

[nickl-]

Can I run the codecov to see the code coverage or only you can ? I didn't find this option.... 🫠

There is a code coverage in the maven build script too, although it is not the same as codecov, which is part of the automated build on github. It should run automatically when you push to this branch, that is the idea anyway, unless something went wrong with it.

[Net-0]

Hello,

I know that I'm going to say it again, but now I guess that everything is fine :D

About the changes, I basically make an adjust in the bsh.BSHTypedVariableDeclaration because when you declare a class field, it was getting the field value even though it being useless ( because the return value after evaluating a script when the last statement is a class declaration is the class itself ) and thus it was triggering the SecurityGuard to validate if can get that field. And I also used the jacoco that is in the maven scrips that you recommended, then I improved the tests and code coverage until I achieved the 100%.

But I guess that there is something weird, because I guess that codecov isn't updating:

codecov now:

jacoco results:

Good night and weekend ✌️, see you in the next weekend.