Go: Add Rs Cors Support #14873
Go: Add Rs Cors Support #14873
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /** | ||
| * Provides abstract class for modeling the Go CORS handler model allow all origins write. | ||
| */ |
Check warning
Code scanning / CodeQL
Class QLDoc style. Warning
| /** | ||
| * Provides abstract class for modeling the Go CORS handler model allow credentials write. | ||
| */ |
Check warning
Code scanning / CodeQL
Class QLDoc style. Warning
|
Actually, since these abstract classes are only used by this one query, they shouldn't go in concepts (which is more meant for abstract classes that are used by multiple queries, I think). And I also didn't notice that this query is still in experimental. That makes it difficult to know where to put them. Maybe where they are is good enough for now. Do you think the query should be promoted? |
| } | ||
|
|
||
| /** | ||
| * A variable of type Options that holds the headers to be set. |
There was a problem hiding this comment.
This isn't actually correct: if v is a.b.c then this is the Variable a and the thing that has type Options is a.b.c. I didn't notice this when you originally modeled GinCors. Can I ask why you are doing it this way? Is there a particular case where just using Variable or just using SsaWithFields doesn't work?
There was a problem hiding this comment.
Looking at it more carefully, I see that, because of the way that they are modeled, RsOptions and GinConfig are confined to be local variables defined in functions. Is that what you intended? Might you ever want to reason about one that is defined at package scope?
There was a problem hiding this comment.
This isn't actually correct: if
visa.b.cthenthisis theVariable aand the thing that has typeOptionsisa.b.c. I didn't notice this when you originally modeled GinCors. Can I ask why you are doing it this way? Is there a particular case where just usingVariableor just usingSsaWithFieldsdoesn't work?
I can change this to just use SSAWithFields. I will ensure that the getBaseVariable() is removed. I see the problem with the current model is that since I am using SSAVariable, getSourceVariable's result is by definition a local variable. I would like to support package variables as well. I have tried to remove SSAs all together and just use Node, but cannot find a way for two nodes to know if they actually represent the same variable. If you can think of any way to do this that would be great.
var opts cors.Options <----- the package opts variables
func rs_vulnerable() {
opts.AllowedMethods = []string{"POST"}
opts.AllowedOrigins = []string{"null"} <--- how to compare Node corresponding to this opts
opts.AllowCredentials = true <---- to Node corresponding to this opts and see that they both correspond to the package level optsThere was a problem hiding this comment.
Sorry I wasn't clearer about why the way they are modeled means they have to be local variables. Well done for figuring it out - coming back to this after four months, it took me a few minutes, even with your explanation.
I would say that SSAWithFields works great as long as you don't mind missing out writes to global variables. You may decide that in practice, that pattern doesn't come up and you're okay with not spotting it.
When dealing with just nodes, I think the normal way to think about them "representing the same variable" is whether there is value/taint flow from one to another.
There is an approach to this kind of thing in go/ql/src/experimental/CWE-1004/AuthCookie.qll, but I'm not sure it's necessarily one that I'd recommend.
I'll ask the rest of the codeql-go team if they can think of a better way of doing it.
There was a problem hiding this comment.
@owen-mc let me know if you have any updates on this :)
There was a problem hiding this comment.
Yes, sorry I didn't get back to you. The best way to include global variables is to copy this code which defines SsaWithFieldsand replace the root case with a read from a global variable. I guess GlobalWithFields would be the obvious name.
There was a problem hiding this comment.
@owen-mc tbh this seems to be a bit complex for my CodeQL skills, but I have written some initial code in the spirit of what you suggested Let me know if I'm on a the right the path (ignore any of the comments). I need getDefinition but it uses getLocalDefinition but I'm not too familiar with basic blocks in CodeQL to create a version for global. Any advice would be great regarding if this was what you were thinking of and in regards to getLocalDefinition. Cheers
Add RsCors support to CodeQL CorsMisconfiguration.ql