Add query to detect out-of-order cpp/linux privilege dropping

[theopolis]

Hi folks, I am working on this query (my first!) to detect usage of setuid before setgid, initgroups, etc and when error checking is missing. This situation indicates an attempt to drop privileges incorrectly, and the most important part of the bug is the lack of error handling. This would effect setuid/setgid-bit executables and daemons run as root with the intention to later dropping privileges. I added inline comments about my thoughts on false positives and false negatives.

I think this category of checks can be extended, for example another query can be added to find instances where initgroups or setgroups (dropping supplemental groups) are missing or where capabilities are not dropped, etc. For this reason I created a subfolder for the query, test code, and helpfile.

I tried to create the small set of test cases to improve accuracy. I also browsed GitHub for about 100 projects that may be impacted, by looking for usage of setuid. At this point I am mildly confident this is working as intended. But this is my first time using CodeQL and I am sure it could use a lot of feedback. On my mind are: is this performance enough, am I using the QL stdlib correctly, can I simplify the logic, is there a way to test it on a larger set of projects?

[JLLeitschuh]

Very cool. I have a few scripts that can let you bulk import projects into your LGTM account so you can run this query against large numbers of projects.

I see you're not in the slack channel. I recommend reaching out to the GitHub Security Lab to get an invite. I'm more than happy to share them with you there.
https://twitter.com/ghsecuritylab

[rdmarsh2]

Thanks for your submission. This is really impressive for a first query.

I've made some suggestions about both performance and general approach, see the PR comments below.

For testing on a larger set of projects, you can use lgtm.com, which automatically builds CodeQL snapshots for a large set of open-source projects and makes them available for download or online querying through the query console. We can also run the query across all projects on lgtm.com, but that's typically one of the last steps in the process.

[rdmarsh2]

You could potentially use semmle.code.cpp.controlflow.Guards to require a check of errno, but handling wrapper functions would be a bit tricky.

[theopolis]

I am going to pass on this for the moment. I do not think this happens often (where someone ignores the result for specific values of errno). My goal with the comment was to highlight that this additional checking should be considered.

If we are able to run the query on a massive set of projects and find that adding a check here is worthwhile I am happy to follow up and explore adding a guard. :) (I am going to go learn more about guard implementations right now.)

[theopolis]

Wow, thank you so much for code review @rdmarsh2! It will take me some time to apply the feedback because I’d like to dig into some of what you’re mentioning.

[theopolis]

Alright, I re-tested this against my sample set of projects (n=100). All of these projects use setuid-like functions in one way or another.

Here are some examples of the follow up

• Lightning-fast improvements from LXC tree-wide: improve setgroups() dropping lxc/lxc#3265
• True-positive and fixed Privilege dropping code may not be working swaywm/sway#4990
• True-positive and fixed Extend privilege dropping implementation to set correct supplemental groups DNS-OARC/dnscap#208
• Created an issue, I will try to follow up (confirm impact/write PR) later: Check return setgroups result for safety darkk/redsocks#149
• Additional true-positives one dormant projects, will consider opening a PR later.

It is interesting if you take away the requirement for the result to flow into a condition. These results are more prone to false positives but are fun for code reviewing.

[JLLeitschuh]

Should these reports have CVE numbers assigned to them?

[theopolis]

Should these reports have CVE numbers assigned to them?

IMO no unless there's precedent for documenting similar issues. But I can break the issues into specifics:

• [best practice] Target user's supplemental groups are not initialized (or alternatively source groups are not dropped). Meaning initgroups or setgroups are not called.
• [best practice] Order of dropping is correct but return codes are not checked. (for example dimkr/loksh:misc.c#L298)
• [low priority weakness] Order is incorrect and return codes are not checked. This is what the query detects as written here.

The SwayVM case falls into the last category and I can see documenting that as a CVE. I will look for precedent this weekend and we can apply consistency.

[nicowaisman]

Hey @theopolis,
A couple of logistic questions. The PR actually need to happen on https://github.com/Semmle/ql where CodeQL main queries are hosted (In this query case, under cpp section).
Then we ask contributors, to create an issue in this repo to keep a track of each submission.

Let me know if you have any question

[theopolis]

Hi @nicowaisman,

The PR actually need to happen on https://github.com/Semmle/ql where CodeQL main queries are hosted (In this query case, under cpp section).

I am not confident enough that this query should be added to the the set of main queries. I'd rather keep it in the group of queries in this repo unless someone can run it across all of the LGTM projects and comment on the false positive rate.

Then we ask contributors, to create an issue in this repo to keep a track of each submission.

I was mostly interested in learning CodeQL and getting feedback on my query (which I have, and the feedback has been great!) Since this query finds issues that are focused on correctness around mitigations it feels a little below the bar for qualifying for a bounty. If you think it does I'd rather pursue the bug slayer template due to my concern above about confidence.

4. Open a pull request in the security-lab repo with a single CodeQL query. See the contribution guidelines for more details.
5. Create an issue using the bug slayer template. The issue should link to your pull request and contain a detailed report of the vulnerabilities your query finds. It should include a description of the vulnerabilities, their associated CVEs, and how the query allowed you to find them. Pull requests without an accompanying issue cannot be considered.

[JLLeitschuh]

The way the program is setup, you can actually do both with the same vuln.

You can use the query to find vulns in open source projects and get CVEs assigned for it (you need a minimum of 4 for Bug Slayer). That's the first BB program.

Then for the second, submit it to the Semmle/ql repo for the All For One and that's a second BB submission.

I'm doing that here with a query I already had accepted to the 'All-For-One' program:

1. All-For-One - Bountied - Java (Maven): Use of insecure protocol to download/upload artifacts #21
2. Bug Hunter - Pending - Java (Maven): Actually fix the use of insecure protocol to download/upload artifacts #38

[theopolis]

Ok so my next step is to open a similar PR for the ql repo. This may take several days.