Skip to content

Commit cbcf4c0

Browse files
joefarebrotherjoefarebrother
committed
Move to main query pack
1 parent 0f013b8 commit cbcf4c0

File tree

11 files changed

+590
-0
lines changed

11 files changed

+590
-0
lines changed

‎csharp/ql/lib/semmle/code/csharp/security/auth/SecureCookies.qll‎

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
/**
2+
* Provides classes and predicates for detecting insecure cookies.
3+
*/
4+
5+
import csharp
6+
import semmle.code.csharp.frameworks.microsoft.AspNetCore
7+
8+
/**
9+
* Holds if the expression is a variable with a sensitive name.
10+
*/
11+
predicate isCookieWithSensitiveName(Expr cookieExpr) {
12+
exists(DataFlow::Node sink |
13+
AuthCookieName::flowTo(sink) and
14+
sink.asExpr() = cookieExpr
15+
)
16+
}
17+
18+
/**
19+
* Configuration for tracking if a variable with a sensitive name is used as an argument.
20+
*/
21+
private module AuthCookieNameConfig implements DataFlow::ConfigSig {
22+
private predicate isAuthVariable(Expr expr) {
23+
exists(string val |
24+
(
25+
val = expr.getValue() or
26+
val = expr.(Access).getTarget().getName()
27+
) and
28+
val.regexpMatch("(?i).*(session|login|token|user|auth|credential).*") and
29+
not val.regexpMatch("(?i).*(xsrf|csrf|forgery).*")
30+
)
31+
}
32+
33+
predicate isSource(DataFlow::Node source) { isAuthVariable(source.asExpr()) }
34+
35+
predicate isSink(DataFlow::Node sink) { exists(Call c | sink.asExpr() = c.getAnArgument()) }
36+
}
37+
38+
/**
39+
* Tracks if a variable with a sensitive name is used as an argument.
40+
*/
41+
private module AuthCookieName = DataFlow::Global<AuthCookieNameConfig>;
42+
43+
/**
44+
* Configuration module tracking creation of `CookieOptions` to `IResponseCookies.Append(String, String, CookieOptions)`
45+
* calls as a third parameter.
46+
*/
47+
private module CookieOptionsTrackingConfig implements DataFlow::ConfigSig {
48+
predicate isSource(DataFlow::Node source) {
49+
source.asExpr().(ObjectCreation).getType() instanceof MicrosoftAspNetCoreHttpCookieOptions
50+
}
51+
52+
predicate isSink(DataFlow::Node sink) {
53+
exists(MicrosoftAspNetCoreHttpResponseCookies iResponse, MethodCall mc |
54+
iResponse.getAppendMethod() = mc.getTarget() and
55+
mc.getArgument(2) = sink.asExpr()
56+
)
57+
}
58+
}
59+
60+
/**
61+
* Tracking creation of `CookieOptions` to `IResponseCookies.Append(String, String, CookieOptions)`
62+
* calls as a third parameter.
63+
*/
64+
module CookieOptionsTracking = DataFlow::Global<CookieOptionsTrackingConfig>;
65+
66+
/**
67+
* Looks for property value of `CookiePolicyOptions` passed to `app.UseCookiePolicy` in `Startup.Configure`.
68+
*/
69+
Expr getAValueForCookiePolicyProp(string prop) {
70+
exists(Method m, MethodCall mc, ObjectCreation oc, Expr val |
71+
m.getName() = "Configure" and
72+
m.getDeclaringType().getName() = "Startup" and
73+
m.getBody().getAChild+() = mc and
74+
mc.getTarget() =
75+
any(MicrosoftAspNetCoreBuilderCookiePolicyAppBuilderExtensions e).getUseCookiePolicyMethod() and
76+
oc.getType() instanceof MicrosoftAspNetCoreBuilderCookiePolicyOptions and
77+
getAValueForProp(oc, _, prop) = val and
78+
result = val
79+
)
80+
}
81+
82+
/**
83+
* A simplistic points-to alternative: given an object creation and a property name, get the values that property can be assigned.
84+
*
85+
* Assumptions:
86+
* - we don't reassign the variable that the creation is stored in
87+
* - we always access the creation through the same variable it is initially assigned to
88+
*
89+
* This should cover most typical patterns...
90+
*/
91+
Expr getAValueForProp(ObjectCreation create, Assignment a, string prop) {
92+
// values set in object init
93+
exists(MemberInitializer init, Expr src, PropertyAccess pa |
94+
a.getLValue() = pa and
95+
pa.getTarget().hasName(prop) and
96+
init = create.getInitializer().(ObjectInitializer).getAMemberInitializer() and
97+
init.getLValue() = pa and
98+
DataFlow::localExprFlow(src, init.getRValue()) and
99+
result = src
100+
)
101+
or
102+
// values set on var that create is assigned to
103+
exists(Expr src, PropertyAccess pa |
104+
a.getLValue() = pa and
105+
pa.getTarget().hasName(prop) and
106+
DataFlow::localExprFlow(create, pa.getQualifier()) and
107+
DataFlow::localExprFlow(src, a.getRValue()) and
108+
result = src
109+
)
110+
}
111+
112+
/**
113+
* Checks if the given property was explicitly set to a value.
114+
*/
115+
predicate isPropertySet(ObjectCreation oc, string prop) { exists(getAValueForProp(oc, _, prop)) }
116+
117+
private signature string propertyName();
118+
119+
/**
120+
* Configuration for tracking if a callback used in `OnAppendCookie` sets a cookie property to `true`.
121+
*/
122+
private module OnAppendCookieTrackingConfig<propertyName/0 getPropertyName> implements
123+
DataFlow::ConfigSig
124+
{
125+
/**
126+
* Specifies the cookie property name to track.
127+
*/
128+
predicate isSource(DataFlow::Node source) {
129+
exists(PropertyWrite pw, Assignment delegateAssign, Callable c |
130+
pw.getProperty().getName() = "OnAppendCookie" and
131+
pw.getProperty().getDeclaringType() instanceof MicrosoftAspNetCoreBuilderCookiePolicyOptions and
132+
delegateAssign.getLValue() = pw and
133+
(
134+
exists(LambdaExpr lambda |
135+
delegateAssign.getRValue() = lambda and
136+
lambda = c
137+
)
138+
or
139+
exists(DelegateCreation delegate |
140+
delegateAssign.getRValue() = delegate and
141+
delegate.getArgument().(CallableAccess).getTarget() = c
142+
)
143+
) and
144+
c.getParameter(0) = source.asParameter()
145+
)
146+
}
147+
148+
predicate isSink(DataFlow::Node sink) {
149+
exists(PropertyWrite pw, Assignment a |
150+
pw.getProperty().getDeclaringType() instanceof MicrosoftAspNetCoreHttpCookieOptions and
151+
pw.getProperty().getName() = getPropertyName() and
152+
a.getLValue() = pw and
153+
exists(Expr val |
154+
DataFlow::localExprFlow(val, a.getRValue()) and
155+
val.getValue() = "true"
156+
) and
157+
sink.asExpr() = pw.getQualifier()
158+
)
159+
}
160+
161+
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
162+
node2.asExpr() =
163+
any(PropertyRead pr |
164+
pr.getQualifier() = node1.asExpr() and
165+
pr.getProperty().getDeclaringType() instanceof
166+
MicrosoftAspNetCoreCookiePolicyAppendCookieContext
167+
)
168+
}
169+
}
170+
171+
private string getPropertyNameSecure() { result = "Secure" }
172+
173+
/**
174+
* Configuration module for tracking if a callback used in `OnAppendCookie` sets `Secure` to `true`.
175+
*/
176+
private module OnAppendCookieSecureTrackingConfig =
177+
OnAppendCookieTrackingConfig<getPropertyNameSecure/0>;
178+
179+
/**
180+
* Tracks if a callback used in `OnAppendCookie` sets `Secure` to `true`.
181+
*/
182+
module OnAppendCookieSecureTracking = DataFlow::Global<OnAppendCookieSecureTrackingConfig>;
183+
184+
private string getPropertyNameHttpOnly() { result = "HttpOnly" }
185+
186+
/**
187+
* Configuration module for tracking if a callback used in `OnAppendCookie` sets `HttpOnly` to `true`.
188+
*/
189+
private module OnAppendCookieHttpOnlyTrackingConfig =
190+
OnAppendCookieTrackingConfig<getPropertyNameHttpOnly/0>;
191+
192+
/**
193+
* Tracks if a callback used in `OnAppendCookie` sets `HttpOnly` to `true`.
194+
*/
195+
module OnAppendCookieHttpOnlyTracking = DataFlow::Global<OnAppendCookieHttpOnlyTrackingConfig>;

‎csharp/ql/src/Security Features/CWE-1004/CookieWithoutHttpOnly.qhelp‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Cookies without <code>HttpOnly</code> flag are accessible to JavaScript running in the same origin. In case of
9+
Cross-Site Scripting (XSS) vulnerability the cookie can be stolen by malicious script.
10+
</p>
11+
</overview>
12+
13+
<recommendation>
14+
<p>
15+
Protect sensitive cookies, such as related to authentication, by setting <code>HttpOnly</code> to <code>true</code> to make
16+
them not accessible to JavaScript. In ASP.NET case it is also possible to set the attribute via <code>&lt;httpCookies&gt;</code> element
17+
of <code>web.config</code> with the attribute <code>httpOnlyCookies="true"</code>.
18+
</p>
19+
</recommendation>
20+
21+
<example>
22+
23+
<p>
24+
In the example below <code>Microsoft.AspNetCore.Http.CookieOptions.HttpOnly</code> is set to <code>true</code>.
25+
</p>
26+
27+
<sample src="httponlyflagcore.cs" />
28+
29+
<p>
30+
In the following example <code>CookiePolicyOptions</code> are set programmatically to configure defaults.
31+
</p>
32+
33+
<sample src="cookiepolicyoptions.cs" />
34+
35+
<p>
36+
In the example below <code>System.Web.HttpCookie.HttpOnly</code> is set to <code>true</code>.
37+
</p>
38+
39+
<sample src="httponlyflag.cs" />
40+
41+
</example>
42+
43+
<references>
44+
45+
<li><a href="https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.cookieoptions.httponly">CookieOptions.HttpOnly Property,</a></li>
46+
<li><a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie">Set-Cookie</a> Header,</li>
47+
<li><a href="https://msdn.microsoft.com/en-us/library/system.web.httpcookie.httponly(v=vs.110).aspx">HttpCookie.HttpOnly Property,</a></li>
48+
<li><a href="https://msdn.microsoft.com/library/ms228262%28v=vs.100%29.aspx">httpCookies Element,</a></li>
49+
50+
</references>
51+
</qhelp>

‎csharp/ql/src/Security Features/CWE-1004/CookieWithoutHttpOnly.ql‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/**
2+
* @name 'HttpOnly' attribute is not set to true
3+
* @description Omitting the 'HttpOnly' attribute for security sensitive data allows
4+
* malicious JavaScript to steal it in case of XSS vulnerability. Always set
5+
* 'HttpOnly' to 'true' to authentication related cookie to make it
6+
* not accessible by JavaScript.
7+
* @kind problem
8+
* @problem.severity warning
9+
* @precision high
10+
* @id cs/web/cookie-httponly-not-set
11+
* @tags security
12+
* experimental
13+
* external/cwe/cwe-1004
14+
*/
15+
16+
import csharp
17+
import semmle.code.asp.WebConfig
18+
import semmle.code.csharp.frameworks.system.Web
19+
import semmle.code.csharp.frameworks.microsoft.AspNetCore
20+
import semmle.code.csharp.security.auth.SecureCookies
21+
22+
predicate cookieAppendHttpOnlyByDefault() {
23+
// default is set to `Always`
24+
getAValueForCookiePolicyProp("HttpOnly").getValue() = "1"
25+
or
26+
// there is an `OnAppendCookie` callback that sets `HttpOnly` to true
27+
not OnAppendCookieHttpOnlyTracking::flowTo(_)
28+
}
29+
30+
predicate httpOnlyFalse(ObjectCreation oc) {
31+
exists(Assignment a |
32+
getAValueForProp(oc, a, "HttpOnly") = a.getRValue() and
33+
a.getRValue().getValue() = "false"
34+
)
35+
}
36+
37+
predicate httpOnlyFalseOrNotSet(ObjectCreation oc) {
38+
httpOnlyFalse(oc)
39+
or
40+
not isPropertySet(oc, "HttpOnly")
41+
}
42+
43+
predicate nonHttpOnlyCookieOptionsCreation(ObjectCreation oc, MethodCall append) {
44+
// `HttpOnly` property in `CookieOptions` passed to IResponseCookies.Append(...) wasn't set
45+
oc.getType() instanceof MicrosoftAspNetCoreHttpCookieOptions and
46+
httpOnlyFalseOrNotSet(oc) and
47+
exists(DataFlow::Node creation, DataFlow::Node sink |
48+
CookieOptionsTracking::flow(creation, sink) and
49+
creation.asExpr() = oc and
50+
sink.asExpr() = append.getArgument(2)
51+
)
52+
}
53+
54+
predicate nonHttpOnlySensitiveCookieCreation(ObjectCreation oc) {
55+
oc.getType() instanceof SystemWebHttpCookie and
56+
isCookieWithSensitiveName(oc.getArgument(0)) and
57+
(
58+
httpOnlyFalse(oc)
59+
or
60+
// the property wasn't explicitly set, so a default value from config is used
61+
not isPropertySet(oc, "HttpOnly") and
62+
// the default in config is not set to `true`
63+
not exists(XmlElement element |
64+
element instanceof HttpCookiesElement and
65+
element.(HttpCookiesElement).isHttpOnlyCookies()
66+
)
67+
)
68+
}
69+
70+
predicate sensitiveCookieAppend(MethodCall mc) {
71+
exists(MicrosoftAspNetCoreHttpResponseCookies iResponse |
72+
iResponse.getAppendMethod() = mc.getTarget() and
73+
isCookieWithSensitiveName(mc.getArgument(0))
74+
)
75+
}
76+
77+
predicate nonHttpOnlyCookieCall(Call c) {
78+
(
79+
not cookieAppendHttpOnlyByDefault() and
80+
exists(MethodCall mc |
81+
sensitiveCookieAppend(mc) and
82+
(
83+
nonHttpOnlyCookieOptionsCreation(c, mc)
84+
or
85+
// IResponseCookies.Append(String, String) was called, `HttpOnly` is set to `false` by default
86+
mc = c and
87+
mc.getNumberOfArguments() < 3
88+
)
89+
)
90+
or
91+
nonHttpOnlySensitiveCookieCreation(c)
92+
)
93+
}
94+
95+
predicate nonHttpOnlyPolicyAssignment(Assignment a, Expr val) {
96+
val.getValue() = "false" and
97+
exists(PropertyWrite pw |
98+
(
99+
pw.getProperty().getDeclaringType() instanceof MicrosoftAspNetCoreHttpCookieBuilder or
100+
pw.getProperty().getDeclaringType() instanceof
101+
MicrosoftAspNetCoreAuthenticationCookiesCookieAuthenticationOptions
102+
) and
103+
pw.getProperty().getName() = "HttpOnly" and
104+
a.getLValue() = pw and
105+
DataFlow::localExprFlow(val, a.getRValue())
106+
)
107+
}
108+
109+
from Expr httpOnlySink
110+
where
111+
(
112+
nonHttpOnlyCookieCall(httpOnlySink)
113+
or
114+
exists(Assignment a |
115+
httpOnlySink = a.getRValue() and
116+
nonHttpOnlyPolicyAssignment(a, _)
117+
)
118+
)
119+
select httpOnlySink, "Cookie attribute 'HttpOnly' is not set to true."

‎csharp/ql/src/Security Features/CWE-1004/cookiepolicyoptions.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
public class Startup
2+
{
3+
// This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
4+
public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
5+
{
6+
app.UseCookiePolicy(new CookiePolicyOptions()
7+
{
8+
Secure = Microsoft.AspNetCore.Http.CookieSecurePolicy.Always,
9+
HttpOnly = Microsoft.AspNetCore.CookiePolicy.HttpOnlyPolicy.Always
10+
});
11+
}
12+
}

‎csharp/ql/src/Security Features/CWE-1004/httponlyflag.cs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
class MyController : Controller
2+
{
3+
void Login()
4+
{
5+
var cookie = new System.Web.HttpCookie("cookieName") { HttpOnly = true };
6+
}
7+
}

0 commit comments

Comments
 (0)