-
Notifications
You must be signed in to change notification settings - Fork 2k
C++: Add flow sources from Windows' http.h
#21619
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
Changes from 6 commits
18a25c5
21ea7eb
c6d1ec5
102221d
9e97e04
ab34bd2
dc8dc61
16a7e39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * Added `HttpReceiveHttpRequest`, `HttpReceiveRequestEntityBody`, and `HttpReceiveClientCertificate` from Win32's `http.h` as remote flow sources. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,193 @@ | ||
| private import cpp | ||
| private import semmle.code.cpp.ir.dataflow.FlowSteps | ||
| private import semmle.code.cpp.dataflow.new.DataFlow | ||
|
|
||
| private class HttpRequest extends Class { | ||
| HttpRequest() { this.hasGlobalName("_HTTP_REQUEST") } | ||
| } | ||
|
|
||
| private class HttpRequestInheritingContent extends TaintInheritingContent, DataFlow::FieldContent { | ||
| HttpRequestInheritingContent() { | ||
| this.getAField().getDeclaringType() instanceof HttpRequest and | ||
| ( | ||
| this.getAField().hasName("pRawUrl") and | ||
| this.getIndirectionIndex() = 2 | ||
| or | ||
| this.getAField().hasName("CookedUrl") and | ||
| this.getIndirectionIndex() = 1 | ||
| or | ||
| this.getAField().hasName("Headers") and | ||
| this.getIndirectionIndex() = 1 | ||
| or | ||
| this.getAField().hasName("pEntityChunks") and | ||
| this.getIndirectionIndex() = 2 | ||
| or | ||
| this.getAField().hasName("pSslInfo") and | ||
| this.getIndirectionIndex() = 2 | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| private class HttpCookedUrl extends Class { | ||
| HttpCookedUrl() { this.hasGlobalName("_HTTP_COOKED_URL") } | ||
| } | ||
|
|
||
| private class HttpCookedUrlInheritingContent extends TaintInheritingContent, DataFlow::FieldContent { | ||
| HttpCookedUrlInheritingContent() { | ||
| this.getAField().getDeclaringType() instanceof HttpCookedUrl and | ||
| this.getAField().hasName(["pFullUrl", "pHost", "pAbsPath", "pQueryString"]) and | ||
| this.getIndirectionIndex() = 2 | ||
| } | ||
| } | ||
|
|
||
| private class HttpRequestHeaders extends Class { | ||
| HttpRequestHeaders() { this.hasGlobalName("_HTTP_REQUEST_HEADERS") } | ||
| } | ||
|
|
||
| private class HttpRequestHeadersInheritingContent extends TaintInheritingContent, | ||
| DataFlow::FieldContent | ||
| { | ||
| HttpRequestHeadersInheritingContent() { | ||
| this.getAField().getDeclaringType() instanceof HttpRequestHeaders and | ||
| ( | ||
| this.getAField().hasName("KnownHeaders") and | ||
| this.getIndirectionIndex() = 1 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this one also be updated?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're too fast for me 😅 The TLDR is 'no'. I've explained why here |
||
| or | ||
| this.getAField().hasName("pUnknownHeaders") and | ||
| this.getIndirectionIndex() = 1 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do these have an indirection of 1?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two bugs uncovered:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've got a PR with a fix for the conflation in draft here: #21622 |
||
| ) | ||
| } | ||
| } | ||
|
|
||
| private class HttpKnownHeader extends Class { | ||
| HttpKnownHeader() { this.hasGlobalName("_HTTP_KNOWN_HEADER") } | ||
| } | ||
|
|
||
| private class HttpKnownHeaderInheritingContent extends TaintInheritingContent, | ||
| DataFlow::FieldContent | ||
| { | ||
| HttpKnownHeaderInheritingContent() { | ||
| this.getAField().getDeclaringType() instanceof HttpKnownHeader and | ||
| this.getAField().hasName("pRawValue") and | ||
| this.getIndirectionIndex() = 2 | ||
| } | ||
| } | ||
|
|
||
| private class HttpUnknownHeader extends Class { | ||
| HttpUnknownHeader() { this.hasGlobalName("_HTTP_UNKNOWN_HEADER") } | ||
| } | ||
|
|
||
| private class HttpUnknownHeaderInheritingContent extends TaintInheritingContent, | ||
| DataFlow::FieldContent | ||
| { | ||
| HttpUnknownHeaderInheritingContent() { | ||
| this.getAField().getDeclaringType() instanceof HttpUnknownHeader and | ||
| this.getAField().hasName(["pName", "pRawValue"]) and | ||
| this.getIndirectionIndex() = 2 | ||
| } | ||
| } | ||
|
|
||
| private class HttpDataChunk extends Class { | ||
| HttpDataChunk() { this.hasGlobalName("_HTTP_DATA_CHUNK") } | ||
| } | ||
|
|
||
| private class HttpDataChunkInheritingContent extends TaintInheritingContent, DataFlow::FieldContent { | ||
| HttpDataChunkInheritingContent() { | ||
| this.getAField().getDeclaringType().(Union).getDeclaringType() instanceof HttpDataChunk and | ||
| ( | ||
| this.getAField().hasName("FromMemory") and | ||
| this.getIndirectionIndex() = 1 | ||
| or | ||
| this.getAField().hasName("FromFileHandle") and | ||
| this.getIndirectionIndex() = 1 | ||
| or | ||
| this.getAField().hasName("FromFragmentCache") and | ||
| this.getIndirectionIndex() = 1 | ||
| or | ||
| this.getAField().hasName("FromFragmentCacheEx") and | ||
| this.getIndirectionIndex() = 1 | ||
| or | ||
| this.getAField().hasName("Trailers") and | ||
| this.getIndirectionIndex() = 1 | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| private class FromMemory extends Class { | ||
| FromMemory() { | ||
| this.getDeclaringType().(Union).getDeclaringType() instanceof HttpDataChunk and | ||
| this.getAField().hasName("pBuffer") | ||
| } | ||
| } | ||
|
|
||
| private class FromMemoryInheritingContent extends TaintInheritingContent, DataFlow::FieldContent { | ||
| FromMemoryInheritingContent() { | ||
| this.getAField().getDeclaringType() instanceof FromMemory and | ||
| this.getAField().hasName("pBuffer") and | ||
| this.getIndirectionIndex() = 2 | ||
| } | ||
| } | ||
|
|
||
| private class FromFileHandle extends Class { | ||
| FromFileHandle() { | ||
| this.getDeclaringType().(Union).getDeclaringType() instanceof HttpDataChunk and | ||
| this.getAField().hasName("FileHandle") | ||
| } | ||
| } | ||
|
|
||
| private class FromFileHandleInheritingContent extends TaintInheritingContent, DataFlow::FieldContent | ||
| { | ||
| FromFileHandleInheritingContent() { | ||
| this.getAField().getDeclaringType() instanceof FromFileHandle and | ||
| this.getIndirectionIndex() = 1 and | ||
| this.getAField().hasName("FileHandle") | ||
| } | ||
| } | ||
|
|
||
| private class FromFragmentCacheOrCacheEx extends Class { | ||
| FromFragmentCacheOrCacheEx() { | ||
| this.getDeclaringType().(Union).getDeclaringType() instanceof HttpDataChunk and | ||
| this.getAField().hasName("pFragmentName") | ||
| } | ||
| } | ||
|
|
||
| private class FromFragmentCacheInheritingContent extends TaintInheritingContent, | ||
| DataFlow::FieldContent | ||
| { | ||
| FromFragmentCacheInheritingContent() { | ||
| this.getAField().getDeclaringType() instanceof FromFragmentCacheOrCacheEx and | ||
| this.getIndirectionIndex() = 2 and | ||
| this.getAField().hasName("pFragmentName") | ||
| } | ||
| } | ||
|
|
||
| private class HttpSslInfo extends Class { | ||
| HttpSslInfo() { this.hasGlobalName("_HTTP_SSL_INFO") } | ||
| } | ||
|
|
||
| private class HttpSslInfoInheritingContent extends TaintInheritingContent, DataFlow::FieldContent { | ||
| HttpSslInfoInheritingContent() { | ||
| this.getAField().getDeclaringType() instanceof HttpSslInfo and | ||
| this.getAField().hasName(["pServerCertIssuer", "pServerCertSubject", "pClientCertInfo"]) and | ||
| this.getIndirectionIndex() = 2 | ||
| } | ||
| } | ||
|
|
||
| private class HttpSslClientCertInfo extends Class { | ||
| HttpSslClientCertInfo() { this.hasGlobalName("_HTTP_SSL_CLIENT_CERT_INFO") } | ||
| } | ||
|
|
||
| private class HttpSslClientCertInfoInheritingContent extends TaintInheritingContent, | ||
| DataFlow::FieldContent | ||
| { | ||
| HttpSslClientCertInfoInheritingContent() { | ||
| this.getAField().getDeclaringType() instanceof HttpSslClientCertInfo and | ||
| ( | ||
| this.getAField().hasName("pCertEncoded") and | ||
| this.getIndirectionIndex() = 2 | ||
| or | ||
| this.getAField().hasName("Token") and | ||
| this.getIndirectionIndex() = 1 | ||
| ) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which version should I be looking at? V1 or V2?
https://msdn.microsoft.com/en-us/library/aa364612(v=vs.85)
https://msdn.microsoft.com/en-us/library/aa364613(v=vs.85)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, doesn't really matter, as V2 extends V1.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry about that! I meant to model
V1and just leaveV2for the future, but it seems I somehow messed up my naming in the process. dc8dc61 fixes it so that we explicitly call thisV1.