Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Python: Promote the insecure cookie query from experimental #16933

Merged
merged 25 commits into from
Aug 7, 2024
Merged

Python: Promote the insecure cookie query from experimental #16933

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
6f7b2a2
Add cookie flags to cookie write concept, and alter experimental quer…
joefarebrother May 28, 2024
2df09f6
Change flag predicates to boolean parameters rather than boolean results
joefarebrother Jun 28, 2024
9ad6c8c
Implement cookie attributes for cases in which a raw header is set
joefarebrother Jul 8, 2024
033dd9f
Promote insecure cookie query
joefarebrother Jul 8, 2024
6a7bdaf
Fix experimental query compilation
joefarebrother Jul 9, 2024
32fbe52
Model cookie attributes for Django and Flask
joefarebrother Jul 9, 2024
df5569f
Add documentation
joefarebrother Jul 11, 2024
226e4eb
Use a 3-valued newtype for hasSameSiteAttribute
joefarebrother Jul 17, 2024
a73d675
Remove experimental query versions
joefarebrother Jul 19, 2024
be87eb5
Add cookie models to each framework
joefarebrother Jul 22, 2024
b28d799
Update ConceptsTests and make a fix
joefarebrother Jul 23, 2024
93f70b3
Add unit tests
joefarebrother Jul 23, 2024
4427181
Add change note
joefarebrother Jul 23, 2024
db27fd9
Add tests for tornado and twisted
joefarebrother Jul 23, 2024
8f714c6
Code reveiw suggestions. correction in changenote + style in example
joefarebrother Jul 24, 2024
d997eee
Code review suggestions - make definitions clearer
joefarebrother Jul 29, 2024
1127b08
Merge branch 'main' into python-cookie-concept-promote
joefarebrother Jul 29, 2024
c7f9095
Apply similar changes to httponly
joefarebrother Jul 29, 2024
90e87a1
Factor each framework implementation of the cookie parameters to a co…
joefarebrother Jul 29, 2024
ef3bbea
Add check for kwargs in cookie attribute predicates
joefarebrother Jul 29, 2024
68512ee
Remove remaining files from experimental tests
joefarebrother Jul 29, 2024
f10d007
Add additional test for kwargs case
joefarebrother Jul 29, 2024
82da8b9
Fix typo
joefarebrother Jul 29, 2024
e68ef87
update inline tests for rest_framework tests
joefarebrother Jul 29, 2024
24df548
Review suggestion - Add link to qldoc
joefarebrother Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add cookie models to each framework
  • Loading branch information
@joefarebrother
joefarebrother committed Jul 23, 2024
commit be87eb50d494704b901ddcaff53b4a876a72ee1a
45 changes: 45 additions & 0 deletions python/ql/lib/semmle/python/frameworks/Aiohttp.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,51 @@ module AiohttpWebModel {
override DataFlow::Node getNameArg() { result in [this.getArg(0), this.getArgByName("name")] }

override DataFlow::Node getValueArg() { result in [this.getArg(1), this.getArgByName("value")] }

override predicate hasSecureFlag(boolean b) {
super.hasSecureFlag(b)
or
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("secure") |
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
b = bool.booleanValue()
)
or
not exists(this.getArgByName("secure")) and
b = false
}

override predicate hasHttpOnlyFlag(boolean b) {
super.hasHttpOnlyFlag(b)
or
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("httponly") |
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
b = bool.booleanValue()
)
or
not exists(this.getArgByName("httponly")) and
b = false
}

override predicate hasSameSiteAttribute(Http::Server::CookieWrite::SameSiteValue v) {
super.hasSameSiteAttribute(v)
or
exists(DataFlow::Node arg, StringLiteral str | arg = this.getArgByName("samesite") |
DataFlow::localFlow(DataFlow::exprNode(str), arg) and
(
str.getText().toLowerCase() = "strict" and
v instanceof Http::Server::CookieWrite::SameSiteStrict
or
str.getText().toLowerCase() = "lax" and
v instanceof Http::Server::CookieWrite::SameSiteLax
or
str.getText().toLowerCase() = "none" and
v instanceof Http::Server::CookieWrite::SameSiteNone
)
)
or
not exists(this.getArgByName("samesite")) and
v instanceof Http::Server::CookieWrite::SameSiteLax // Lax is the default
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code seems to be copied across a bunch of frameworks. Would it make sense to create a SetCookieCall abstraction so the frameworks just have to point out where the calls are? It seems all calls to set_cookie (in any framework) are likely to follow this convention (and if not, it can be overridden by the framework).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it would make sense. Should it be defined in the Concepts library?

Copy link
Contributor

@yoff yoff Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, since it comes from the protocol (something like here) it should probably not be in a specific framework, so if it could be in Concepts, that would be nice.

}

/**
Expand Down
2 changes: 1 addition & 1 deletion python/ql/lib/semmle/python/frameworks/Django.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2220,7 +2220,7 @@ module PrivateDjango {
str.getText().toLowerCase() = "strict" and
v instanceof Http::Server::CookieWrite::SameSiteStrict
or
str.getText().toLowerCase() = "strict" and
str.getText().toLowerCase() = "lax" and
v instanceof Http::Server::CookieWrite::SameSiteLax
or
str.getText().toLowerCase() = "none" and
Expand Down
45 changes: 45 additions & 0 deletions python/ql/lib/semmle/python/frameworks/FastApi.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,51 @@ module FastApi {
override DataFlow::Node getValueArg() {
result in [this.getArg(1), this.getArgByName("value")]
}

override predicate hasSecureFlag(boolean b) {
super.hasSecureFlag(b)
or
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("secure") |
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
b = bool.booleanValue()
)
or
not exists(this.getArgByName("secure")) and
b = false
}

override predicate hasHttpOnlyFlag(boolean b) {
super.hasHttpOnlyFlag(b)
or
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("httponly") |
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
b = bool.booleanValue()
)
or
not exists(this.getArgByName("httponly")) and
b = false
}

override predicate hasSameSiteAttribute(Http::Server::CookieWrite::SameSiteValue v) {
super.hasSameSiteAttribute(v)
or
exists(DataFlow::Node arg, StringLiteral str | arg = this.getArgByName("samesite") |
DataFlow::localFlow(DataFlow::exprNode(str), arg) and
(
str.getText().toLowerCase() = "strict" and
v instanceof Http::Server::CookieWrite::SameSiteStrict
or
str.getText().toLowerCase() = "lax" and
v instanceof Http::Server::CookieWrite::SameSiteLax
or
str.getText().toLowerCase() = "none" and
v instanceof Http::Server::CookieWrite::SameSiteNone
)
)
or
not exists(this.getArgByName("samesite")) and
v instanceof Http::Server::CookieWrite::SameSiteLax // Lax is the default
}
}

/**
Expand Down
2 changes: 1 addition & 1 deletion python/ql/lib/semmle/python/frameworks/Flask.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ module Flask {
str.getText().toLowerCase() = "strict" and
v instanceof Http::Server::CookieWrite::SameSiteStrict
or
str.getText().toLowerCase() = "strict" and
str.getText().toLowerCase() = "lax" and
v instanceof Http::Server::CookieWrite::SameSiteLax
or
str.getText().toLowerCase() = "none" and
Expand Down
45 changes: 45 additions & 0 deletions python/ql/lib/semmle/python/frameworks/Pyramid.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,51 @@ module Pyramid {
override DataFlow::Node getValueArg() {
result = [this.getArg(1), this.getArgByName("value")]
}

override predicate hasSecureFlag(boolean b) {
super.hasSecureFlag(b)
or
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("secure") |
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
b = bool.booleanValue()
)
or
not exists(this.getArgByName("secure")) and
b = false
}

override predicate hasHttpOnlyFlag(boolean b) {
super.hasHttpOnlyFlag(b)
or
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("httponly") |
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
b = bool.booleanValue()
)
or
not exists(this.getArgByName("httponly")) and
b = false
}

override predicate hasSameSiteAttribute(Http::Server::CookieWrite::SameSiteValue v) {
super.hasSameSiteAttribute(v)
or
exists(DataFlow::Node arg, StringLiteral str | arg = this.getArgByName("samesite") |
DataFlow::localFlow(DataFlow::exprNode(str), arg) and
(
str.getText().toLowerCase() = "strict" and
v instanceof Http::Server::CookieWrite::SameSiteStrict
or
str.getText().toLowerCase() = "lax" and
v instanceof Http::Server::CookieWrite::SameSiteLax
or
str.getText().toLowerCase() = "none" and
v instanceof Http::Server::CookieWrite::SameSiteNone
)
)
or
not exists(this.getArgByName("samesite")) and
v instanceof Http::Server::CookieWrite::SameSiteLax // Lax is the default
}
}
}

Expand Down
45 changes: 45 additions & 0 deletions python/ql/lib/semmle/python/frameworks/Tornado.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,5 +604,50 @@ module Tornado {
override DataFlow::Node getNameArg() { result in [this.getArg(0), this.getArgByName("name")] }

override DataFlow::Node getValueArg() { result in [this.getArg(1), this.getArgByName("value")] }

override predicate hasSecureFlag(boolean b) {
super.hasSecureFlag(b)
or
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("secure") |
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
b = bool.booleanValue()
)
or
not exists(this.getArgByName("secure")) and
b = false
}

override predicate hasHttpOnlyFlag(boolean b) {
super.hasHttpOnlyFlag(b)
or
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("httponly") |
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
b = bool.booleanValue()
)
or
not exists(this.getArgByName("httponly")) and
b = false
}

override predicate hasSameSiteAttribute(Http::Server::CookieWrite::SameSiteValue v) {
super.hasSameSiteAttribute(v)
or
exists(DataFlow::Node arg, StringLiteral str | arg = this.getArgByName("samesite") |
DataFlow::localFlow(DataFlow::exprNode(str), arg) and
(
str.getText().toLowerCase() = "strict" and
v instanceof Http::Server::CookieWrite::SameSiteStrict
or
str.getText().toLowerCase() = "lax" and
v instanceof Http::Server::CookieWrite::SameSiteLax
or
str.getText().toLowerCase() = "none" and
v instanceof Http::Server::CookieWrite::SameSiteNone
)
)
or
not exists(this.getArgByName("samesite")) and
v instanceof Http::Server::CookieWrite::SameSiteLax // Lax is the default
}
}
}
43 changes: 43 additions & 0 deletions python/ql/lib/semmle/python/frameworks/Twisted.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,49 @@ private module Twisted {
override DataFlow::Node getNameArg() { result in [this.getArg(0), this.getArgByName("k")] }

override DataFlow::Node getValueArg() { result in [this.getArg(1), this.getArgByName("v")] }

override predicate hasSecureFlag(boolean b) {
super.hasSecureFlag(b)
or
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("secure") |
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
b = bool.booleanValue()
)
or
not exists(this.getArgByName("secure")) and
b = false
}

override predicate hasHttpOnlyFlag(boolean b) {
super.hasHttpOnlyFlag(b)
or
exists(DataFlow::Node arg, BooleanLiteral bool | arg = this.getArgByName("httponly") |
DataFlow::localFlow(DataFlow::exprNode(bool), arg) and
b = bool.booleanValue()
)
or
not exists(this.getArgByName("httponly")) and
b = false
}

override predicate hasSameSiteAttribute(Http::Server::CookieWrite::SameSiteValue v) {
super.hasSameSiteAttribute(v)
or
exists(DataFlow::Node arg, StringLiteral str | arg = this.getArgByName("samesite") |
DataFlow::localFlow(DataFlow::exprNode(str), arg) and
(
str.getText().toLowerCase() = "strict" and
v instanceof Http::Server::CookieWrite::SameSiteStrict
or
str.getText().toLowerCase() = "lax" and
v instanceof Http::Server::CookieWrite::SameSiteLax
// sting "none" is not accepted
)
)
or
not exists(this.getArgByName("samesite")) and
v instanceof Http::Server::CookieWrite::SameSiteLax // Lax is the default
}
}

/**
Expand Down