Python: Add LDAP Improper Authentication query

[jorgectf]

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

[RasmusWL]

After reading up on LDAP, I don't quite agree about this query. As pointed out by your own #5443, all user-input should be escaped before being used to construct DNs/filters, so the examples auth_good_2.py and auth_good_3.py doesn't look good to me, since they still contain vulnerabilities.

I think there is value in finding places where the bind operation is being used in an insecure way, for example by allowing the password to be empty/non-existent. However, in it's current form, I don't think I would accept this query. If I've misunderstood something, please do let know though 👍

[jorgectf]

all user-input should be escaped before being used to construct DNs/filters, so the examples auth_good_2.py and auth_good_3.py doesn't look good to me, since they still contain vulnerabilities.

You are right, the tests are focused on the actual vulnerability/bad practice (using None, an empty string or not specifying a password in a bind process)

I think there is value in finding places where the bind operation is being used in an insecure way, for example by allowing the password to be empty/non-existent. However, in it's current form, I don't think I would accept this query. If I've misunderstood something, please do let know though 👍

I had to copy most of the structure from #5443, but this query can be very much simplified using that PR's .qll and just checking the arguments regarding the password set in the bind function.

How would you see this query fine for a PR? I'm open to additions 😀

[RasmusWL]

I see that this query basically does what SonarSource does in https://rules.sonarsource.com/python/type/Vulnerability/RSPEC-4433.

I agree that authenticating with any service (internal/external) with only a username and no password is bad practice.

However, I think such a query does fall on the simple side, where most cases could be found by just using grep.

If you do go ahead with this PR, I do think you need to only focus on the calls to bind, and no so much on what kind of LDAP operations are made after the bind 😊

[RasmusWL]

Thanks for letting me know 👍 I've started tests, and will do a proper review in the coming days 👍

[RasmusWL]

I think this PR looks ok. I did a few minor changes, which I pushed directly to this PR. They were so small that I felt it was easier just to fix them up myself 😊

This PR is going through a bit of internal process, but if everything goes according to plan, it should be merged within the next few days +1

I'm not 100% convinced about the qhelp since it talks about strong password for executing a query the user controls. I don't see how taking a password from an environment variable helps in this case. I think the original sonarsource query that you link to simply wants all LDAP connections to use passwords.

I'm also not 100% happy that the "secure" examples are vulnerable to LDAP injection 😅

I'm not necessarily expecting you to fix these up, just noting it down if we want to promote this query to our standard query set 👍

[jorgectf]

I think this PR looks ok. I did a few minor changes, which I pushed directly to this PR. They were so small that I felt it was easier just to fix them up myself 😊

Thanks for that! 😊

I'm not 100% convinced about the qhelp since it talks about strong password for executing a query the user controls. I don't see how taking a password from an environment variable helps in this case. I think the original sonarsource query that you link to simply wants all LDAP connections to use passwords.

You are right, the main point of improper authentication is the fact of not using a password (or an empty one), so I'm going to change the qhelp to strictly focus on this point.

I'm also not 100% happy that the "secure" examples are vulnerable to LDAP injection 😅

Haha, I wanted to focus on showing the binding process, but since the code is an example of secure code, I'm going to escape user input and change the comments accordingly.

I'm not necessarily expecting you to fix these up, just noting it down if we want to promote this query to our standard query set 👍

I want the query to be the most accurate possible (and so learning through the process), so I have no problem on making any changes needed. Thanks a lot for your review and suggestions 😄

[RasmusWL]

Awesome, thanks 💪

[kevinbackhouse]

@jorgectf: Please could you take a look at these results on Bladefidz/mongodb-university: https://lgtm.com/query/9086417468810526559/
They look like false positives to me because I see username/passwords in all of them. Other projects that I have looked at have very similar results.

[jorgectf]

@jorgectf: Please could you take a look at these results on Bladefidz/mongodb-university: https://lgtm.com/query/9086417468810526559/
They look like false positives to me because I see username/passwords in all of them. Other projects that I have looked at have very similar results.

Thanks @kevinbackhouse for the FP catch! The problem was that I didn't realize .matches("%bind%") would also take unbinding operations. It should be fixed in 2f9e645. Apologies for that!

Sorry @RasmusWL for another review cancel 😅.

[RasmusWL]

Just fixed up merge conflict

[jorgectf]

Just fixed up merge conflict

Thanks!

[w2n1ck]

Nice, We've already encountered similar bug