[lcmangalagiri]

Detects user-controlled data flowing into deserialization functions (serde_json, bincode, rmp_serde, ciborium, serde_yaml, toml).

Query include:

1. Extension library with sources, sinks, and barriers
2. Models-as-data sink definitions
3. Query help (.qhelp) with examples
4. Test cases with inline expectations

Query ID: rust/unsafe-deserialization

[geoffw0]

Hi, just a quick note that I do intend to review this soon, but I've got a bit delayed with other things (including the other new query pull request).

[lcmangalagiri]

Hi, just a quick note that I do intend to review this soon, but I've got a bit delayed with other things (including the other new query pull request).

Hi @geoffw0, no rush — just a friendly reminder since it's been a few days. Feel free to take a look at this one whenever you get a chance. Thanks!

[geoffw0]

just a friendly reminder since it's been a few days

Yep, I'm looking forward to getting round to this - I think it will be a good query. I am away next week though. Sorry for the delays.

[github-actions]

QHelp previews:

rust/ql/src/queries/security/CWE-502/UnsafeDeserialization.qhelp

Deserialization of user-controlled data

Deserializing untrusted data without validation can allow an attacker to cause denial of service, consume excessive resources, or in some cases execute arbitrary code. Rust's memory safety mitigates some risks, but deserializing untrusted data with libraries likeserde, bincode, or rmp-serde can still lead to panics, excessive memory allocation, or logic bugs. This is especially true when trait objects or polymorphic types are involved.

Recommendation

Avoid deserializing untrusted data with formats that allow unbounded allocation or polymorphic dispatch. Where possible, prefer formats with schema validation, such as Protocol Buffers, when processing untrusted input. If you do use serde, consider:

• Validating input size before deserialization.
• Using #[serde(deny_unknown_fields)] to reject unexpected data.
• Avoiding #[typetag] or trait object deserialization with untrusted input.
• Using bounded containers (e.g., limiting Vec length via custom deserializers).

Example

In the following example, data from an HTTP request is directly deserialized without any validation. An attacker could send a crafted payload that causes excessive memory allocation or other unintended behavior.

use serde::Deserialize;

#[derive(Deserialize)]
struct UserData {
    name: String,
    items: Vec<String>,
}

fn handle_request(body: &[u8]) -> UserData {
    // BAD: deserializing user-controlled data without size validation
    serde_json::from_slice(body).unwrap()
}

A safer approach validates the input size and uses strict deserialization settings:

use serde::Deserialize;

const MAX_BODY_SIZE: usize = 1024 * 1024; // 1 MB limit

#[derive(Deserialize)]
#[serde(deny_unknown_fields)]
struct UserData {
    name: String,
    #[serde(deserialize_with = "bounded_vec")]
    items: Vec<String>,
}

fn bounded_vec<'de, D>(deserializer: D) -> Result<Vec<String>, D::Error>
where
    D: serde::Deserializer<'de>,
{
    let v = Vec::<String>::deserialize(deserializer)?;
    if v.len() > 100 {
        return Err(serde::de::Error::custom("too many items"));
    }
    Ok(v)
}

fn handle_request(body: &[u8]) -> Result<UserData, String> {
    // GOOD: validate input size before deserialization, use bounded containers
    if body.len() > MAX_BODY_SIZE {
        return Err("payload too large".to_string());
    }
    serde_json::from_slice(body).map_err(|e| e.to_string())
}

References

• OWASP: Deserialization of untrusted data.
• Common Weakness Enumeration: CWE-502.

[geoffw0]

Its right, though to be honest I slightly prefer your alert message.

[geoffw0]

This looks pretty good and we'll be glad to have an unsafe deserialization query in the Rust query suite! ✨

In the long run we may want to add barriers to this query so that we don't report spurious results when there are checks on the untrusted data before it gets deserialized. I don't see a lot of noisy results in the wild so I think it will be OK to merge without that feature.

[geoffw0]

Hmm, I tried out the query locally and it looks like we aren't finding the results in the test (despite the tags suggesting that we are). I'm looking into what's gone wrong...

[geoffw0]

Hi @lcmangalagiri , are you planning to update this PR? I made some minor suggestions above and I think there are issues with some of the models-as-data models (.yml extensions). Do you want to fix the issues yourself, should I make further code suggestions, or would you rather I open a new PR for the query so I can control the edits?

[lcmangalagiri]

Hi @geoffw0, thanks for the review and the follow-up! I've pushed the fixes in bccfc94.

• Fixed the models-as-data paths: removed the incorrect crate:: prefixes and used the canonical defining-module paths for the re-exported deserialization functions.
• Added explicit handling for the string payload of serde_json::Value::String at from_value sinks. The expanded regression test caught this additional missing flow.
• Added regression coverage for all 13 modeled deserialization APIs, plus both the bad and good query-help examples.
• Marked the size-validated case and the good query-help example as SPURIOUS, since validation barriers remain out of scope for this PR, as discussed.
• Updated the OWASP link and removed the manually duplicated CWE-502 reference. I kept the existing alert message, per your preference.
• Generated the missing .expected baseline with the CodeQL test runner.

Validation: the targeted CWE-502 CodeQL test passes without --learn, all 16 expected alerts are present, and cargo check passes (only unused-field warnings in the fixtures). Query-help XML and whitespace checks also pass.

Would you mind taking another look when you have a chance? Thanks!

[geoffw0]

Thanks for the changes, I'm happy with the code now. 👍

I've restarted CI + DCA + am doing a MRVA run too (i.e. three different levels of testing).

[geoffw0]

I've created a pull request lcmangalagiri#1 onto this one, containing minor changes that should permit CI to pass. @lcmangalagiri if you're happy with that change, please merge that and the commits should show up here, and I'll start another CI run.

I'm happy with what I saw on DCA and MRVA, and I'm itching to merge this!

Docs team, this PR has been bouncing back and forth for a while and I don't want to delay it any further. If you see this after the PR is merged, please review it anyway and I will be very happy to address your comments and suggestions as follow-up.

[mchammer01]

@lcmangalagiri 👋🏻 - Thanks for documenting this new query ✨
Made a few suggestions to bring your updates more in line with our style guide and recommendations.
Appoving to unlock but it would be great it you were to consider the suggestions. Thank!

[lcmangalagiri]

Hi @geoffw0, thanks again for the fixes! I’ve merged your CI and sink-metrics changes in 2399460, without adding another change note, as you advised.

The latest workflows report action_required. Could you please approve/start CI for the updated head? GitHub also shows REVIEW_REQUIRED, so could you re-review and approve once the checks pass?

Thanks again for helping get this ready to merge!

[geoffw0]

Very happy with this, ready to merge as soon as CI passes.

Thank you @lcmangalagiri for your contribution to CodeQL analysis, and for your persistence addressing the various concerns, suggestions and CI failures to get this merged. ✨ 🎉 ✨

[lcmangalagiri]

Thank you, @geoffw0! I really appreciate your thoughtful reviews, patience, and help with the CI fixes. Your guidance made this contribution stronger. Glad to contribute to CodeQL, and looking forward for many more in the future.