CodeQL: Add mldsa/ declaration visibility check#1052
Conversation
c6a2786 to
084de54
Compare
CodeQL is GitHub's semantic code analysis engine: it builds a typed AST
database from a build trace, then answers queries written in QL (a
Datalog-flavoured language) against it.
This commit adds a custom query that flags any externally-linked
declaration under mldsa/ lacking the appropriate visibility annotation
(MLD_INTERNAL_API, MLD_EXTERNAL_API, MLD_API_QUALIFIER for functions;
MLD_INTERNAL_DATA_DECLARATION / MLD_INTERNAL_DATA_DEFINITION for
variables), catching new cross-TU symbols that miss the macro required
for single-CU builds. Assembly symbols (*_asm) and empty-CU placeholders
(*empty_cu_*) are excluded.
A scripts/ql driver wraps the CLI: it manages the CodeQL database
(auto-rebuilding when sources change), dispatches queries, and runs
query-specific post-processors that add colored output and pass/fail
logic. The mldsa-declarations post-processor is registered for the
visibility check; other queries get plain table output.
A codeql.yml workflow invoked from all.yml runs the check in CI.
The new query flagged two missing annotations:
mld_polyeta_unpack in poly_kl.c and mld_prepare_domain_separation_prefix
in sign.c lacked MLD_INTERNAL_API / MLD_EXTERNAL_API, respectively.
Both are fixed in this commit, too.
Licensing: CodeQL uses a GitHub-specific license and is marked 'unfree'
in nixpkgs. It is free for open-source analysis and GitHub code
scanning, covering our use case. The nix flake does not globally
allowlist the package; developers opt in via
NIXPKGS_ALLOW_UNFREE=1 nix develop --impure .#codeql
to avoid silent acceptance of the non-OSS license.
Signed-off-by: Hanno Becker <[email protected]>
| - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| - uses: ./.github/actions/setup-shell | ||
| with: | ||
| nix-shell: codeql |
There was a problem hiding this comment.
what's the benefit of installing codeql via nix in CI? Wouldn't github/codeql-action/init be the easier and less intrusive path?
There was a problem hiding this comment.
Or do we expect this to be very version sensitive? That's the only reason I can think of.
There was a problem hiding this comment.
I felt it's in keeping with our existing setup, and yes, more robust towards version changes. But I am OK either way.
mkannwischer
left a comment
There was a problem hiding this comment.
Thanks @hanno-becker. Thanks for looking into this and finding the two places where we were missing the markers.
I have some questions. If we plan to scale this, I think it makes sense to try to simplify this as much as possible.
There was a problem hiding this comment.
I'm not sure I see the value of this 640-line Python script.
What's the benefit over just changing the rules to actually throw errors and the workflow to
- uses: github/codeql-action/init@x
with:
languages: cpp
queries: ./test/codeql/mldsa-declarations.ql
One could still keep the ql script for local testing, but that sounds more like 10 line bash script rather than a 640 line Python script.
There was a problem hiding this comment.
Can you elaborate? What are you suggesting?
There was a problem hiding this comment.
Thanks! If we can directly trigger an error from the .ql that may be easier indeed.
| devShells.cbmc = util.mkShell { | ||
| packages = builtins.attrValues { inherit (config.packages) cbmc toolchains_native; } ++ [ pkgs.gh ]; | ||
| }; | ||
| devShells.codeql = util.mkShell { |
There was a problem hiding this comment.
The following diff would allow us to install it unconditionally:
diff --git a/flake.nix b/flake.nix
index 3231ee183..83c7721d1 100644
--- a/flake.nix
+++ b/flake.nix
@@ -50,6 +50,8 @@
{
_module.args.pkgs = import inputs.nixpkgs {
inherit system;
+ config.allowUnfreePredicate = pkg:
+ builtins.elem (inputs.nixpkgs.lib.getName pkg) [ "codeql" ];
overlays = [
(_:_: {
clang_22 = pkgs-unstable.clang_22;
With an appropriate comment explaining the license, I believe that may be preferable?
There was a problem hiding this comment.
If we expect the developer to run this, this should be included in the default shell.
There was a problem hiding this comment.
I would like the non-free license to be explicit, so the lack of convenience in the current PR is by design.
There was a problem hiding this comment.
If we consider this non-free, then I prefer to not use it at all.
There seem to be free alternatives. Let me try that.
There was a problem hiding this comment.
If we consider this non-free, then I prefer to not use it at all.
No need to be undifferentiated here. The license terms cover our use case. But I don't want to expose a nix shell which hides from the user the fact that not all use cases are covered. For that reason I also don't want it in the default shell, which I want to continue to work as-is.
| # Pinned to the version shipped with the CodeQL CLI currently packaged in | ||
| # nixpkgs (2.23.3). Bump together with the CLI. |
There was a problem hiding this comment.
What happens if we forget to bump this? I would prefer leaving out this comment as this is guaranteed to be outdated soon.
CBMC Results (ML-DSA-44)Full Results (185 proofs)
|
CBMC Results (ML-DSA-65)Full Results (185 proofs)
|
CBMC Results (ML-DSA-87)Full Results (185 proofs)
|
|
Shelving for now, @mkannwischer is not convinced of the approach and will explore alternatives first. We can always come back to this. |
|
Reopening so we don't forget this exists. |
CodeQL is GitHub's semantic code analysis engine: it builds a typed AST database from a build trace, then answers queries written in QL (a Datalog-flavoured language) against it.
This commit adds a custom query that flags any externally-linked declaration under mldsa/ lacking the appropriate visibility annotation (MLD_INTERNAL_API, MLD_EXTERNAL_API, MLD_API_QUALIFIER for functions; MLD_INTERNAL_DATA_DECLARATION / MLD_INTERNAL_DATA_DEFINITION for variables), catching new cross-TU symbols that miss the macro required for single-CU builds. Assembly symbols (asm) and empty-CU placeholders (*empty_cu) are excluded.
A scripts/ql driver wraps the CLI: it manages the CodeQL database (auto-rebuilding when sources change), dispatches queries, and runs query-specific post-processors that add colored output and pass/fail logic. The mldsa-declarations post-processor is registered for the visibility check; other queries get plain table output.
A codeql.yml workflow invoked from all.yml runs the check in CI.
The new query flagged two missing annotations:
mld_polyeta_unpack in poly_kl.c and mld_prepare_domain_separation_prefix in sign.c lacked MLD_INTERNAL_API / MLD_EXTERNAL_API, respectively. Both are fixed in this commit, too.
Licensing: CodeQL uses a GitHub-specific license and is marked 'unfree' in nixpkgs. It is free for open-source analysis and GitHub code scanning, covering our use case. The nix flake does not globally allowlist the package; developers opt in via
NIXPKGS_ALLOW_UNFREE=1 nix develop --impure .#codeql
to avoid silent acceptance of the non-OSS license.