Skip to content

Python: Add LDAP Insecure Authentication query #5445

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.

Sign up for GitHub

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
Merged
merged 23 commits into from
Sep 23, 2021
Merged

Python: Add LDAP Insecure Authentication query #5445

merged 23 commits into from
Sep 23, 2021

Conversation

jorgectf
Copy link
Contributor

@jorgectf jorgectf commented Mar 18, 2021 •
edited
Loading

Any suggestions/tricks to improve the query or workarounds (to learn), no matter how minimal they are, are massively appreciated.

@jorgectf jorgectf requested a review from a team as a code owner March 18, 2021 16:42
@jorgectf jorgectf mentioned this pull request Mar 18, 2021
Closed
1 task
@jorgectf jorgectf marked this pull request as draft April 8, 2021 22:57
@RasmusWL
Copy link
Member

RasmusWL commented May 5, 2021

The intent of this PR, alerting when not using SSL/TLS for connecting to a remote LDAP server, looks good to me 👍

Without being an expert in this area, I think this should have a @problem.severity warning. But let's discuss when you're ready to work on this again 😊

@jorgectf
Copy link
Contributor Author

jorgectf commented May 7, 2021

The intent of this PR, alerting when not using SSL/TLS for connecting to a remote LDAP server, looks good to me 👍

Great! I drafted it because it can be ported to ApiGraphs. I will do it soon 👍

Without being an expert in this area, I think this should have a @problem.severity warning. But let's discuss when you're ready to work on this again 😊

I agree ^^

@jorgectf
Copy link
Contributor Author

I do have an idea of how to rewrite this query using #5444 new stuff, so I will wait for that query to be merged @RasmusWL :)

@jorgectf jorgectf marked this pull request as ready for review July 22, 2021 16:43
@jorgectf
Copy link
Contributor Author

This PR is ready for code review 😃 @RasmusWL

@RasmusWL RasmusWL self-assigned this Aug 26, 2021
RasmusWL
RasmusWL reviewed Aug 26, 2021
View reviewed changes
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.

Just took a quick glance at a few parts of the code (so not a full review by any means). I found a few minor things that I thought I might as well point out now.

@RasmusWL
Copy link
Member

I also think the .expected files needs to be updated. I've just started the tests on this PR, so they will probably fail 🤷

jorgectf and others added 2 commits August 26, 2021 12:20
@jorgectf @RasmusWL
Apply suggestions from code review
d458464 
Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
@jorgectf
Update .expected
786edb7
@jorgectf
Copy link
Contributor Author

I also think the .expected files needs to be updated. I've just started the tests on this PR, so they will probably fail 🤷

My bad! Thanks for the suggestions, the .expected has just been updated :)

@kevinbackhouse
Copy link
Contributor

Would it be possible to add qhelp to this query? I think that will help people to understand how to fix the bug. It's not obvious from the error message that the solution is to enable TLS.

@jorgectf
Copy link
Contributor Author

Would it be possible to add qhelp to this query? I think that will help people to understand how to fix the bug. It's not obvious from the error message that the solution is to enable TLS.

Of course, working on it now :)

RasmusWL
RasmusWL requested changes Sep 1, 2021
View reviewed changes
Copy link
Member

@RasmusWL RasmusWL left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall looks good 👍

I'm a bit worried that this query will not be able to flag "ldap://" + os.getenv("LDAP_HOST"). I think you might be able to simplify your source definition to source.(StrConst).getText().matches("ldap://"), and then let the default taint steps take care of any form of string concatenation. Not sure if there is a good reason you didn't do this, would be happy to hear. See new comment below

Besides that, a few minor things that could be improved (some of which probably fall into the nitpicky side of things)

@RasmusWL
Copy link
Member

RasmusWL commented Sep 5, 2021 •
edited
Loading

I'm a bit worried that this query will not be able to flag "ldap://" + os.getenv("LDAP_HOST"). I think you might be able to simplify your source definition to source.(StrConst).getText().matches("ldap://"), and then let the default taint steps take care of any form of string concatenation. Not sure if there is a good reason you didn't do this, would be happy to hear.

It just dawned upon me that this is might be a bad suggestion, since this completely goes against your efforts on reducing FPs from private IPs such as 127.0.0.1 😐 So this becomes a tradeoff between having more results or lower FP rate. I'm curios to hear if you had seen data that suggested it was important to ignore private IPs, or if it just felt like the right thing to do? 😊

jorgectf and others added 2 commits September 7, 2021 18:37
@jorgectf
Copy link
Contributor Author

jorgectf commented Sep 7, 2021

I'm a bit worried that this query will not be able to flag "ldap://" + os.getenv("LDAP_HOST"). I think you might be able to simplify your source definition to source.(StrConst).getText().matches("ldap://"), and then let the default taint steps take care of any form of string concatenation. Not sure if there is a good reason you didn't do this, would be happy to hear.

It just dawned upon me that this is might be a bad suggestion, since this completely goes against your efforts on reducing FPs from private IPs such as 127.0.0.1 😐 So this becomes a tradeoff between having more results or lower FP rate. I'm curios to hear if you had seen data that suggested it was important to ignore private IPs, or if it just felt like the right thing to do? 😊

It felt the right thing to do while reviewing Java's similar query 👍

RasmusWL
RasmusWL approved these changes Sep 23, 2021
View reviewed changes
Copy link
Member

@RasmusWL RasmusWL left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the fixes 👍 (apologies about it taking so long to review 😬)

@RasmusWL RasmusWL merged commit f14e3f6 into github:main Sep 23, 2021
@jorgectf
Copy link
Contributor Author

Absolutely no problem @RasmusWL. Thank you for reviewing the PR and helping me as usual :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RasmusWL RasmusWL RasmusWL approved these changes

Assignees

@RasmusWL RasmusWL

Labels
documentation Python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jorgectf @RasmusWL @kevinbackhouse