Skip to content
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

TOB-FUEL-21: Startup warnings are not logged due to wrong import #1327

Closed
xgreenx opened this issue Aug 28, 2023 · 1 comment · Fixed by #1383
Closed

TOB-FUEL-21: Startup warnings are not logged due to wrong import #1327

xgreenx opened this issue Aug 28, 2023 · 1 comment · Fixed by #1383
Assignees
Labels
audit-report Somehow related to the audit report

Comments

@xgreenx
Copy link
Collaborator

xgreenx commented Aug 28, 2023

Description

The fuel-core node imports logging macros (like info!, warn!, or error!) from the log facade instead from the tracing. Messages logged through the log facade do not get printed to the terminal and get ignored silently. Both facades are provided by the respective dependencies tracing and log.
The following code highlights a wrong import.

Figure 21.1: Import from the log facade instead of tracing. (fuel-core/bin/fuel-core/src/cli/run.rs#47–51)

use tracing::{
    info,
    log::warn,
    trace, 
};

The wrong import is used in the following figure to print a warning.

Figure 21.2: Usage of wrong import. (fuel-core/bin/fuel-core/src/cli/run.rs#260–262)

if consensus_key.is_some() && trigger == Trigger::Never {
    warn!("Consensus key configured but block production is disabled!")
}

Exploit Scenario

An attacker controls the configuration and is able to enable debugging options. Because relevant warnings during startup are not logged it is not possible to audit the node.

Recommendations

Short term, make sure that the log facade is not used.
Long term, enforce that the log facade is not used by using a clippy lint. Alternatively, configure the log facade to output messages to the terminal as well. The formatting of these messages could indicate an additional warning that the wrong facade is used.

@xgreenx xgreenx added the audit-report Somehow related to the audit report label Aug 28, 2023
@xgreenx
Copy link
Collaborator Author

xgreenx commented Aug 28, 2023

Accidentally fixed in #1257, but still it is better to add lint rule to avoid that in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-report Somehow related to the audit report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants