[javascript] CodeQL query to detect if cookies are sent without the flag secure being set by dellalibera · Pull Request #3978 · github/codeql

dellalibera · 2020-07-26T21:04:03Z

Query to detect if the secure flag is set to cookies is available in java query but it is not available in javascript query.

This query detects if cookies are sent without the flag secure being set or with the flag secure being set to false.

Failing to set the secure flag on a cookie can cause it to be sent in cleartext.
This makes it easier for an attacker to intercept and read the cookie.

This query handles the cases when cookies are set with:

Express middleware cookie-session (https://github.com/expressjs/cookie-session)
Express middleware express-session (https://github.com/expressjs/session)
Express response.cookie (https://expressjs.com/en/api.html#res.cookie)
Set-Cookie of an HTTP response (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie)
js-cookie (https://github.com/js-cookie/js-cookie)

I added several examples showing the different cases when the cookie is sent without the flag secure being set or with the flag secure being set to false.

Any feedback is welcome.
I hope this will help.

… set

intrigus-lgtm · 2020-07-26T21:31:55Z

 <recommendation>

-<p>Always set the <code>secure</code> flag to `true` on a cookie before adding it
+<p>Always set the `secure` flag to `true` on a cookie before adding it


Why did you change this?
As far as I know, this will not work, but <code> will work.

Sorry, I was thinking the opposite. Thanks for the feedback. I made the change.

esbena

Very nice. I only have some minor change requests.

Have you seen https://github.com/github/codeql/blob/71933a4d8a32d32f085f14b7b44f41d1cdcae2e4/javascript/ql/src/semmle/javascript/frameworks/CookieLibraries.qll ? It could inspire some changes to this PR, or perhaps some of the content in this PR belongs in that file (note that this file is not in "experimental", so I will set the bar a review bar a bit higher)?

One large, and optional, change to consider.
The qhelp file is a bit verbose in terms of examples. Could you remove some of them, and perhaps add them as tests instead? See #3835 (comment) for how another external contributor recently made tests.

It is a bit of a shame that result = this.getCookieOptionsArgument().getAPropertyWrite(flag).getRhs() has to be repeated so many times. This could be fixed with some additional inheritance, but that is a matter of taste.

Generally. Whenever you write exist(X x | x = getX() and ...) without ever using the x more than once, you can write exists(getX()) and ... instead.

esbena · 2020-08-04T06:55:08Z

+  /**
+   * Abstract class to represent different cases of insecure cookie settings.
+   */
+  abstract class InsecureCookies extends DataFlow::Node {


Could we rename the entire InsecureCookie module and class to just Cookie, and then just let the isInsecure determine if the cookie is secure or not.

Yes, no problem. I made the change.

esbena · 2020-08-04T07:00:43Z

+     * Predicate that determines if a cookie is insecure.
+     */
+    abstract predicate isInsecure();


Suggested change

* Predicate that determines if a cookie is insecure.

*/

abstract predicate isInsecure();

* Holds if this cookie is secure.

*/

abstract predicate isSecure();

Optional: I think it is clearer to flip the polarity and avoid the negation in the predicate name.
Please note that this requires a change to all implementations of this predicate below.

Yes, I agree with you. I changed the polarity (I did it in one commit) of all the implementations of this predicate.

esbena · 2020-08-04T07:25:00Z

+    override predicate isInsecure() {
+      not exists(string s |
+        getCookieOptionsArgument().mayHaveStringValue(s) and
+        s.matches("%; secure%")


Hmm. What if the string is ...;secure...? That is: it lacks a space between ; and secure.

How about something likes.regexpMatch("(.*;)?\\s*secure.*") instead? That should allow any whitespace variation, and also permit the string to start with secure.

Again, you are right. I missed that. I made the change

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

dellalibera · 2020-08-16T13:29:04Z

Hi @esbena,

sorry for the late reply.
I super appreciated your feedback (I learnt a lot).

About the js-cookie library I was inspired by the code that you mentioned https://github.com/github/codeql/blob/71933a4d8a32d32f085f14b7b44f41d1cdcae2e4/javascript/ql/src/semmle/javascript/frameworks/CookieLibraries.qll.
However the existing implementation provides access only to the value of a cookie and not its options (please correct me if I am wrong).

I made all the changes that you suggested and then I also changed the polarity of the predicate isInsecure (I changed also all the implementations of this predicate).

Finally, I removed all the examples and I added them as tests, as you suggested (I hope I added in the correct folder).

Let me know if now it’s ok and if there is something else I can do.
Again, thanks for all the feedback.

esbena · 2020-08-24T07:39:43Z

Thanks. The new tests seem fine.
I have a few QL style comments left.
Please remember to run codeql query format before pushing, otherwise the PR checks will fail.

dellalibera · 2020-08-24T17:00:25Z

Thanks for your reply. I formatted the code.
Let me know if there is something else I can do.

esbena

Oops. Forgot to attach the inline feedback. PTAL

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

dellalibera · 2020-08-25T23:51:33Z

@esbena, no problem :)
thanks for the other suggestions (I learnt some new stuff).

Let me know if there is something else I can do.

intrigus-lgtm · 2020-08-26T00:26:53Z

^ Pro tip: in the "Files changed" view, you have the option of adding review suggestions to a batch and commit them all at once. (github/codeql-go#66 (comment))

esbena

One more thing. I stumbled upon it when I looked at the results of the query on LGTM.com.

Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

dellalibera · 2020-08-26T21:51:49Z

@intrigus-lgtm thanks for the tip.

@esbena I made the changes and updated the expected output.

esbena · 2020-08-27T07:47:11Z

I took the liberty of pushing a formatting fix to your branch. The tests should go green shortly.

esbena · 2020-08-28T07:32:22Z

Thank you for yet another contribution @dellalibera.

dellalibera · 2020-08-29T08:53:40Z

@esbena It was really a pleasure. Thank you for all the feedback.

Add Codeql query to detect if cookies are sent without the flag being…

c469f71

… set

dellalibera requested a review from a team as a code owner July 26, 2020 21:04

dellalibera changed the title ~~CodeQL query to detect if cookies are sent without the flag secure being set~~ [javascript] CodeQL query to detect if cookies are sent without the flag secure being set Jul 26, 2020

Update .qhelp

2cec8f7

intrigus-lgtm reviewed Jul 26, 2020

View reviewed changes

dellalibera added 2 commits July 26, 2020 23:47

Update .qhelp

ac7c511

Update .qhelp

8dee3da

esbena self-assigned this Aug 3, 2020

asgerf added the JS label Aug 3, 2020

esbena requested changes Aug 4, 2020

View reviewed changes

adityasharad changed the base branch from master to main August 14, 2020 18:33

dellalibera and others added 19 commits August 16, 2020 14:13

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

67fccac

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

0c12106

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

8d26b81

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

10bd745

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

5cae300

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

e463014

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

fb3ffb8

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

97f039a

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

40e101d

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

ab128f7

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

9292e3b

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

275b8df

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

14c8e4c

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

a2e9456

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

bfef84e

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

ab20beb

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

05ffd67

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

1ba39e4

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Remove redundancy

e290802

dellalibera added 6 commits August 16, 2020 14:48

Replace regex

d4b231b

Replace class and module name

91d4485

Changed .qhelp

2a32297

Remove examples

3e9142b

Add query-tests

5d6e6be

Change polarity predicate isInsecure

8ec91ef

Format code

22f5ae4

esbena reviewed Aug 24, 2020

View reviewed changes

dellalibera and others added 4 commits August 26, 2020 01:46

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

57cf447

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

3bd7615

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

a1f64e2

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie…

e027c8c

….qll Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

esbena requested changes Aug 26, 2020

View reviewed changes

Comment thread javascript/ql/src/experimental/Security/CWE-614/InsecureCookie.ql Outdated

dellalibera and others added 2 commits August 26, 2020 23:33

Update javascript/ql/src/experimental/Security/CWE-614/InsecureCookie.ql

dcf51c7

Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Update expected output

cd1d50b

JS: fix formatting of InsecureCookie.qll

9aa1404

esbena approved these changes Aug 27, 2020

View reviewed changes

codeql-ci merged commit ac94869 into github:main Aug 28, 2020

dellalibera mentioned this pull request Aug 29, 2020

[javascript] CWE-614: CodeQL query to detect if cookies are sent without the flag secure being set github/securitylab#169

Closed

1 task

Conversation

dellalibera commented Jul 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esbena left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dellalibera commented Aug 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esbena commented Aug 24, 2020

Uh oh!

dellalibera commented Aug 24, 2020

Uh oh!

esbena left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dellalibera commented Aug 25, 2020

Uh oh!

intrigus-lgtm commented Aug 26, 2020

Uh oh!

esbena left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dellalibera commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esbena commented Aug 27, 2020

Uh oh!

esbena commented Aug 28, 2020

Uh oh!

dellalibera commented Aug 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dellalibera commented Aug 16, 2020 •

edited

Loading

dellalibera commented Aug 26, 2020 •

edited

Loading