Skip to content
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

ParseDN does not correctly handle base64 encoded values #416

Closed
TomSellers opened this issue Feb 8, 2023 · 4 comments · Fixed by #425
Closed

ParseDN does not correctly handle base64 encoded values #416

TomSellers opened this issue Feb 8, 2023 · 4 comments · Fixed by #425

Comments

@TomSellers
Copy link
Contributor

In the currently released code ParseDN doesn't correctly handle a base64 encoded attribute value when all three of the following are true:

  • the value ends in two equal signs
  • the attribute / value pair is not the last entry in a string
  • the equal signs are NOT escaped (this is not required by RFC)

This was observed when parsing dnQualifier attributes in a string produced by a 3rd party library.

Example code

Playground link: https://go.dev/play/p/yAeqSlL6rY4

package main

import (
	"fmt"

	ldap "github.com/go-ldap/ldap/v3"
)

func main() {

	// First test, double '=' at the end of the string
	sample1 := "example1=ZXhhbXBsZVRleHQ=,example2=ZXhhbXBsZVRleHQxMQ=="

	if _, err := ldap.ParseDN(sample1); err != nil {
		fmt.Printf("sample1 failed with err: %s\n", err)
	} else {
		fmt.Println("sample1 passed!")
	}

	// Second test, double '=' in the middle of the string
	sample2 := "example2=ZXhhbXBsZVRleHQxMQ==, example1=ZXhhbXBsZVRleHQ="
	if _, err := ldap.ParseDN(sample2); err != nil {
		fmt.Printf("sample2 failed with err: %s\n", err)
	} else {
		fmt.Println("sample2 passed!")
	}

	// Third test, double '=' in the middle of the string but they are escaped
	sample3 := "example2=ZXhhbXBsZVRleHQxMQ\\=\\=, example1=ZXhhbXBsZVRleHQ\\="
	if _, err := ldap.ParseDN(sample3); err != nil {
		fmt.Printf("sample3 failed with err: %s\n", err)
	} else {
		fmt.Println("sample3 passed!")
	}
}

Example results

sample1 passed!
sample2 failed with err: incomplete type, value pair
sample3 passed!
@johnweldon
Copy link
Member

Thanks for reporting this - we should add this test case to our tests when we address this.

@TomSellers
Copy link
Contributor Author

It seems that it doesn't handle a single = sign in the middle of a string correctly in some cases either. For example, in the example code below it turns EXAMPLE1=ZXhhbXBsZVRleHQ=,CN=foo.bar into zxhhbxbszvrlehq=,cn=foo.barwhich meant that it stripped the attribute name, EXAMPLE1 from the string. It also changed the casing of the value but I think that is a side effect treating it like the attribute name.

Example code

Playground link: https://go.dev/play/p/-4ShTySaYOW

package main

import (
	"fmt"

	ldap "github.com/go-ldap/ldap/v3"
)

func main() {

	// First test, double '=' at the end of the string
	sample1 := "example1=ZXhhbXBsZVRleHQ=,example2=ZXhhbXBsZVRleHQxMQ=="

	if _, err := ldap.ParseDN(sample1); err != nil {
		fmt.Printf("sample1 failed with err: %s\n", err)
	} else {
		fmt.Println("sample1 passed!")
	}

	// Second test, double '=' in the middle of the string
	sample2 := "example2=ZXhhbXBsZVRleHQxMQ==, example1=ZXhhbXBsZVRleHQ="
	if _, err := ldap.ParseDN(sample2); err != nil {
		fmt.Printf("sample2 failed with err: %s\n", err)
	} else {
		fmt.Println("sample2 passed!")
	}

	// Third test, double '=' in the middle of the string but they are escaped
	sample3 := "example2=ZXhhbXBsZVRleHQxMQ\\=\\=, example1=ZXhhbXBsZVRleHQ\\="
	if _, err := ldap.ParseDN(sample3); err != nil {
		fmt.Printf("sample3 failed with err: %s\n", err)
	} else {
		fmt.Println("sample3 passed!")
	}

	// Fourth test
	sample4 := "EXAMPLE1=ZXhhbXBsZVRleHQ=,CN=foo.bar"
	if res, err := ldap.ParseDN(sample4); err != nil {
		fmt.Printf("sample4 failed with err: %s\n", err)
	} else {
		fmt.Printf("sample4 passed!\n\toriginal: %s\n\tresult:   %s\n", sample4, res)
	}
}

Example results

sample1 passed!
sample2 failed with err: incomplete type, value pair
sample3 passed!
sample4 passed!
	original: EXAMPLE1=ZXhhbXBsZVRleHQ=,CN=foo.bar
	result:   zxhhbxbszvrlehq=,cn=foo.bar

@cpuschma
Copy link
Member

We could update the switch-case in ParseDN to ignore the = in case a type has already been declared (which is necessary in contrary to the value):

ldap/dn.go

Line 159 in b64a808

case char == '=':

becomes

case char == '=' && attribute.Type == "":

In that case, the next case match would be the default and add the char to the string buffer

sample1 passed! example1=ZXhhbXBsZVRleHQ=,example2=ZXhhbXBsZVRleHQxMQ==
sample2 passed! example2=ZXhhbXBsZVRleHQxMQ==, example1=ZXhhbXBsZVRleHQ=   
sample3 passed! example2=ZXhhbXBsZVRleHQxMQ\=\=, example1=ZXhhbXBsZVRleHQ\=
sample4 passed!                                                            
        original: EXAMPLE1=ZXhhbXBsZVRleHQ=,CN=foo.bar                     
        result:   example1=ZXhhbXBsZVRleHQ=,cn=foo.bar

james-d-elliott added a commit to james-d-elliott/ldap that referenced this issue Mar 27, 2023
This fixes an issue where attributes with equal chars in the value would not be correctly parsed. As we only set the AttributeTypeAndValue's Type field once and we reset it on the step to the next AttributeTypeAndValue we can assume if this field already has a value that it should not consider it the type/value separator.

Fixes go-ldap#416
james-d-elliott added a commit to james-d-elliott/ldap that referenced this issue Mar 27, 2023
This fixes an issue where attributes with equal chars in the value would not be correctly parsed. As we only set the AttributeTypeAndValue's Type field once and we reset it on the step to the next AttributeTypeAndValue we can assume if this field already has a value that it should not consider it the type/value separator.

Fixes go-ldap#416
@james-d-elliott
Copy link
Contributor

@cpuschma sorry I came up with the same solution, just realizing this now. Happy for you to take credit, and take my PR as me having the same findings. I am happy for you to PR it instead since technically you solved it first, not a big deal for me either way.

cpuschma pushed a commit that referenced this issue Mar 27, 2023
This fixes an issue where attributes with equal chars in the value would not be correctly parsed. As we only set the AttributeTypeAndValue's Type field once and we reset it on the step to the next AttributeTypeAndValue we can assume if this field already has a value that it should not consider it the type/value separator.

Fixes #416
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants