Python: Add LDAP Insecure Authentication query

[jorgectf]

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

[RasmusWL]

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]

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]

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]

This PR is ready for code review 😃 @RasmusWL

[RasmusWL]

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]

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]

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]

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]

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]

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]

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]

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]

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

[jorgectf]

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