URL: https://github.com/SSSD/sssd/pull/192
Title: #192: Add certificate mapping library
lslebodn commented:
"""
There is a valgring error in unit test.
```
==11568== Invalid read of size 1
==11568== at 0x4C32CC4: strcmp (vg_replace_strmem.c:842)
==11568== by 0x50412EA: _assert_string_equal (in
/usr/lib64/libcmocka.so.0.4.0)
==11568== by 0x401F3E: test_sss_cert_get_content_2 (test_certmap.c:815)
==11568== by 0x5041968: ??? (in /usr/lib64/libcmocka.so.0.4.0)
==11568== by 0x5042250: _cmocka_run_group_tests (in
/usr/lib64/libcmocka.so.0.4.0)
==11568== by 0x401631: main (test_certmap.c:1305)
==11568== Address 0xb9deaee is 0 bytes after a block of size 110 alloc'd
==11568== at 0x4C2EB1B: malloc (vg_replace_malloc.c:299)
==11568== by 0x5AFF26E: _talloc_memdup (in /usr/lib64/libtalloc.so.2.1.9)
==11568== by 0x5D1380F: add_string_other_name_to_san_list
(sss_cert_content_nss.c:434)
==11568== by 0x5D1380F: get_san (sss_cert_content_nss.c:790)
==11568== by 0x5D142AC: sss_cert_get_content (sss_cert_content_nss.c:986)
==11568== by 0x401B0A: test_sss_cert_get_content_2 (test_certmap.c:758)
==11568== by 0x5041968: ??? (in /usr/lib64/libcmocka.so.0.4.0)
==11568== by 0x5042250: _cmocka_run_group_tests (in
/usr/lib64/libcmocka.so.0.4.0)
==11568== by 0x401631: main (test_certmap.c:1305)
==11568==
```
I think taht problem is in uni test; because it tries to compare
`item->bin_val` with a string. But IIUC `bin_val` is not null terminated.
We should use `assert_memory_equal`
+ there are few warnins reported by static analyzers
```
Error: NULL_RETURNS (CWE-476): [#def1]
sssd-1.15.2/src/db/sysdb_certmap.c:149: var_assigned: Assigning: "el->values" =
null return value from "_talloc_zero_array".
sssd-1.15.2/src/db/sysdb_certmap.c:152: dereference: Dereferencing a null
pointer "el->values".
# 150|
# 151| for (c = 0; certmap->domains[c] != NULL; c++) {
# 152|-> el->values[c].data = (uint8_t *) talloc_strdup(el->values,
# 153|
certmap->domains[c]);
# 154| if (el->values[c].data == NULL) {
Error: NULL_RETURNS (CWE-476): [#def4]
sssd-1.15.2/src/lib/certmap/sss_cert_content_nss.c:892: var_assigned:
Assigning: "current" = null return value from "CERT_GetNextGeneralName".
sssd-1.15.2/src/lib/certmap/sss_cert_content_nss.c:788: dereference:
Dereferencing a null pointer "current".
# 786| current = name_list;
# 787| do {
# 788|-> switch (current->type) {
# 789| case certOtherName:
# 790| ret = add_string_other_name_to_san_list(mem_ctx,
Error: PW.SET_BUT_NOT_USED: [#def5]
sssd-1.15.2/src/lib/certmap/sss_cert_content_nss.c:923: set_but_not_used:
variable "parameters" was set but never used
# 921| SECItem der_item;
# 922| NSSInitContext *nss_ctx;
# 923|-> NSSInitParameters parameters = { 0 };
# 924| parameters.length = sizeof (parameters);
# 925|
Error: CHECKED_RETURN (CWE-252): [#def6]
sssd-1.15.2/src/lib/certmap/sss_cert_content_nss.c:1003: check_return: Calling
"NSS_ShutdownContext" without checking return value (as is done elsewhere 2 out
of 3 times).
sssd-1.15.2/src/p11_child/p11_child_nss.c:486: example_assign: Example 1:
Assigning: "rv" = return value from "NSS_ShutdownContext(nss_ctx)".
sssd-1.15.2/src/p11_child/p11_child_nss.c:487: example_checked: Example 1
(cont.): "rv" has its value checked in "rv != SECSuccess".
sssd-1.15.2/src/util/cert/nss/cert.c:404: example_assign: Example 2: Assigning:
"rv" = return value from "NSS_ShutdownContext(nss_ctx)".
sssd-1.15.2/src/util/cert/nss/cert.c:405: example_checked: Example 2 (cont.):
"rv" has its value checked in "rv != SECSuccess".
# 1001|
# 1002| CERT_DestroyCertificate(cert);
# 1003|-> NSS_ShutdownContext(nss_ctx);
# 1004|
# 1005| if (ret == EOK) {
Error: CLANG_WARNING: [#def11]
sssd-1.15.2/src/lib/certmap/sss_certmap.c:586:9: warning: Function call
argument is an uninitialized value
# talloc_free(exp);
# ^
/usr/include/talloc.h:228:26: note: expanded from macro 'talloc_free'
##define talloc_free(ctx) _talloc_free(ctx, __location__)
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:906:9: note: Assuming '_filter' is
not equal to null
# if (_filter == NULL || _domains == NULL) {
# ^~~~~~~~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:906:9: note: Left side of '||' is
false
sssd-1.15.2/src/lib/certmap/sss_certmap.c:906:28: note: Assuming '_domains' is
not equal to null
# if (_filter == NULL || _domains == NULL) {
# ^~~~~~~~~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:906:5: note: Taking false branch
# if (_filter == NULL || _domains == NULL) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:911:9: note: Assuming 'ret' is equal
to 0
# if (ret != 0) {
# ^~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:911:5: note: Taking false branch
# if (ret != 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:30: note: Assuming 'p' is not
equal to null
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:5: note: Loop condition is true.
Entering loop body
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:32: note: Assuming 'r' is equal
to null
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:9: note: Loop condition is false.
Execution continues on line 916
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:30: note: Assuming 'p' is not
equal to null
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:5: note: Loop condition is true.
Entering loop body
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:32: note: Assuming 'r' is equal
to null
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:9: note: Loop condition is false.
Execution continues on line 916
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:30: note: Assuming 'p' is not
equal to null
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:5: note: Loop condition is true.
Entering loop body
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:32: note: Assuming 'r' is equal
to null
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:9: note: Loop condition is false.
Execution continues on line 916
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:30: note: Assuming 'p' is not
equal to null
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:5: note: Loop condition is true.
Entering loop body
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:32: note: Assuming 'r' is not
equal to null
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:9: note: Loop condition is true.
Entering loop body
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:919:17: note: Assuming 'ret' is equal
to 0
# if (ret == 0) {
# ^~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:919:13: note: Taking true branch
# if (ret == 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:921:23: note: Calling 'get_filter'
# ret = get_filter(ctx, r->parsed_mapping_rule, cert_content,
# ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:603:9: note: Assuming 'result' is not
equal to null
# if (result == NULL) {
# ^~~~~~~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:603:5: note: Taking false branch
# if (result == NULL) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:607:44: note: Assuming 'comp' is not
equal to null
# for (comp = parsed_mapping_rule->list; comp != NULL; comp = comp->next) {
# ^~~~~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:607:5: note: Loop condition is true.
Entering loop body
# for (comp = parsed_mapping_rule->list; comp != NULL; comp = comp->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:608:9: note: Taking false branch
# if (comp->type == comp_string) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:610:16: note: Taking true branch
# } else if (comp->type == comp_template) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:611:19: note: Calling
'expand_template'
# ret = expand_template(ctx, comp->parsed_template, cert_content,
# ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:553:5: note: 'exp' declared without
an initial value
# char *exp;
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:555:5: note: Taking false branch
# if (strcmp("issuer_dn", parsed_template->name) == 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:558:12: note: Taking false branch
# } else if (strcmp("subject_dn", parsed_template->name) == 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:561:12: note: Taking false branch
# } else if (strncmp("subject_", parsed_template->name, 8) == 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:563:12: note: Taking false branch
# } else if (strcmp("cert", parsed_template->name) == 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:568:9: note: Control jumps to line 583
# goto done;
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:583:5: note: Taking false branch
# if (ret == 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:586:9: note: Function call argument
is an uninitialized value
# talloc_free(exp);
# ^ ~~~
/usr/include/talloc.h:228:26: note: expanded from macro 'talloc_free'
##define talloc_free(ctx) _talloc_free(ctx, __location__)
# ^ ~~~
# 584| *expanded = exp;
# 585| } else {
# 586|-> talloc_free(exp);
# 587| }
# 588|
```
And I am also not sure about few regex related warnings
```
Error: FORWARD_NULL (CWE-476): [#def12]
sssd-1.15.2/src/lib/certmap/sss_certmap.c:661: var_deref_model: Passing "NULL"
to "regexec", which dereferences it.
# 659| return false;
# 660| }
# 661|-> match = (regexec(®exp, tmp_str, 0, NULL, 0) == 0);
# 662| talloc_free(tmp_str);
# 663| } else {
Error: FORWARD_NULL (CWE-476): [#def13]
sssd-1.15.2/src/lib/certmap/sss_certmap.c:665: var_deref_model: Passing "NULL"
to "regexec", which dereferences it.
# 663| } else {
# 664| match = (item->val != NULL
# 665|-> && regexec(®exp, item->val, 0, NULL,
0) == 0);
# 666| }
# 667| if (!match) {
```
man 3 `regex` says:
```
REG_NOSUB
Do not report position of matches. The nmatch and pmatch argu‐
ments to regexec() are ignored if the pattern buffer supplied
was compiled with this flag set.
```
But I think we use `regcomp` only in file
`src/lib/certmap/sss_certmap_krb5_match.c`
without cflag `REG_NOSUB` and we do not allow pass regex as an input in API.
I think we should be safe.
LS
"""
See the full comment at
https://github.com/SSSD/sssd/pull/192#issuecomment-286389021
_______________________________________________
sssd-devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]
