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.

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

Python: CWE-079 - Add Email injection query #7127

Merged
merged 63 commits into from Jun 14, 2022

Conversation

mrthankyou
Copy link
Contributor

This PR introduces a new query that searches for reflected XSS email injections. This query specifically looks for vulnerabilities that utilize any one of these major Python email libraries:

The most common scenario this query looks for is a Python application that utilizes an email library to deliver emails. The Python application passes user input into an email's HTML body, which can result in a reflective XSS attack on the email recipient.

PS: Please note that both @mrthankyou and @jorgectf wrote this query. If accepted as a bug bounty we would like a 100% of the reward to go to @jorgectf. This will be re-iterated in the bug bounty submission.

mrthankyou and others added 30 commits June 21, 2021 19:01
Most of these query tests need to be cleaned up. Also, some of these query tests will fail because no user-tainted data is passing into the email bodies that are generated and sent to a victim user.
Besides removing comments, I also reduced the complexity of some of the Python code examples.
yoff
yoff previously approved these changes Mar 9, 2022
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, I think this is now acceptable to merge into experimental. However, you might want to hold off and retain your ability to address a potential high FP rate, in case you move forward with your bounty submission?

You currently only have the string comparison sanitizer. Is that how people would normally protect themselves?

@jorgectf
Copy link
Contributor

jorgectf commented Mar 9, 2022

Thank you for the changes, I think this is now acceptable to merge into experimental. However, you might want to hold off and retain your ability to address a potential high FP rate, in case you move forward with your bounty submission?

You currently only have the string comparison sanitizer. Is that how people would normally protect themselves?

Thank you for the suggestion! We've based this query's sanitizers on main's XSS query's (only StringConstCompare for now)

class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }

Would you suggest any other sanitizer? :)

@mrthankyou
Copy link
Contributor Author

mrthankyou commented Mar 9, 2022

This is an interesting question about sanitizers.

I did some research and only found one sanitizer that looks to be popular called Bleach. I don't think this is something the CodeQL library accounts for at this time. It has some popularity with 2k stars and it has 163k dependent repositories.

Bleach is pretty simple to use:

import bleach

bleach.clean('evil text') // this sanitizes text
bleach.linkify('evil text with a link: github.com') // this also sanitizes text

It might be worth it add this sanitizer. Idk. It looks to be a pretty simple step we can add in the query but at the same time I think Bleach also has a lot of weird ways to sanitize text which can impact how long this will take (see here). Also, this seems like something that should be baked into the CodeQL library and could deserve it's own PR.

@jorgectf
Copy link
Contributor

jorgectf commented Mar 9, 2022

Thank you for the changes, I think this is now acceptable to merge into experimental. However, you might want to hold off and retain your ability to address a potential high FP rate, in case you move forward with your bounty submission?
You currently only have the string comparison sanitizer. Is that how people would normally protect themselves?

Thank you for the suggestion! We've based this query's sanitizers on main's XSS query's (only StringConstCompare for now)

class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }

Would you suggest any other sanitizer? :)

Oops, I referenced (and took into account) the wrong query type. I've added HtmlEscaping as sanitizer :)

@jorgectf
Copy link
Contributor

jorgectf commented Mar 9, 2022

This is an interesting question about sanitizers.

I did some research and only found one sanitizer that looks to be popular called Bleach. I don't think this is something the CodeQL library accounts for at this time. It has some popularity with 2k stars and it has 163k dependent repositories.

Bleach is pretty simple to use:

import bleach

bleach.clean('evil text') // this sanitizes text
bleach.linkify('evil text with a link: github.com') // this also sanitizes text

It might be worth it add this sanitizer. Idk. It looks to be a pretty simple step we can add in the query but at the same time I think Bleach also has a lot of weird ways to sanitize text which can impact how long this will take (see here). Also, this seems like something that should be baked into the CodeQL library and could deserve it's own PR.

As discussed offline, we believe adding Bleach would require an additional PR given its complexity. We will work on that in the future in case this query arises many repos using Bleach for this :)

@yoff
Copy link
Contributor

yoff commented Mar 10, 2022

The HtmlEscaping is a good addition to the sanitizers. That concept can be made more powerful by adding a model of Bleach, but I agree that this can be done separately.

@yoff
Copy link
Contributor

yoff commented May 20, 2022

This submission has now been scored. Can I ask you to solve the conflicts and fix the expected test output so that we can get it merged? :-)

@mrthankyou
Copy link
Contributor Author

mrthankyou commented May 26, 2022

@yoff,

The merge conflict was resolved by overriding our branch's private import of RemoteFlowSources and making it public. If this should in fact be private let me know and I can make the change. Considering main has the import set to public I opted to use main's branch.

Also, sorry I missed the mention of expected test output. I haven't looked at the expected test output. I'll take a look and see how to fix it. Sorry for alerting you to review this PR when it's actually not ready :(.

@mrthankyou mrthankyou requested a review from yoff May 26, 2022 20:29
private DataFlow::Node getSMTPSubscriptByIndex(DataFlow::CallCfgNode sendCall, string index) {
exists(DefinitionNode def, Subscript sub |
sub = def.getNode() and
DataFlow::exprNode(sub.getObject()).getALocalSource() =
[sendCall.getArg(2), sendCall.getArg(2).(DataFlow::MethodCallNode).getObject()]
.getALocalSource() and
sub.getIndex().(StrConst).getText() = index and
result.asCfgNode() = def.getValue()
)
}

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase. Warning

Acronyms in getSMTPSubscriptByIndex should be PascalCase/camelCase
private class SMTPMessageConfig extends TaintTracking2::Configuration {
SMTPMessageConfig() { this = "SMTPMessageConfig" }

override predicate isSource(DataFlow::Node source) { source = mimeText(_) }

override predicate isSink(DataFlow::Node sink) {
sink = smtpMimeMultipartInstance().getACall().getArgByName("_subparts")
}
}

Check warning

Code scanning / CodeQL

Acronyms should be PascalCase/camelCase. Warning

Acronyms in SMTPMessageConfig should be PascalCase/camelCase
yoff
yoff previously approved these changes May 30, 2022
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

LGTM

@yoff
Copy link
Contributor

yoff commented May 30, 2022

Considering main has the import set to public I opted to use main's branch.

It should probably be private, but I do not think you have to fix that in this PR..

@yoff
Copy link
Contributor

yoff commented Jun 1, 2022

It seems the dead code has to be removed before we can merge. I do not have permission to do it for you, but I have created suggestions that you should be able to simply accept.

Co-authored-by: yoff <lerchedahl@gmail.com>
@jorgectf
Copy link
Contributor

jorgectf commented Jun 1, 2022

Thanks @yoff, done!

@yoff
Copy link
Contributor

yoff commented Jun 1, 2022

Hm, you have to format FlaskMail.qll and Sendgrid.qll, my suggestions must have left some spaces or something...

@jorgectf
Copy link
Contributor

jorgectf commented Jun 9, 2022

Friendly ping @yoff :D

Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

Thanks

@yoff yoff merged commit 6997618 into github:main Jun 14, 2022
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants