-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Python: Promote py/pam-auth-bypass
#9108
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
Conversation
99% of our .qhelp files have manually wrapped lines, so just wanted to keep things consistent
They did not have proper formatting (only 2 spaces), and I restructured them a bit more so they look like code in the wild
Thought that calling out the actual vulnerability would make things easier for our end users :)
The value 8.1 was calculated by our internal tool. This corresponds to a 'High' severity, which from my gut feeling seems reasonable for authorization bypass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor bits and bobs that need fixing (most of them presented as suggestions for easy inclusion), but otherwise this looks good to me. 👍
@@ -0,0 +1,19 @@ | |||
libpam = CDLL(find_library("pam")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's... Quite the formatting. 😮
python/ql/test/query-tests/Security/CWE-285-PamAuthorization/PamAuthorization.expected
Outdated
Show resolved
Hide resolved
@@ -33,4 +34,5 @@ where | |||
acctMgmtCall = libPam().getMember("pam_acct_mgmt").getACall() and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not allowed to add a comment in the appropriate place, but there's a use of API::moduleImport("ctypes.util")
on line 20 above that will need fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, that probably what the merge conflict is also about 👍
Co-authored-by: Taus <tausbn@github.com>
This promotes the experimental query from #8595