Update comments and names

Author: joefarebrother

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

@@ -1,12 +1,12 @@
 /**
- * Provides classes and predicates for detecting insecure cookies.
+ * Definitions for detecting insecure and non-httponly cookies.
  */
 
 import csharp
 import semmle.code.csharp.frameworks.microsoft.AspNetCore
 
 /**
- * Holds if the expression is a variable with a sensitive name.
+ * Holds if the expression is a sensitive string literal or a variable with a sensitive name.
  */
 predicate isCookieWithSensitiveName(Expr cookieExpr) {
   exists(DataFlow::Node sink |
@@ -16,7 +16,7 @@ predicate isCookieWithSensitiveName(Expr cookieExpr) {
 }
 
 /**
- * Configuration for tracking if a variable with a sensitive name is used as an argument.
+ * Configuration for tracking if a sensitive string literal or a variable with a sensitive name is used as an argument.
  */
 private module AuthCookieNameConfig implements DataFlow::ConfigSig {
   private predicate isAuthVariable(Expr expr) {
@@ -118,13 +118,13 @@ private signature string propertyName();
 
 /**
  * Configuration for tracking if a callback used in `OnAppendCookie` sets a cookie property to `true`.
+ *
+ * ` getPropertyName` specifies the cookie property name to track.
  */
 private module OnAppendCookieTrackingConfig<propertyName/0 getPropertyName> implements
   DataFlow::ConfigSig
 {
-  /**
-   * Specifies the cookie property name to track.
-   */
+  /** Source is the parameter of a callback passed to `OnAppendCookie` */
   predicate isSource(DataFlow::Node source) {
     exists(PropertyWrite pw, Assignment delegateAssign, Callable c |
       pw.getProperty().getName() = "OnAppendCookie" and
@@ -145,6 +145,7 @@ private module OnAppendCookieTrackingConfig<propertyName/0 getPropertyName> impl
     )
   }
 
+  /** Sink is a property write that sets the given property to `true`. */
   predicate isSink(DataFlow::Node sink) {
     exists(PropertyWrite pw, Assignment a |
       pw.getProperty().getDeclaringType() instanceof MicrosoftAspNetCoreHttpCookieOptions and
@@ -177,7 +178,7 @@ private module OnAppendCookieSecureTrackingConfig =
   OnAppendCookieTrackingConfig<getPropertyNameSecure/0>;
 
 /**
- * Tracks if a callback used in `OnAppendCookie` sets `Secure` to `true`.
+ * Tracks if a callback used in `OnAppendCookie` sets `Secure` to `true`, and thus cookies appended to responses are secure by default.
  */
 module OnAppendCookieSecureTracking = DataFlow::Global<OnAppendCookieSecureTrackingConfig>;
 
@@ -190,6 +191,6 @@ private module OnAppendCookieHttpOnlyTrackingConfig =
   OnAppendCookieTrackingConfig<getPropertyNameHttpOnly/0>;
 
 /**
- * Tracks if a callback used in `OnAppendCookie` sets `HttpOnly` to `true`.
+ * Tracks if a callback used in `OnAppendCookie` sets `HttpOnly` to `true`, and thus cookies appended to responses are httponly by default.
  */
 module OnAppendCookieHttpOnlyTracking = DataFlow::Global<OnAppendCookieHttpOnlyTrackingConfig>;

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

@@ -1,15 +1,13 @@
 /**
- * @name 'HttpOnly' attribute is not set to true
- * @description Omitting the 'HttpOnly' attribute for security sensitive data allows
- *              malicious JavaScript to steal it in case of XSS vulnerability. Always set
- *              'HttpOnly' to 'true' to authentication related cookie to make it
- *              not accessible by JavaScript.
+ * @name Cookie 'HttpOnly' attribute is not set to true
+ * @description Sensitive cookies without the `HttpOnly` property set are accessible by client-side scripts such as JavaScript.
+ *              This makes them more vulnerable to being stolen by an XSS attack.
  * @kind problem
  * @problem.severity warning
+ * @security-severity 5.0
  * @precision high
  * @id cs/web/cookie-httponly-not-set
  * @tags security
- *       experimental
  *       external/cwe/cwe-1004
  */
 
@@ -51,7 +49,7 @@ predicate nonHttpOnlyCookieOptionsCreation(ObjectCreation oc, MethodCall append)
   )
 }
 
-predicate nonHttpOnlySensitiveCookieCreation(ObjectCreation oc) {
+predicate nonHttpOnlySystemWebSensitiveCookieCreation(ObjectCreation oc) {
   oc.getType() instanceof SystemWebHttpCookie and
   isCookieWithSensitiveName(oc.getArgument(0)) and
   (
@@ -88,7 +86,7 @@ predicate nonHttpOnlyCookieCall(Call c) {
       )
     )
     or
-    nonHttpOnlySensitiveCookieCreation(c)
+    nonHttpOnlySystemWebSensitiveCookieCreation(c)
   )
 }
 

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

@@ -1,10 +1,10 @@
 /**
- * @name 'Secure' attribute is not set to true
- * @description Omitting the 'Secure' attribute allows data to be transmitted insecurely
- *              using HTTP. Always set 'Secure' to 'true' to ensure that HTTPS
- *              is used at all times.
+ * @name Cookie 'Secure' attribute is not set to true
+ * @description Cookies without the `Secure` flag may be sent in cleartext.
+ *              This makes them vulnerable to be intercepted by an attacker.
  * @kind problem
  * @problem.severity error
+ * @security-severity 5.0
  * @precision high
  * @id cs/web/cookie-secure-not-set
  * @tags security
@@ -61,15 +61,14 @@ predicate insecureCookieAppend(Expr sink) {
   )
 }
 
-predicate insecureCookieCreation(ObjectCreation oc) {
+predicate insecureSystemWebCookieCreation(ObjectCreation oc) {
   oc.getType() instanceof SystemWebHttpCookie and
   (
     secureFalse(oc)
     or
     // `Secure` property in `System.Web.HttpCookie` wasn't set, so a default value from config is used
-    isPropertySet(oc, "Secure") and
+    not isPropertySet(oc, "Secure") and
     // the default in config is not set to `true`
-    // the `exists` below covers the `cs/web/requiressl-not-set`
     not exists(XmlElement element |
       element instanceof FormsElement and
       element.(FormsElement).isRequireSsl()
@@ -88,7 +87,7 @@ predicate insecureCookieCall(Call c) {
     insecureCookieAppend(c)
   )
   or
-  insecureCookieCreation(c)
+  insecureSystemWebCookieCreation(c)
 }
 
 predicate insecureSecurePolicyAssignment(Assignment a, Expr val) {
@@ -105,12 +104,14 @@ predicate insecureSecurePolicyAssignment(Assignment a, Expr val) {
   )
 }
 
-from Expr secureSink
+from Expr secureSink, string msg
 where
-  insecureCookieCall(secureSink)
+  insecureCookieCall(secureSink) and
+  msg = "Cookie attribute 'Secure' is not set to true."
   or
   exists(Assignment a |
     secureSink = a.getRValue() and
     insecureSecurePolicyAssignment(a, _)
-  )
-select secureSink, "Cookie attribute 'Secure' is not set to true."
+  ) and
+  msg = "Cookie security policy sets cookies as insecure by default."
+select secureSink, msg