scripts: add secure plo scripts with signature verification#257
scripts: add secure plo scripts with signature verification#257winderdoot wants to merge 2 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary of ChangesHello @winderdoot, 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 comprehensive framework for generating and verifying secure Phoenix-RTOS Loader (PLO) scripts. The primary goal is to establish a robust root of trust by allowing PLO scripts and their associated binaries (kernel, applications) to be cryptographically signed. This ensures the integrity and authenticity of the boot process, which is a critical requirement for secure boot implementations on embedded targets. Highlights
Changelog
Activity
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 introduces support for asymmetrically signed PLO user scripts, enhancing the root of trust with new 'secure' kernel, app, and call commands, pycryptodome integration, and new signing scripts like sign_file.py. While the image_builder.py refactoring and dynamic command discovery in PloCmdFactory improve maintainability, a security audit has revealed critical vulnerabilities. Specifically, there's a critical Server-Side Template Injection (SSTI) leading to Remote Code Execution, high-severity Path Traversal vulnerabilities allowing arbitrary file reads, and medium-severity issues with insecure private key password handling on the command line. Additionally, a potential bug in the script signing logic, minor improvements for exception handling, and a duplicate import require attention.
| key_path = Path(self.public_key) | ||
| self.public_key = read_pem_pub_key(key_path) |
There was a problem hiding this comment.
In the PloCmdKernelSecure class, the public_key field, which is read from the input YAML file, is used to construct a file path without proper sanitization. An attacker can provide a malicious path (e.g., ../../../../etc/passwd) in the YAML configuration, causing the application to attempt to read an arbitrary file from the filesystem. This can lead to information disclosure about file existence and permissions.
There was a problem hiding this comment.
I believe these security concerns don't really make sense, but the current shape of the command line arguments should be approved by someone.
| key_path = Path(self.public_key) | ||
| self.public_key = read_pem_pub_key(key_path) |
There was a problem hiding this comment.
In the PloCmdCallSecure class, the public_key field, which is read from the input YAML file, is used to construct a file path without proper sanitization. An attacker can provide a malicious path (e.g., ../../../../etc/passwd) in the YAML configuration, causing the application to attempt to read an arbitrary file from the filesystem. This is the same vulnerability pattern as found in PloCmdKernelSecure.
|
|
||
| # opt-in to secure user plo scripts by providing private-key | ||
| partition.add_argument("--private-key", type=Path, default=None, dest="script_privkey", help="Private ECC key path for creating a signed, secure PLO script") | ||
| partition.add_argument("--passwd", type=str, default=None, dest="privkey_pass", help="Password for encrypted private key") |
There was a problem hiding this comment.
The private key password is provided as a command-line argument via --passwd. Command-line arguments can be exposed in the system's process list and shell history, making them visible to other users on the same machine. This can lead to the leakage of the private key password.
| parser = argparse.ArgumentParser(description="Sign an ELF file (ECC only)") | ||
| parser.add_argument("-i", "--input", type=Path, required=True, help="Path to input ELF file") | ||
| parser.add_argument("--private-key", type=Path, required=True, help="Path to private ECC key") | ||
| parser.add_argument("--passwd", type=str, required=True, help="Password for encrypted private key") |
There was a problem hiding this comment.
The private key password is provided as a command-line argument via --passwd. Command-line arguments can be exposed in the system's process list and shell history, making them visible to other users on the same machine. This can lead to the leakage of the private key password.
497f63c to
4f82c84
Compare
4f82c84 to
348805c
Compare
|
Initial questions:
|
|
Regarding documentation we have:
And when it comes to rootfs targets: |
YT: RTOS-1084
348805c to
c5e0cb3
Compare
2 participants
Introduce changes to user scripts to allow making plo root of trust
Description
Motivation and Context
On targets where plo/plo scripts are used, signing them lets plo continue the root of trust.
Required for stmn32 secureboot (YT: RTOS-1084).
Types of changes
How Has This Been Tested?
Checklist:
Special treatment
PR for the plo implementation of secure commands isn't ready yet, an issue with a driver came up.