Conversation
…perator_and_operand_from_comparison for removing eval in check() in next commit
so the script doesn't crash on undefined tlm
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3143 +/- ##
==========================================
- Coverage 78.23% 78.22% -0.01%
==========================================
Files 673 673
Lines 55239 55321 +82
Branches 728 728
==========================================
+ Hits 43214 43275 +61
- Misses 11947 11968 +21
Partials 78 78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
ryan-pratt
force-pushed
the
bug/check-undefined-tlm
branch
from
20182f0 to
8e0cbc8
Compare
|
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| end | ||
| end | ||
|
|
||
| if eval_is_valid && eval(string) |
There was a problem hiding this comment.
Remote Code Execution possible via eval()-type functions - critical severity
Using functions such as eval can lead to users being able to run their own code on your servers.
Show fix
Remediation: If possible, avoid using these functions altogether. If not, use a list of allowed inputs that can feed into these functions.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| try: | ||
| if eval(string): | ||
| eval_is_valid = _check_eval_validity(value, comparison_to_eval) | ||
| if eval_is_valid and eval(string): |
There was a problem hiding this comment.
Unsafe eval usage can lead to remote code execution - critical severity
Using eval on expressions based on user input can execute arbitrary code.
Show fix
Remediation: Consider using ast.literal_eval as an alternative. If that is not possible, replace the usage with a safer alternative that strictly parses the expected input format.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| raise CheckError(message) | ||
| try: | ||
| if eval(string): | ||
| if eval_is_valid and eval(string): |
There was a problem hiding this comment.
Unsafe eval usage can lead to remote code execution - critical severity
Using eval on expressions based on user input can execute arbitrary code.
Show fix
Remediation: Consider using ast.literal_eval as an alternative. If that is not possible, replace the usage with a safer alternative that strictly parses the expected input format.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
1 participant
This is a potentially breaking change if users were abusing the
check()API.Before, if you booted fresh COSMOS with the demo and wrote a script with
check(EXAMPLE STATUS VALUE > 1), then the script would crash (even in disconnect mode) because that tlm value is nil/None, which can't be compared using < or >. This fixes that by making sure the eval can be called before calling it and logging an error more gracefully. I also added some other safeguards around the comparisons you can do.Also fixed a couple bugs in extract.py and added unit tests based on the ruby ones.
I gave up removing
eval()because the code paths just became too numerous to keep in my head and I was spending too much time on it; plus like @/ryanmelt mentioned in standup, this is in an executing script anyway. I'd like to remove those, but it's tech debt for another time imo.