Skip to content

Commit 8ed5b2d

Browse files
joefarebrotherjoefarebrother
committed
Refactor secure cookie query
1 parent b57243e commit 8ed5b2d

File tree

1 file changed

+103
-81
lines changed

1 file changed

+103
-81
lines changed

‎csharp/ql/src/experimental/Security Features/CWE-614/CookieWithoutSecure.ql‎

Lines changed: 103 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -17,91 +17,113 @@ import csharp
1717
import semmle.code.asp.WebConfig
1818
import semmle.code.csharp.frameworks.system.Web
1919
import semmle.code.csharp.frameworks.microsoft.AspNetCore
20-
deprecated import experimental.dataflow.flowsources.AuthCookie
20+
import experimental.dataflow.flowsources.AuthCookie
2121

22-
deprecated query predicate problems(Expr secureSink, string message) {
22+
predicate cookieAppendSecureByDefault() {
23+
// default is set to `Always` or `SameAsRequest`
2324
(
24-
exists(Call c |
25-
secureSink = c and
26-
(
27-
// default is not configured or is not set to `Always` or `SameAsRequest`
28-
not (
29-
getAValueForCookiePolicyProp("Secure").getValue() = "0" or
30-
getAValueForCookiePolicyProp("Secure").getValue() = "1"
31-
) and
32-
// there is no callback `OnAppendCookie` that sets `Secure` to true
33-
not OnAppendCookieSecureTracking::flowTo(_) and
34-
(
35-
// `Secure` property in `CookieOptions` passed to IResponseCookies.Append(...) wasn't set
36-
exists(ObjectCreation oc |
37-
oc = c and
38-
oc.getType() instanceof MicrosoftAspNetCoreHttpCookieOptions and
39-
not isPropertySet(oc, "Secure") and
40-
exists(DataFlow::Node creation |
41-
CookieOptionsTracking::flow(creation, _) and
42-
creation.asExpr() = oc
43-
)
44-
)
45-
or
46-
// IResponseCookies.Append(String, String) was called, `Secure` is set to `false` by default
47-
exists(MethodCall mc, MicrosoftAspNetCoreHttpResponseCookies iResponse |
48-
mc = c and
49-
iResponse.getAppendMethod() = mc.getTarget() and
50-
mc.getNumberOfArguments() < 3
51-
)
52-
)
53-
or
54-
exists(ObjectCreation oc |
55-
oc = c and
56-
oc.getType() instanceof SystemWebHttpCookie and
57-
// the property wasn't explicitly set, so a default value from config is used
58-
not isPropertySet(oc, "Secure") and
59-
// the default in config is not set to `true`
60-
// the `exists` below covers the `cs/web/requiressl-not-set`
61-
not exists(XmlElement element |
62-
element instanceof FormsElement and
63-
element.(FormsElement).isRequireSsl()
64-
or
65-
element instanceof HttpCookiesElement and
66-
element.(HttpCookiesElement).isRequireSsl()
67-
)
68-
)
69-
)
25+
getAValueForCookiePolicyProp("Secure").getValue() = "0" or
26+
getAValueForCookiePolicyProp("Secure").getValue() = "1"
27+
)
28+
or
29+
//callback `OnAppendCookie` that sets `Secure` to true
30+
OnAppendCookieSecureTracking::flowTo(_)
31+
}
32+
33+
predicate insecureCookieOptionsCreation(Expr sink) {
34+
// `Secure` property in `CookieOptions` passed to IResponseCookies.Append(...) wasn't set
35+
exists(ObjectCreation oc |
36+
oc = sink and
37+
oc.getType() instanceof MicrosoftAspNetCoreHttpCookieOptions and
38+
not isPropertySet(oc, "Secure") and
39+
exists(DataFlow::Node creation |
40+
CookieOptionsTracking::flow(creation, _) and
41+
creation.asExpr() = oc
7042
)
43+
)
44+
}
45+
46+
predicate insecureCookieAppend(Expr sink) {
47+
// IResponseCookies.Append(String, String) was called, `Secure` is set to `false` by default
48+
exists(MethodCall mc, MicrosoftAspNetCoreHttpResponseCookies iResponse |
49+
mc = sink and
50+
iResponse.getAppendMethod() = mc.getTarget() and
51+
mc.getNumberOfArguments() < 3
52+
)
53+
}
54+
55+
predicate insecureCookieCreationFromConfig(Expr sink) {
56+
// `Secure` property in `System.Web.HttpCookie` wasn't set, so a default value from config is used
57+
exists(ObjectCreation oc |
58+
oc = sink and
59+
oc.getType() instanceof SystemWebHttpCookie and
60+
not isPropertySet(oc, "Secure") and
61+
// the default in config is not set to `true`
62+
// the `exists` below covers the `cs/web/requiressl-not-set`
63+
not exists(XmlElement element |
64+
element instanceof FormsElement and
65+
element.(FormsElement).isRequireSsl()
66+
or
67+
element instanceof HttpCookiesElement and
68+
element.(HttpCookiesElement).isRequireSsl()
69+
)
70+
)
71+
}
72+
73+
predicate insecureCookieCall(Call c) {
74+
not cookieAppendSecureByDefault() and
75+
(
76+
insecureCookieOptionsCreation(c)
7177
or
72-
exists(Assignment a, Expr val |
73-
secureSink = a.getRValue() and
74-
(
75-
exists(ObjectCreation oc |
76-
getAValueForProp(oc, a, "Secure") = val and
77-
val.getValue() = "false" and
78-
(
79-
oc.getType() instanceof SystemWebHttpCookie
80-
or
81-
oc.getType() instanceof MicrosoftAspNetCoreHttpCookieOptions and
82-
// there is no callback `OnAppendCookie` that sets `Secure` to true
83-
not OnAppendCookieSecureTracking::flowTo(_) and
84-
// the cookie option is passed to `Append`
85-
exists(DataFlow::Node creation |
86-
CookieOptionsTracking::flow(creation, _) and
87-
creation.asExpr() = oc
88-
)
89-
)
90-
)
91-
or
92-
exists(PropertyWrite pw |
93-
(
94-
pw.getProperty().getDeclaringType() instanceof MicrosoftAspNetCoreHttpCookieBuilder or
95-
pw.getProperty().getDeclaringType() instanceof
96-
MicrosoftAspNetCoreAuthenticationCookiesCookieAuthenticationOptions
97-
) and
98-
pw.getProperty().getName() = "SecurePolicy" and
99-
a.getLValue() = pw and
100-
DataFlow::localExprFlow(val, a.getRValue()) and
101-
val.getValue() = "2" // None
102-
)
78+
insecureCookieAppend(c)
79+
)
80+
or
81+
insecureCookieCreationFromConfig(c)
82+
}
83+
84+
predicate insecureCookieCreationAssignment(Assignment a, Expr val) {
85+
exists(ObjectCreation oc |
86+
getAValueForProp(oc, a, "Secure") = val and
87+
val.getValue() = "false" and
88+
(
89+
oc.getType() instanceof SystemWebHttpCookie
90+
or
91+
oc.getType() instanceof MicrosoftAspNetCoreHttpCookieOptions and
92+
// there is no callback `OnAppendCookie` that sets `Secure` to true
93+
not OnAppendCookieSecureTracking::flowTo(_) and
94+
// the cookie option is passed to `Append`
95+
exists(DataFlow::Node creation |
96+
CookieOptionsTracking::flow(creation, _) and
97+
creation.asExpr() = oc
10398
)
10499
)
105-
) and
106-
message = "Cookie attribute 'Secure' is not set to true."
100+
)
107101
}
102+
103+
predicate insecureSecurePolicyAssignment(Assignment a, Expr val) {
104+
exists(PropertyWrite pw |
105+
(
106+
pw.getProperty().getDeclaringType() instanceof MicrosoftAspNetCoreHttpCookieBuilder or
107+
pw.getProperty().getDeclaringType() instanceof
108+
MicrosoftAspNetCoreAuthenticationCookiesCookieAuthenticationOptions
109+
) and
110+
pw.getProperty().getName() = "SecurePolicy" and
111+
a.getLValue() = pw and
112+
DataFlow::localExprFlow(val, a.getRValue()) and
113+
val.getValue() = "2" // None
114+
)
115+
}
116+
117+
from Expr secureSink
118+
where
119+
insecureCookieCall(secureSink)
120+
or
121+
exists(Assignment a |
122+
secureSink = a.getRValue() and
123+
(
124+
insecureCookieCreationAssignment(a, _)
125+
or
126+
insecureSecurePolicyAssignment(a, _)
127+
)
128+
)
129+
select secureSink, "Cookie attribute 'Secure' is not set to true."

0 commit comments

Comments
 (0)