C++: Add flow sources from Windows' http.h#21619
C++: Add flow sources from Windows' http.h#21619MathiasVP wants to merge 8 commits intogithub:mainfrom
http.h#21619Conversation
MathiasVP
force-pushed
the
more-http-remote-flow-sources
branch
from
2b6ee7a to
ab34bd2
Compare
There was a problem hiding this comment.
Pull request overview
Adds new C++ Models-as-Data remote sources for the Windows HTTP Server API (http.h) receive functions and extends taint propagation into relevant HTTP_REQUEST-related fields, with accompanying library-test coverage.
Changes:
- Add
HttpReceiveHttpRequest,HttpReceiveRequestEntityBody, andHttpReceiveClientCertificateas remote sources inWindows.model.yml. - Introduce a new C++ modeling implementation module (
implementations/Http.qll) and hook it into the global models import list. - Extend the external-models library test (
windows.cpp) and update expected results (sources.expected,flow.expected) accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cpp/ql/test/library-tests/dataflow/external-models/windows.cpp | Adds test scaffolding for http.h structs/APIs and asserts expected remote flow from new sources into nested fields. |
| cpp/ql/test/library-tests/dataflow/external-models/sources.expected | Updates expected source locations to include the three new HTTP Server API sources. |
| cpp/ql/test/library-tests/dataflow/external-models/flow.expected | Updates expected flow graph output to reflect the new models and additional propagation. |
| cpp/ql/lib/semmle/code/cpp/models/Models.qll | Registers the new HTTP modeling implementation module. |
| cpp/ql/lib/semmle/code/cpp/models/implementations/Http.qll | Adds field-content taint propagation support for key http.h request/cert structures. |
| cpp/ql/lib/ext/Windows.model.yml | Declares the three new http.h receive functions as remote sourceModel entries. |
| cpp/ql/lib/change-notes/2026-03-31-http-flow-sources.md | Documents the addition of the new remote flow sources. |
| private import semmle.code.cpp.dataflow.new.DataFlow | ||
|
|
||
| private class HttpRequest extends Class { | ||
| HttpRequest() { this.hasGlobalName("_HTTP_REQUEST") } |
There was a problem hiding this comment.
Which version should I be looking at? V1 or V2?
There was a problem hiding this comment.
Ah, doesn't really matter, as V2 extends V1.
There was a problem hiding this comment.
Yeah, sorry about that! I meant to model V1 and just leave V2 for the future, but it seems I somehow messed up my naming in the process. dc8dc61 fixes it so that we explicitly call this V1.
| this.getIndirectionIndex() = 1 | ||
| or | ||
| this.getAField().hasName("pUnknownHeaders") and | ||
| this.getIndirectionIndex() = 1 |
There was a problem hiding this comment.
Why do these have an indirection of 1?
There was a problem hiding this comment.
Good question! These should indeed have an indirection of 2. However, I'm confused why the tests pass with an indirection of 1 🤔 I'll investigate!
There was a problem hiding this comment.
Two bugs uncovered:
- The indirection was indeed wrong. I've fixed in 16a7e39. We need the indirection to be 2 for
pUnknownHeadersbecause it's a pointer to a struct (i.e., aHTTP_UNKNOWN_HEADER*), but forKnownHeadersthe indirection should be 1 because it's an array (i.e., aHTTP_KNOWN_HEADER[]). - The reason this works right now is because of unintended pointer/pointee conflation in one of the default taint-tracking rules. I'll open a PR to fix that separately.
…t because of a conflation bug in taint-tracking.
| @@ -54,7 +54,7 @@ private class HttpRequestHeadersInheritingContent extends TaintInheritingContent | |||
| this.getIndirectionIndex() = 1 | |||
There was a problem hiding this comment.
Should this one also be updated?
There was a problem hiding this comment.
You're too fast for me 😅 The TLDR is 'no'. I've explained why here
This PR adds three new sources of remote flow. In order to get the required flow I also added object->field taint-flow from to some of the relevant fields.