Go: Add Improper LDAP Authentication query (CWE-287)

[maikypedia]

This pull request adds a query for Improper LDAP Authentication to prevent attackers use an empty password. I am not very familiar with CodeQL Go, and I have been struggling to generate correct expected files 🙃.

Looking forward to your suggestions.

[mbg]

Hi @maikypedia, thank you for opening this pull request!

Looking at the documentation for Bind, it seems that an empty password is not accepted by that method. Would it be correct in saying that this vulnerability cannot be exploited if that is the case?

[maikypedia]

Hi @maikypedia, thank you for opening this pull request!

Looking at the documentation for Bind, it seems that an empty password is not accepted by that method. Would it be correct in saying that this vulnerability cannot be exploited if that is the case?

Hi @mbg , I didn't realize that in v3 empty password option with Bind is disabled, sorry for the inconvenience 😅

[maikypedia]

Would it be worth adding Bind as sink for v2 and UnauthenticatedBind for v3? As a warning query.

[maikypedia]

ping @mbg

[mbg]

Hi @maikypedia, sorry, I must have missed your question. Thank you for pinging me to bring this to my attention.

For Bind in v2, the query does make sense so it could be worth adding and I would be happy to review that.

For UnauthenticatedBind, I am not sure it makes sense to add a query to detect its use. The name of the method already notes that it is unauthenticated so using it would have to be deliberate and intentional.

[owen-mc]

Your test isn't giving any results - it should be finding the bad cases. You may need to make sure that the test file builds correctly (try running go build in the directory that it is in to see what the errors are).

[github-actions]

QHelp previews:

go/ql/src/experimental/CWE-287/ImproperLdapAuth.qhelp

Improper LDAP Authentication

If an LDAP connection uses user-supplied data as password, anonymous bind could be caused using an empty password to result in a successful authentication.

Recommendation

Don't use user-supplied data as password while establishing an LDAP connection.

Example

In the following examples, the code accepts a bind password via a HTTP request in variable bindPassword. The code builds a LDAP query whose authentication depends on user supplied data.

package main

import (
	"fmt"
	"log"
)

func bad() interface{} {
	bindPassword := req.URL.Query()["password"][0]

	// Connect to the LDAP server
	l, err := ldap.Dial("tcp", fmt.Sprintf("%s:%d", "ldap.example.com", 389))
	if err != nil {
		log.Fatalf("Failed to connect to LDAP server: %v", err)
	}
	defer l.Close()

	err = l.Bind("cn=admin,dc=example,dc=com", bindPassword)
	if err != nil {
		log.Fatalf("LDAP bind failed: %v", err)
	}
}

In the following examples, the code accepts a bind password via a HTTP request in variable bindPassword. The function ensures that the password provided is not empty before binding.

package main

import (
	"fmt"
	"log"
)

func good() interface{} {
	bindPassword := req.URL.Query()["password"][0]

	// Connect to the LDAP server
	l, err := ldap.Dial("tcp", fmt.Sprintf("%s:%d", "ldap.example.com", 389))
	if err != nil {
		log.Fatalf("Failed to connect to LDAP server: %v", err)
	}
	defer l.Close()

	if bindPassword != "" {
		err = l.Bind("cn=admin,dc=example,dc=com", bindPassword)
		if err != nil {
			log.Fatalf("LDAP bind failed: %v", err)
		}
	}
}

References

• MITRE: CWE-287: Improper Authentication.
• Common Weakness Enumeration: CWE-287.

[owen-mc]

The following files need to be reformatted using gofmt or have compilation errors:
Error: ./ql/src/experimental/CWE-287/examples/LdapAuthGood.go:1:1: expected 'package', found 'func'
Error: make: *** [Makefile:35: check-formatting] Error 1
Error: ./ql/src/experimental/CWE-287/examples/LdapAuthBad.go:1:1: expected 'package', found 'func'

[owen-mc]

Well done for generating stubs. You've actually put them in two places by mistake - you can remove all changes to go/vendor and keep the ones in the test folder. Your tests now generate the right results now, but they will also generate some build errors because of missing return statements. Please address that and the rest of my previous review comments.

[maikypedia]

I don't know if I've forgotten something, I think that's all for now. 😗

[owen-mc]

Please don't make any changes to go/vendor/modules.txt. Please add a package declaration and imports to make LdapAuthBad.go and LdapAuthGood.go valid go files.

[maikypedia]

Hi @owen-mc ! Sorry for the delay, I had some exams these weeks 😅 In the example files of other experimental queries they do not include the imports of libraries that are not builtin, should I leave it like that?

[owen-mc]

@maikypedia The reason for making these valid go files is so that gofmt can parse them, because it can highlight some syntax errors. It seems to have passed. I see now that imports aren't actually needed, just the package directive. I would leave it as it is.

Your test is failing. It couldn't build the go file. I think there is a missing go.mod file.

[maikypedia]

@maikypedia The reason for making these valid go files is so that gofmt can parse them, because it can highlight some syntax errors. It seems to have passed. I see now that imports aren't actually needed, just the package directive. I would leave it as it is.

Your test is failing. It couldn't build the go file. I think there is a missing go.mod file.

Done :), thanks

[owen-mc]

I don't think this is ever used. If that is the case then please delete it.

[maikypedia]

Done 😃 👍