Azure python sdk url summary upstream #20563
Conversation
bdrodes
commented
Sep 30, 2025
bdrodes
commented
- Added MaD sinks for URLs in the azure SDK, labeled as 'ssrf'
- Updated HTTP request clients to use 'ssrf' MaD
There was a problem hiding this comment.
Pull Request Overview
This PR adds Server-Side Request Forgery (SSRF) modeling for the Azure Python SDK by introducing MaD (Models as Data) sinks and updating HTTP request clients to recognize SSRF vulnerabilities. This enables CodeQL to detect when user input can control URLs passed to Azure SDK methods.
- Added SSRF sink modeling framework for MaD-based HTTP requests
- Created comprehensive Azure SDK models for KeyVault and Storage services
- Added test coverage for Azure client SSRF scenarios
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| python/ql/lib/semmle/python/frameworks/SSRFSink.qll | New framework for handling SSRF sinks through MaD models |
| python/ql/lib/semmle/python/frameworks/Azure.Storage.model.yml | MaD definitions for Azure Storage SDK SSRF sinks |
| python/ql/lib/semmle/python/frameworks/Azure.Keyvault.model.yml | MaD definitions for Azure KeyVault SDK SSRF sinks |
| python/ql/lib/semmle/python/Frameworks.qll | Import statement for new SSRFSink framework |
| python/ql/test/query-tests/Security/CWE-918-ServerSideRequestForgery/test_azure_client.py | Test cases for Azure SDK SSRF detection |
| python/ql/test/query-tests/Security/CWE-918-ServerSideRequestForgery/PartialServerSideRequestForgery.expected | Expected results for partial SSRF tests |
| python/ql/test/query-tests/Security/CWE-918-ServerSideRequestForgery/FullServerSideRequestForgery.expected | Expected results for full SSRF tests |
| python/ql/lib/change-notes/released/2025-09-30-azure_ssrf_models | Release notes for the changes |
python/ql/test/query-tests/Security/CWE-918-ServerSideRequestForgery/test_azure_client.py
Outdated
Show resolved
Hide resolved
python/ql/test/query-tests/Security/CWE-918-ServerSideRequestForgery/test_azure_client.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…orgery/test_azure_client.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…orgery/test_azure_client.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
owen-mc
left a comment
owen-mc
left a comment
There was a problem hiding this comment.
I'm not familiar with the python library in general, but I can review the use of models-as-data.
|
Looking at the other test failures (which aren't related to validation of sink kinds):
|
…t cases expected results, off by one line. Changed to using ModelOutput::sinkNode.
Fixed. |
There was a problem hiding this comment.
This file doesn't really match the way the rest of the repo is structured. Normally this kind of code would go in python/ql/lib/semmle/python/security/dataflow/ServerSideRequestForgeryCustomizations.qll, but then it would only affect just the two SSRF queries. Since these models are principally adding to Http::Client::Request::Range, and only indirectly becoming SSRF sinks, it might make more sense to put them somewhere like python/ql/lib/semmle/python/Concepts.qll, maybe just below line 1658 import ConceptsShared::Http::Client as Client.
There was a problem hiding this comment.
(And I would name the class HttpClientRequestFromModel.)
There was a problem hiding this comment.
Indeed, this is my only remaining issue. There is also the philosophical question of whether all SSRF sinks really are client requests. If we had MaD for concepts, we could do this the other way round: Make all these models be client requests (with appropriate frameworks and certificate validation) and then they would be imported into the query automatically (and the query would decide that all client requests are SSRF sinks).
But for now, following the suggestion with HttpClientRequestFromModel would be ok.
There was a problem hiding this comment.
I've just pushed a change for this. The class was moved and renamed as suggested. I did remove it from a module though. Let me know how this looks.
Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>
…ientRequestFromModel as suggested in PR review.
3 participants
