Allow to sanitize spec file content before parsing with RPM#519
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant security enhancement by implementing a sanitization mechanism for RPM spec file content. It aims to neutralize potentially dangerous shell expansions and impure Lua macros before the spec file is parsed by RPM, thereby safeguarding against arbitrary code execution and other vulnerabilities. The changes provide a configurable option within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to introduce a robust sanitization feature for spec files by adding a new Sanitizer class to handle potentially unsafe shell expansions and Lua macros. However, the Sanitizer class itself contains critical security vulnerabilities, including a Lua injection flaw that could lead to Remote Code Execution (RCE), a high-severity bypass of the unsafe Lua macro check, and a medium-severity macro injection vulnerability. These issues must be addressed to ensure the intended security benefits. Additionally, there are suggestions to improve the maintainability and clarity of the new sanitizer.py file.
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 17s |
|
/gemini review |
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 04s |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable security feature by adding a Sanitizer class for RPM spec file content. While the intention is to remove potentially unsafe constructs, the current implementation has critical flaws that allow for bypasses, potentially leading to Remote Code Execution (RCE). Specifically, unsafe Lua macros and directives like %include can be smuggled through conditional macro expansions or nested shell expansions, and the Lua blacklist is incomplete, bypassable with functions like loadstring. Beyond these critical security concerns, the review also identified recurring issues in regular expressions, suggested improvements for the main sanitizer function's structure for better maintainability, and noted a correction needed for a unit test assertion. Addressing the security bypasses through recursive sanitization and strengthened macro type checks is highly recommended.
|
/gemini review |
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 05s |
There was a problem hiding this comment.
Code Review
The PR introduces a sanitization feature for spec files, aiming to replace unsafe shell expansions and Lua macros with safer equivalents, thereby improving security when parsing untrusted spec files. However, the sanitizer implementation has critical security flaws, including bypasses and Remote Code Execution (RCE) due to improper handling of nested shell expansions and an incomplete/bypassable Lua macro denylist. Additionally, there are potential Python version compatibility issues in sanitizer.py and an incorrect unit test. These critical security flaws must be addressed before this feature can be safely used.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable sanitization feature to enhance security when parsing RPM spec files by converting potentially unsafe shell expansions and Lua macros into safer equivalents. However, the Sanitizer class itself contains critical Lua injection vulnerabilities in the convert_glob_removal function due to insufficient escaping of user-supplied patterns, and the is_lua_safe check is bypassable due to incomplete regular expressions. These security flaws significantly undermine the intended benefits. Furthermore, there are critical issues with the handling of chained sed commands, which generates invalid nested Lua expressions, and significant bugs in the translation of sed regular expression patterns and replacement strings to their Lua counterparts.
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 06s |
lbarcziova
left a comment
lbarcziova
left a comment
There was a problem hiding this comment.
the overall design looks good to me - clean API and solid test coverage. I don't have more to add on the technicalities than what Gemini already flagged.
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 05s |
|
/gemini review |
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 04s |
There was a problem hiding this comment.
Code Review
This pull request introduces a new sanitization mechanism to prevent unsafe shell expansions and Lua macros in spec files. However, the implementation of the Lua safety check is fundamentally flawed and can be easily bypassed using standard Lua syntax. Furthermore, the sanitizer fails to recursively process macros within shell expansions, allowing for macro injection that can lead to arbitrary code execution, posing a critical Remote Code Execution (RCE) risk. Additionally, there are suggestions for improving the implementation, including a bug fix for sed substitution handling and performance with chained commands, refactoring the long sanitize_shell_expansion method, and minor style improvements for regular expression definitions.
|
/gemini review |
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 15s |
There was a problem hiding this comment.
Code Review
This pull request introduces a powerful and important feature for sanitizing spec file content by replacing potentially unsafe shell expansions and Lua macros. However, the implementation of the Sanitizer class contains critical security vulnerabilities that allow for Command Injection and Remote Code Execution (RCE). Specifically, sanitize_shell_expansion fails to validate several input components before embedding them into generated Lua code, leading to recursive macro expansion vulnerabilities, and is_lua_safe can be bypassed. Additionally, there is a suggestion for improvement in specfile/sanitizer.py to extend the support for the cut command's field selection syntax.
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to sanitize spec files, converting potentially unsafe constructs like shell expansions and impure Lua macros into safer equivalents. However, a critical vulnerability exists in the Lua sandbox implementation within is_lua_safe. This can be bypassed using string concatenation and aliasing, potentially leading to Remote Code Execution (RCE) when processing malicious spec files. Specifically, dangerous RPM macros can be constructed at runtime and expanded by the RPM engine. Apart from this issue, the implementation appears robust and well-executed.
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 13s |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant security enhancement by adding a sanitization feature for spec file content, utilizing a new Sanitizer class to convert potentially unsafe shell expansions and Lua macros. While the security review could not provide specific vulnerability feedback due to malformed input, the code review identified a minor bug in a regular expression and suggested refactoring for a large function to improve maintainability. The implementation is well-supported by a thorough test suite.
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 06s |
|
/gemini review |
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 10s |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust sanitizer for RPM spec files, aiming to prevent arbitrary code execution through shell expansions and unsafe Lua macros by converting common shell command patterns into safer RPM macros or Lua expressions. However, a critical Command Injection vulnerability has been identified in the Lua safety validation logic. The sanitizer strips Lua comments before identifying string literals, which allows an attacker to hide malicious code that will still be executed by the Lua interpreter, leading to arbitrary code execution. Additionally, there are limitations in the regex-based parsing of shell commands (sed, awk, tr) that could be improved, and one check in the Lua safety validation might be overly aggressive. It is recommended to address the Command Injection vulnerability by using a proper Lua lexer or ensuring string literals are handled before comment stripping.
Assisted-by: Claude Opus 4.6 via Cursor Signed-off-by: Nikola Forró <nforro@redhat.com> Co-authored-by: Laura Barcziová <49026743+lbarcziova@users.noreply.github.com>
Signed-off-by: Nikola Forró <nforro@redhat.com>
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 12s |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a sanitizer to process spec files and remove potentially unsafe constructs like shell expansions and impure Lua macros. This is a significant security enhancement. The implementation is comprehensive, covering a wide range of shell patterns and Lua safety checks. The accompanying tests are thorough and cover many edge cases and bypass attempts. I have a couple of suggestions to improve the maintainability of the new sanitizer module.
| _RE_BASH_ARRAY_FIELD = re.compile( | ||
| r"^(\w+)\s*=\s*[\"']?(%\{?\w+\}?)[\"']?\s*;\s*" | ||
| r"(\w+)=\(\$\{\1//(.)/ \}\)\s*;\s*" | ||
| r"echo\s+\$\{\3\[(\d+)\]\}\s*$" | ||
| ) |
There was a problem hiding this comment.
The regular expressions in this file are quite complex. To improve readability and maintainability, especially for security-critical code like this, consider using the re.VERBOSE flag. This would allow you to add comments to explain the different parts of the regex. For example, this regex could be rewritten as:
_RE_BASH_ARRAY_FIELD = re.compile(
r"""
^
(\w+)\s*=\s*["']?(%\{?\w+\}?)["']?\s*;\s* # var1=MACRO;
(\w+)=\(\$\{\1//(.)/ \}\)\s*;\s* # var2=(${var1//./ })
echo\s+\$\{\3\[(\d+)\]\}\s* # echo ${var2[idx]}
$
""",
re.VERBOSE,
)This would make it much easier to understand what the regex is matching at a glance. This can be applied to other complex regexes in this file as well.
|
|
||
| class Sanitizer: | ||
| @staticmethod | ||
| def sanitize_shell_expansion(body: str) -> str: |
There was a problem hiding this comment.
The sanitize_shell_expansion method is very long (around 400 lines) and contains many nested helper functions. To improve readability and maintainability, consider refactoring it. The nested helper functions like strip_quotes, normalize_macro, sed_pattern_to_lua, parse_sed_substs, parse_cut, etc., could be moved out of sanitize_shell_expansion and become top-level private functions within the module (e.g., _parse_cut). This would make sanitize_shell_expansion shorter and easier to follow, as it would primarily be a dispatcher that calls these helper functions based on which regex matches.
|
/packit test |
|
/packit-stg test |
|
/packit retest-failed |
|
/packit-stg retest-failed |
|
/packit retest-failed |
|
/packit-stg test |
|
regate |
|
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 2m 04s |
github-project-automation
Bot
moved this from In review
to Done
in Packit pull requests
Merge after packit/specfile#519.
Sanitize spec files Merge after packit/specfile#519. Reviewed-by: Laura Barcziová Reviewed-by: gemini-code-assist[bot]
3 participants
No description provided.