Skip to content

Rust: fix CleartextLogging query #19139

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

Merged
merged 2 commits into from
Mar 28, 2025
Merged

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Mar 27, 2025

This pull request adds argument 1 of log::__private_api::log as a log injection sink.

Line 42: debug!("message = {}", password);
expands to:

// Recursive expansion of debug! macro
// ====================================

{
    {
        let lvl = (log::Level::Debug);
        if lvl <= log::STATIC_MAX_LEVEL && lvl <= log::max_level() {
            log::__private_api::log(
                ({ log::__private_api::GlobalLogger }),
                log::__private_api::format_args!("message = {}", password),
                lvl,
                &(
                    (log::__private_api::module_path!()),
                    log::__private_api::module_path!(),
                    log::__private_api::loc(),
                ),
                (),
            );
        }
    }
}

Most likely this commit is responsible for the sudden test failure. That commit is part of the log crate since version 0.4.27.

An alternative "fix" would be to pin version of the log the dependency in the test case, but that's not a great solution. It's probably a good thing that we are notified by changes that break our tests.

qltest_dependencies:
    - log = { version = "= 0.4.26", features = ["kv"] }
    - simple_logger = { version = "5.0.0" }

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Mar 27, 2025
@aibaars aibaars force-pushed the aibaars/rust-clear-text-logging branch from 9f8fe0c to 7b7a85e Compare March 27, 2025 18:32
@aibaars aibaars force-pushed the aibaars/rust-clear-text-logging branch from 7b7a85e to 7fc7b7c Compare March 27, 2025 18:39
@aibaars aibaars marked this pull request as ready for review March 27, 2025 18:49
@Copilot Copilot AI review requested due to automatic review settings March 27, 2025 18:49
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request addresses a break in the CleartextLogging query by updating the log injection sink mappings in the log model YAML file to support changes introduced in log version 0.4.27.

  • Updated the argument index mappings in log.model.yml for pre-0.4.27 compatibility.
  • Adjusted inline comments to reflect the new mapping of parameters.
Files not reviewed (2)
  • rust/ql/test/query-tests/security/CWE-020/RegexInjection.expected: Language not supported
  • rust/ql/test/query-tests/security/CWE-312/CleartextLogging.expected: Language not supported
Comments suppressed due to low confidence (1)

rust/ql/lib/codeql/rust/frameworks/log.model.yml:6

  • [nitpick] Consider clarifying the inline comment for this entry to explicitly state that it maps the logger parameter for pre-0.4.27 in order to improve maintainability and readability.
+      - ["repo:https://github.com/rust-lang/log:log", "crate::__private_api::log", "Argument[0]", "log-injection", "manual"] # logger / args (pre v0.4.27)

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for figuring this one!

@aibaars aibaars merged commit eceeab1 into main Mar 28, 2025
14 checks passed
@aibaars aibaars deleted the aibaars/rust-clear-text-logging branch March 28, 2025 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants