Skip to content

Python: CWE-117 Log injection #6182

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

Merged
yoff merged 5 commits into from
Oct 12, 2021
Merged

Python: CWE-117 Log injection #6182

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c60eded
Fix conflicting
haby0 Sep 15, 2021
9916753
Modify sinks
haby0 Sep 17, 2021
6c07a3e
Apply @yoff's suggestion
haby0 Sep 22, 2021
29ddc76
Update python/ql/test/experimental/query-tests/Security/CWE-117/LogIn…
haby0 Oct 11, 2021
d52f95d
Auto Formatting
haby0 Oct 12, 2021
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
Modify sinks
  • Loading branch information
@haby0
haby0 committed Sep 17, 2021
commit 99167539fb0291805b26ec2f9daefe99a1944992
36 changes: 31 additions & 5 deletions python/ql/src/experimental/semmle/python/frameworks/Log.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ private module log {
* See https://docs.python.org/3/library/logging.html#logger-objects
*/
private class LogOutputMethods extends string {
LogOutputMethods() { this in ["info", "error", "warn", "warning", "debug", "critical"] }
LogOutputMethods() {
this in ["info", "error", "warn", "warning", "debug", "critical", "exception", "log"]
}
}

/**
Expand All @@ -33,7 +35,13 @@ private module log {
this = API::moduleImport("logging").getMember(any(LogOutputMethods m)).getACall()
}

override DataFlow::Node getAnInput() { result = this.getArg(_) }
override DataFlow::Node getAnInput() {
this.getFunction().(DataFlow::AttrRead).getAttributeName() != "log" and
result = this.getArg(0)
or
this.getFunction().(DataFlow::AttrRead).getAttributeName() = "log" and
result = this.getArg(1)
}
Comment on lines +38 to +44
Copy link
Copy Markdown
Contributor

@yoff yoff Sep 22, 2021

Choose a reason for hiding this comment

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

Suggested change
override DataFlow::Node getAnInput() {
this.getFunction().(DataFlow::AttrRead).getAttributeName() != "log" and
result = this.getArg(0)
or
this.getFunction().(DataFlow::AttrRead).getAttributeName() = "log" and
result = this.getArg(1)
}
override DataFlow::Node getAnInput() {
this.getFunction().(DataFlow::AttrRead).getAttributeName() != "log" and
result in [this.getArg(_), this.getArgByName(_) ] // this includes the arg named "msg"
or
this.getFunction().(DataFlow::AttrRead).getAttributeName() = "log" and
result in [this.getArg(any(int i | i > 0)), this.getArgByName(any(string s | s != "level")]
}

I think my previous comment was a bit unclear, it should be something like the above.

If you have a signature like

warning(msg, *args, **kwargs)

you can call it with

warning("msg", "arg1", "arg2", kwarg1="kwarg1", kwargs="kwarg2")

and all of these arguments seem to end up in the log.

}

/**
Expand All @@ -51,7 +59,13 @@ private module log {
.getACall()
}

override DataFlow::Node getAnInput() { result = this.getArg(_) }
override DataFlow::Node getAnInput() {
this.getFunction().(DataFlow::AttrRead).getAttributeName() != "log" and
result = this.getArg(0)
or
this.getFunction().(DataFlow::AttrRead).getAttributeName() = "log" and
result = this.getArg(1)
}
}

/**
Expand All @@ -68,7 +82,13 @@ private module log {
.getACall()
}

override DataFlow::Node getAnInput() { result = this.getArg(_) }
override DataFlow::Node getAnInput() {
this.getFunction().(DataFlow::AttrRead).getAttributeName() != "log" and
result = this.getArg(0)
or
this.getFunction().(DataFlow::AttrRead).getAttributeName() = "log" and
result = this.getArg(1)
}
}

/**
Expand All @@ -87,6 +107,12 @@ private module log {
.getACall()
}

override DataFlow::Node getAnInput() { result = this.getArg(_) }
override DataFlow::Node getAnInput() {
this.getFunction().(DataFlow::AttrRead).getAttributeName() != "log" and
result = this.getArg(0)
or
this.getFunction().(DataFlow::AttrRead).getAttributeName() = "log" and
result = this.getArg(1)
}
}
}