PR#50642: Ticket - 50349 - additional fix: filter schema check must handle subtypes - 389-ds-base

389-ds-base

#50642 Ticket - 50349 - additional fix: filter schema check must handle subtypes

Closed 3 years ago by spichugi. Opened 4 years ago by lkrispen.

lkrispen/389-ds-base t50349 into master

Ticket - 50349 - additional fix: filter schema check must handle subtypes

Ludwig Krispenz • 4 years ago

b7d1118

ldap/servers/slapd/attrsyntax.c

file modified

+23 -1

		`@@ -394,11 +394,33 @@`
		`* The main reason to use this over attr_syntax_get_by_name_locking_optional is to`
		`* avoid the reference count increment/decrement cycle when we only need a boolean`
		`* of existance, rather than retriving the reference to the attribute itself.`
		`+ *`
		`+ * But we do need to strip subtypes`
		`*/`
		`int32_t`
		`attr_syntax_exist_by_name_nolock(char *name) {`
		`struct asyntaxinfo *asi = NULL;`
		`- asi = (struct asyntaxinfo *)PL_HashTableLookup_const(name2asi, name);`
		`+ char *check_name = NULL;`
tbordaz commented 4 years ago Nice finding. Would it be possible to use slapi_attr_basetype ?
		`+ char *p = NULL;`
		`+ int free_attr = 0;`
		`+`
		`+ /* Ignore any attribute subtypes. */`
		`+ if ((p = strchr(name, ';'))) {`
		`+ int check_len = p - name + 1;`
		`+`
		`+ check_name = (char *)slapi_ch_malloc(check_len);`
mreynolds commented 4 years ago It would be nice not to do this allocation, and free. Maybe we could use a static array (check_name[check_len + 1] = {0}) and memcpy the name in?
		`+ PR_snprintf(check_name, check_len, "%s", name);`
		`+ free_attr = 1;`
		`+ } else {`
		`+ check_name = name;`
		`+ }`
		`+`
		`+ asi = (struct asyntaxinfo *)PL_HashTableLookup_const(name2asi, check_name);`
		`+`
		`+ if (free_attr) {`
		`+ slapi_ch_free_string(&check_name);`
		`+ }`
		`+`
		`if (asi != NULL) {`
		`return 1;`
		`}`

lkrispen commented 4 years ago

Bug: if the filter did contain an attribute with a subtype eg givenname;lang-de
then the schema lookup failed.

Fix: The subtype needs to be removed befor asi lookup

Reviewed by: ?

tbordaz commented on line 12 of ldap/servers/slapd/attrsyntax.c 4 years ago

Nice finding. Would it be possible to use slapi_attr_basetype ?

mreynolds commented on line 20 of ldap/servers/slapd/attrsyntax.c 4 years ago

It would be nice not to do this allocation, and free. Maybe we could use a static array (check_name[check_len + 1] = {0}) and memcpy the name in?

lkrispen commented 4 years ago

Nice finding. Would it be possible to use slapi_attr_basetype ?

we could, but then we would have to handle the buffer and the case if the buffer was used or if memory was allocated and it wouldn't get nicer.

lkrispen commented 4 years ago

It would be nice not to do this allocation, and free. Maybe we could use a static array (check_name[check_len + 1] = {0}) and memcpy the name in?

but wouldn't this be a dynamic array based on len, how do you handle this without allocating if an initial size is passed ?

firstyear commented 4 years ago

Yeah, you should don't dynamic array allocs on stack. They have a lot of security risks. The alloc/free is fine. Thanks @lkrispen

tbordaz commented 4 years ago

I tend to agree with @firstyear, a local variable on the stack being allocated from attribute sent by a request looks more dangerous than a malloc.
THe patch looks good to me.

rebased onto b7d1118

4 years ago

Pull-Request has been merged by lkrispen

4 years ago

spichugi commented 3 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3697

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

3 years ago

Metadata

Assignee

None

Tags

No Tags

Changes Summary 1

+23 -1

file changed

ldap/servers/slapd/attrsyntax.c

389-ds-base

Source Code

#50642 Ticket - 50349 - additional fix: filter schema check must handle subtypes Closed 3 years ago by spichugi. Opened 4 years ago by lkrispen. lkrispen/389-ds-base t50349 into master

Metadata

Changes Summary 1

#50642 Ticket - 50349 - additional fix: filter schema check must handle subtypes

Closed 3 years ago by spichugi. Opened 4 years ago by lkrispen.

lkrispen/389-ds-base t50349 into master