Sitelet https://web.archive.org/web/20211030235325im_/https://github.com/github/codeql/pull/6948
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

Jump to bottom

CPP: Add query for CWE-243 Creation of chroot Jail Without Changing Working Directory #6948

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

CPP: Add query for CWE-243 Creation of chroot Jail Without Changing Working Directory #6948

wants to merge 6 commits into from

Conversation

Reviewers

@MathiasVP MathiasVP

At least 1 approving review is required to merge this pull request.

Assignees

@MathiasVP MathiasVP

Labels
C++ documentation
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
@ihsinme @MathiasVP
@ihsinme
Copy link
Contributor

@ihsinme ihsinme commented Oct 25, 2021

The request looks for situations of incorrect work with the setting of the working directory. first of all, this is the lack of checking the return value by the set function, secondly, it is the lack of setting after using the chroot call.

CVE-2008-5110

links to real work results, I will add later. I am currently working on them with developers.
ihsinme added 2 commits Oct 25, 2021
@ihsinme
Add files via upload
baec186
@ihsinme
Add files via upload
5d5d6bc
@ihsinme ihsinme requested a review from as a code owner Oct 25, 2021
@MathiasVP MathiasVP self-assigned this Oct 25, 2021
MathiasVP
MathiasVP reviewed Oct 26, 2021
View changes
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Hi @ihsinme. Here are my first review comments.

cpp/ql/src/experimental/Security/CWE/CWE-243/IncorrectChangingWorkingDirectory.ql
not exists(ConditionalStmt cotmp | cotmp.getControllingExpr().getAChild*() = fc) and
not exists(Loop lptmp | lptmp.getCondition().getAChild*() = fc) and
not exists(ReturnStmt rttmp | rttmp.getExpr().getAChild*() = fc) and
not exists(Assignment astmp | astmp.getAChild*() = fc) and
not exists(Initializer ittmp | ittmp.getExpr().getAChild*() = fc) and
Copy link
Contributor

@MathiasVP MathiasVP Oct 26, 2021

If you just want to ensure that the return value of fc isn't checked, you can replace all of these conditions with this:

Suggested change
not exists(ConditionalStmt cotmp | cotmp.getControllingExpr().getAChild*() = fc) and
not exists(Loop lptmp | lptmp.getCondition().getAChild*() = fc) and
not exists(ReturnStmt rttmp | rttmp.getExpr().getAChild*() = fc) and
not exists(Assignment astmp | astmp.getAChild*() = fc) and
not exists(Initializer ittmp | ittmp.getExpr().getAChild*() = fc) and
fc instanceof ExprInVoidContext

Copy link
Contributor Author

@ihsinme ihsinme Oct 28, 2021

Thanks for the suggestion.
I'll take a look at the tests and accept it.

cpp/ql/src/experimental/Security/CWE/CWE-243/IncorrectChangingWorkingDirectory.ql
fctmp.getASuccessor*() = fcp or
fcp.getASuccessor*() = fctmp
Copy link
Contributor

@MathiasVP MathiasVP Oct 26, 2021

You don't have to fix this, but I thought I should mention this:

Like I mentioned here, using Expr.getASuccessor*() (instead of BasicBlock.getASuccessor*()) likely means that your query may perform poorly on large projects. It may not be a problem since the optimizer may be able to help you out, but if we're ever to promote your query out of the experimental directory, this is something that we have to fix. If you do that fix for us, it may end up with a better final score.

Copy link
Contributor Author

@ihsinme ihsinme Oct 28, 2021

i will work on it

cpp/ql/src/experimental/Security/CWE/CWE-243/IncorrectChangingWorkingDirectory.ql Outdated Show resolved Hide resolved
cpp/ql/src/experimental/Security/CWE/CWE-243/IncorrectChangingWorkingDirectory.ql Show resolved Hide resolved
cpp/ql/src/experimental/Security/CWE/CWE-243/IncorrectChangingWorkingDirectory.ql Show resolved Hide resolved
cpp/ql/src/experimental/Security/CWE/CWE-243/IncorrectChangingWorkingDirectory.ql Outdated Show resolved Hide resolved
cpp/ql/src/experimental/Security/CWE/CWE-243/IncorrectChangingWorkingDirectory.ql Outdated Show resolved Hide resolved
cpp/ql/src/experimental/Security/CWE/CWE-243/IncorrectChangingWorkingDirectory.ql Outdated Show resolved Hide resolved
@ihsinme
Copy link
Contributor Author

@ihsinme ihsinme commented Oct 27, 2021

Good afternoon.
thanks for your comments.
I apologize for the delay in my response.
I will try to answer in the coming days.

ihsinme and others added 4 commits Oct 28, 2021
@ihsinme @MathiasVP
Apply suggestions from code review
c3b1d7e 
Co-authored-by: Mathias Vorreiter Pedersen <mathiasvp@github.com>
@ihsinme
Update IncorrectChangingWorkingDirectory.ql
0addb2d
@ihsinme
Update IncorrectChangingWorkingDirectory.ql
635a668
@ihsinme
Update IncorrectChangingWorkingDirectory.ql
3161d11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment