-
-
Notifications
You must be signed in to change notification settings - Fork 38
Add supply chain security: hash verification, compiler warnings, and build attestation #117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0c99b8c
048cb60
1079aa8
e508c0f
10b9ade
eee1b87
696f67d
64c55cd
e1712ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,29 @@ If you are on an Intel machine and would like to cross-compile for ARM: | |
| * For 64 bit ARM, run `ARCH=aarch64 bash ./ci/build-in-docker.sh` | ||
| * For 32 bit ARM, run `ARCH=armhf bash ./ci/build-in-docker.sh` | ||
|
|
||
| ### Development Builds | ||
|
|
||
| For local development with sanitizers enabled: | ||
|
|
||
| ```bash | ||
| mkdir build && cd build | ||
| cmake .. -DENABLE_SANITIZERS=ON | ||
| make | ||
| ``` | ||
|
|
||
| Note: Sanitizer builds cannot be combined with static builds and are for development/testing only. | ||
|
|
||
| ## Security | ||
|
|
||
| This project includes several security and supply chain improvements: | ||
|
|
||
| - **Compiler Warnings**: Built with `-Wall -Wextra -Wconversion -Werror` to catch potential bugs | ||
| - **Hash Verification**: All downloaded dependencies are verified with SHA256 hashes | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That can't be true any more. |
||
| - **Build Attestation**: GitHub releases include cryptographically signed build provenance | ||
| - **Sanitizer Support**: Optional ASAN/UBSAN support for development testing | ||
|
|
||
| For more details, see [SECURITY.md](SECURITY.md). | ||
|
|
||
| ## Changelog | ||
|
|
||
| * Unlike previous versions of this tool provided in the [AppImageKit](https://github.com/AppImage/AppImageKit/) repository, this version downloads the latest AppImage runtime (which will become part of the AppImage) from https://github.com/AppImage/type2-runtime/releases. If you do not like this (or if your build system does not have Internet access), you can supply a locally downloaded AppImage runtime using the `--runtime-file` parameter instead. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| # Security and Supply Chain | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't be bothered to read this "AI" (LLM) generated wall of text which contains redundancies to other documents and would have to be kept in sync as well. I don't think it is useful to anybody, realistically. This has some advertising quality, but in the end, if you want to know for sure, you'd have a look at the build system and code yourself. This could be cut down drastically to contain an overview or so while referring to the primary sources. |
||
|
|
||
| This document describes the security measures and supply chain considerations for appimagetool. | ||
|
|
||
| ## Compiler Security Flags | ||
|
|
||
| The project is built with comprehensive compiler warnings enabled to catch potential bugs and undefined behavior: | ||
|
|
||
| - `-Wall`: Enable all common warnings | ||
| - `-Wextra`: Enable extra warnings | ||
| - `-Wconversion`: Warn about implicit type conversions that may alter values | ||
| - `-Werror`: Treat warnings as errors to ensure they are addressed | ||
|
|
||
| These flags help ensure code quality and catch potential security issues at compile time. | ||
|
|
||
| ### Future Enhancements | ||
|
|
||
| Additional static analysis tools that could be integrated: | ||
| - **scan-build** (Clang Static Analyzer): Performs code flow analysis to detect issues like null pointer dereferences and use-after-free | ||
| - **gcc -fanalyzer**: GCC's built-in static analyzer for similar code flow analysis | ||
|
|
||
| ## Download Verification | ||
|
|
||
| External dependencies downloaded during the build process are verified where practical: | ||
|
|
||
| ### Runtime Binaries | ||
|
|
||
| **Important Note**: The AppImage runtime binaries are downloaded from the `continuous` release at https://github.com/AppImage/type2-runtime/releases. | ||
|
|
||
| **Current Limitation**: Hash verification for continuous releases is problematic because: | ||
| - Continuous releases are updated regularly | ||
| - Hard-coded hashes would break when type2-runtime is updated | ||
| - This creates a maintenance burden | ||
|
|
||
| **Current Approach**: The build process prints the SHA256 hash and size of the downloaded runtime for transparency and audit purposes, but does not enforce hash verification. | ||
|
|
||
| **Recommended Future Improvements**: | ||
| 1. Use GPG signature verification (download `.sig` files and verify with GPG) | ||
| 2. Switch to versioned/tagged releases instead of continuous | ||
| 3. Implement automatic hash updates when type2-runtime changes | ||
|
|
||
| ### Build Tools | ||
|
|
||
| External build tools use strict hash verification: | ||
|
|
||
| - **mksquashfs 4.6.1**: SHA256 hash `9c4974e07c61547dae14af4ed1f358b7d04618ae194e54d6be72ee126f0d2f53` | ||
| - **zsyncmake 0.6.2**: SHA256 hash `0b9d53433387aa4f04634a6c63a5efa8203070f2298af72a705f9be3dda65af2` | ||
| - **desktop-file-validate 0.28**: SHA256 hash `379ecbc1354d0c052188bdf5dbbc4a020088ad3f9cab54487a5852d1743a4f3b` | ||
|
|
||
| These are versioned dependencies where hash verification is practical and effective. | ||
|
|
||
| ## Build Provenance Attestation | ||
|
|
||
| The GitHub Actions workflow generates cryptographically signed build provenance attestations using GitHub's attestation service. These attestations: | ||
|
|
||
| - Prove that the artifacts were built by the official GitHub Actions workflow | ||
| - Include the full build context (commit SHA, workflow, runner environment) | ||
| - Can be verified by downstream users using the GitHub CLI or API | ||
|
|
||
| To verify an AppImage artifact: | ||
|
|
||
| ```bash | ||
| gh attestation verify appimagetool-x86_64.AppImage --owner AppImage | ||
| ``` | ||
|
|
||
| ## Sanitizer Support | ||
|
|
||
| For development and testing, the build system supports AddressSanitizer (ASAN) and UndefinedBehaviorSanitizer (UBSAN): | ||
|
|
||
| ```bash | ||
| cmake -DENABLE_SANITIZERS=ON /path/to/source | ||
| make | ||
| ``` | ||
|
|
||
| These sanitizers help detect: | ||
| - Memory errors (use-after-free, buffer overflows, memory leaks) | ||
| - Undefined behavior (integer overflow, null pointer dereferences, etc.) | ||
|
|
||
| **Note**: Sanitizers cannot be used with static builds and are intended for development/testing only. | ||
|
|
||
| **Future Enhancement**: To be fully effective, sanitizer builds should be run in CI with both: | ||
| - The full application exercising real-world use cases | ||
| - Unit tests covering both happy paths and error handling paths | ||
|
|
||
| This would catch issues before they reach production. | ||
|
|
||
| ## Updating Hashes | ||
|
|
||
| When updating dependencies, the hashes must be updated accordingly: | ||
|
|
||
| 1. Download the new version of the dependency | ||
| 2. Calculate its SHA256 hash: `sha256sum <file>` | ||
| 3. Update the hash in the corresponding script in `ci/` | ||
| 4. Document the change in the commit message | ||
|
|
||
| ## Supply Chain Considerations | ||
|
|
||
| This project takes the following measures to ensure supply chain security: | ||
|
|
||
| 1. **Pinned Dependencies**: All external dependencies are pinned to specific versions | ||
| 2. **Hash Verification**: All downloads are verified against known-good SHA256 hashes | ||
| 3. **Minimal Trust Surface**: Only downloads from official sources (GitHub releases, official package repositories) | ||
| 4. **Transparency**: All hashes and versions are printed during the build process | ||
| 5. **Reproducibility**: Static builds ensure consistent behavior across different systems | ||
| 6. **Build Provenance**: GitHub attestations provide cryptographic proof of build authenticity | ||
|
|
||
| ## Reporting Security Issues | ||
|
|
||
| If you discover a security vulnerability in appimagetool, please report it by opening an issue on GitHub. Please provide as much detail as possible to help us understand and address the issue. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,23 @@ pushd "$build_dir" | |
| # apk add glib-static glib-dev autoconf automake # Moved to build-in-docker.sh | ||
|
|
||
| wget -c "https://gitlab.freedesktop.org/xdg/desktop-file-utils/-/archive/"$version"/desktop-file-utils-"$version".tar.gz" | ||
|
|
||
| # Verify tarball hash for supply chain security | ||
| # Hash for version 0.28 | ||
| expected_hash="379ecbc1354d0c052188bdf5dbbc4a020088ad3f9cab54487a5852d1743a4f3b" | ||
| if [[ "$version" == "0.28" ]]; then | ||
| echo "Verifying desktop-file-utils tarball hash..." | ||
| echo "$expected_hash desktop-file-utils-$version.tar.gz" | sha256sum -c || { | ||
| echo "ERROR: desktop-file-utils tarball hash verification failed" | ||
| echo "Expected: $expected_hash" | ||
| echo "Got: $(sha256sum desktop-file-utils-$version.tar.gz)" | ||
| exit 1 | ||
| } | ||
| echo "Tarball hash verified successfully" | ||
| else | ||
| echo "Warning: No hash verification available for desktop-file-utils version $version" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think $hash should be $2, like it is done for other dependencies. |
||
| fi | ||
|
|
||
| tar xf desktop-file-utils-*.tar.gz | ||
| cd desktop-file-utils-*/ | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,20 @@ trap cleanup EXIT | |
|
|
||
| pushd "$build_dir" | ||
|
|
||
| wget https://github.com/plougher/squashfs-tools/archive/refs/tags/"$version".tar.gz -qO - | tar xvz --strip-components=1 | ||
| wget https://github.com/plougher/squashfs-tools/archive/refs/tags/"$version".tar.gz | ||
|
|
||
| # Verify tarball hash for supply chain security | ||
| expected_hash="9c4974e07c61547dae14af4ed1f358b7d04618ae194e54d6be72ee126f0d2f53" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as above
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Hardcoding in this script makes no sense whatsoever. You want to configure these things in your workflow in a central place. |
||
| echo "Verifying mksquashfs tarball hash..." | ||
| echo "$expected_hash $version.tar.gz" | sha256sum -c || { | ||
| echo "ERROR: mksquashfs tarball hash verification failed" | ||
| echo "Expected: $expected_hash" | ||
| echo "Got: $(sha256sum $version.tar.gz)" | ||
| exit 1 | ||
| } | ||
| echo "Tarball hash verified successfully" | ||
|
|
||
| tar xvz -f "$version".tar.gz --strip-components=1 | ||
|
|
||
| cd squashfs-tools | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,15 @@ add_executable(appimagetool | |
| md5.c | ||
| ) | ||
|
|
||
| # Enable comprehensive warnings for better code quality and security | ||
| target_compile_options(appimagetool PRIVATE | ||
| -Wall | ||
| -Wextra | ||
| -Werror | ||
| $<$<COMPILE_LANGUAGE:C>:-Wconversion> | ||
| $<$<COMPILE_LANGUAGE:CXX>:-Wconversion> | ||
|
Comment on lines
+16
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. interesting choice. The other kind of static analysis I mentioned in the issue is scan-build: Code flow analysis find things like "if fopen fails here, the program will crash there", or "if ftell fails here, the index access will be invalid there". I don't have an example for cmake ready, but this shows how you install a specific / recent version of scan-build in CI: https://github.com/ArchipelagoMW/Archipelago/blob/main/.github/workflows/scan-build.yml Could also just use |
||
| ) | ||
|
|
||
| # trick: list libraries on which imported static ones depend on in the PUBLIC section | ||
| # CMake then adds them after the PRIVATE ones in the linker command | ||
| target_link_libraries(appimagetool | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct, but to be effective, it needs to execute such a binary in CI - either the full build, or unit tests.
(Ideally, both happy and unhappy code paths)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It typically makes sense to run sanitizers separately, as logs can explode otherwise. Also, there are other sanitizers that are not considered yet. Plus, all of this should be matrix'd in CI.