Skip to content

Python : Add query to detect PAM authorization bypass #8595

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

Merged
merged 5 commits into from
May 10, 2022

Conversation

ghost
Copy link

@ghost ghost commented Mar 29, 2022

Using only a call to pam_authenticate to check the validity of a login can lead to authorization bypass vulnerabilities. A pam_authenticate only verifies the credentials of a user. It does not check if a user has an appropriate authorization to actually login. This means a user with a expired login or a password can still access the system.

This PR includes a qhelp describing the issue, a query which detects instances where a call to pam_acc_mgmt does not follow a call to pam_authenticate and it's corresponding tests.

Some of the public CVE's I can find using this query are :

Using only a call to `pam_authenticate` to check the validity of a login can
lead to authorization bypass vulnerabilities. A `pam_authenticate` only
verifies the credentials of a user. It does not check if a user has an
appropriate authorization to actually login. This means a user with a
expired login or a password can still access the system.

This PR includes a qhelp describing the issue, a query which detects instances where a call to
`pam_acc_mgmt` does not follow a call to `pam_authenticate` and it's
corresponding tests.

This PR has multiple detections. Some of the public one I can find are :
* [CVE-2022-0860](https://nvd.nist.gov/vuln/detail/CVE-2022-0860) found
in [cobbler/cobbler](https://www.github.com/cobbler/cobbler)
* [fredhutch/motuz](https://www.huntr.dev/bounties/d46f91ca-b8ef-4b67-a79a-2420c4c6d52b/)
@ghost ghost self-requested a review as a code owner March 29, 2022 19:18
@ghost
Copy link
Author

ghost commented Mar 29, 2022

@RasmusWL This should ideally also find CVE-2022-1049 found in clusterlabs/pcs and fixed in ClusterLabs/pcs@fb86000#diff-e8d00a7c356632c11156ad3e8ce897f66538c4cc4a849487f9a5ddc0a19eafb8L53

However, CodeQL does not seem to be able to trace the flow from the prep_fn function defined here

@RasmusWL
Copy link
Member

RasmusWL commented Apr 7, 2022

Thanks for this submission 👍 I'll take a look at it either tomorrow or next week.

Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

Besides inline comments, I would like you to merge the test files to a single, and remove all the stuff that is not absolutely required to make the example work (I highlight a bit of it, but I think there are more things that can be removed).

@ghost
Copy link
Author

ghost commented Apr 12, 2022

@RasmusWL Thanks for the quick response. I have made the changes you sought. Any thoughts on how I could handle the clusterlabs/pcs case I mentioned above?

@RasmusWL
Copy link
Member

RasmusWL commented Apr 13, 2022

I have made the changes you sought.

Thanks 🙏 I made an other minor adjustment, simplifying the code a little bit more. Hope you don't mind.

Any thoughts on how I could handle the clusterlabs/pcs case I mentioned above?

This is sadly a known problem, which is highlighted by this bit of test code. In the code below, a limitation from type trackers is that we don't treat also_x as a result from foo() 😞 So nothing you can do about it, but thanks for highlighting it 👍

def id(obj):
    return obj

x = foo()
also_x = id(x)

RasmusWL
RasmusWL previously approved these changes Apr 13, 2022
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

I think this looks ok now 👍

@ghost
Copy link
Author

ghost commented Apr 19, 2022

@RasmusWL Ping.

@RasmusWL
Copy link
Member

Thanks. I've just gotten back from vacation. Need to do a little internal testing of this before merging.

@ghost ghost mentioned this pull request Apr 19, 2022
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

Had a chance to look at some results, which looks good to me, so added @precision high. Will merge this PR once tests turn green.

@RasmusWL RasmusWL merged commit cb17e2a into github:main May 10, 2022
@ghost ghost deleted the pypam branch May 10, 2022 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant