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-42: Config values lack overflow checks #1335

Closed
xgreenx opened this issue Aug 29, 2023 · 0 comments · Fixed by #1466
Closed

TOB-FUEL-42: Config values lack overflow checks #1335

xgreenx opened this issue Aug 29, 2023 · 0 comments · Fixed by #1466
Assignees
Labels
audit-report Somehow related to the audit report

Comments

@xgreenx
Copy link
Collaborator

xgreenx commented Aug 29, 2023

Description

The Fuel node has configurations for its chain and peer to peer that lack overflow checks on some parameters provided by node operators. Rather than allow a misconfigured node to continue to operate in the event of an overflow, invalid configurations should be validated upfront and rejected by using checked arithmetic.

Figure 42.1: Potential overflow in ChainConfig (audit-fuel/fuel-core/crates/chain-config/src/config/chain.rs#74–78)

impl Default for ChainConfig {
    fn default() -> Self {
        Self {
            chain_name: "local".into(),
            block_gas_limit: ConsensusParameters::DEFAULT.max_gas_per_tx * 10,

Figure 42.2: Potential overflow in P2P configuration (audit-fuel/fuel-core/crates/services/p2p/src/p2p_service.rs#154–171)

pub fn new(config: Config, codec: Codec) -> Self {
    let local_peer_id = PeerId::from(config.keypair.public());
    let gossipsub_data =
        GossipsubData::with_topics(GossipsubTopics::new(&config.network_name));
    let network_metadata = NetworkMetadata { gossipsub_data };
    // configure and build P2P Service
    let (transport, connection_state) = build_transport(&config);
    let behaviour = FuelBehaviour::new(&config, codec.clone());
    let total_connections = {
        // Reserved nodes do not count against the configured peer input/output
limits.
};
    let total_peers =
        config.max_peers_connected + config.reserved_nodes.len() as u32;
    total_peers * config.max_connections_per_peer

Exploit Scenario

A node operator uses invalid configurations that cause an overflow. Rather than receiving an error that these configurations are invalid, the node continues to run with unanticipated behavior.

Recommendations

Short term, use checked arithmetic and validate that configurations are within reasonable bounds.
Long term, consider setting Clippy’s integer arithmetic lint to deny and use checked, saturating, or wrapping arimhetic explicitly (same as #1334).

@xgreenx xgreenx added the audit-report Somehow related to the audit report label Aug 29, 2023
xgreenx added a commit that referenced this issue Nov 1, 2023
Closes #1335

Handling overflows during arithmetic operations by denying
`clippy::arithmetic_side_effects`.

Also, it is the last issue that we plan to fix right now in the scope of
the ToB audit report. So this PR closes
FuelLabs/fuel-vm#513.

---------

Co-authored-by: Brandon Kite <brandonkite92@gmail.com>
@xgreenx xgreenx self-assigned this Nov 8, 2023
crypto523 added a commit to crypto523/fuel-core that referenced this issue Oct 7, 2024
Closes FuelLabs/fuel-core#1335

Handling overflows during arithmetic operations by denying
`clippy::arithmetic_side_effects`.

Also, it is the last issue that we plan to fix right now in the scope of
the ToB audit report. So this PR closes
FuelLabs/fuel-vm#513.

---------

Co-authored-by: Brandon Kite <brandonkite92@gmail.com>
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.

1 participant