https://bugs.openldap.org/show_bug.cgi?id=10452
Issue ID: 10452 Summary: Potential NULL dereference in slap_acl_mask() Product: OpenLDAP Version: 2.6.8 Hardware: All OS: All Status: UNCONFIRMED Keywords: needs_review Severity: normal Priority: --- Component: slapd Assignee: bugs@openldap.org Reporter: nastentsiya.filimonova@gmail.com Target Milestone: ---
I've noticed potentially inconsistent NULL checks for the op->o_conn field in function slap_acl_mask (source file openldap-2.6.8/servers/slapd/acl.c).
At first, op->o_conn is explicitly checked for NULL:
if ( op->o_conn && !BER_BVISNULL( &op->o_conn->c_ndn ) ) { ndn = op->o_conn->c_ndn; } else { ndn = op->o_ndn; }
This suggests that op->o_conn may indeed be NULL in some scenarios.
However, right after that in the same function there are several unconditional dereferences of op->o_conn without a preceding NULL check, for example:
if ( !op->o_conn->c_listener ) { continue; }
if ( !op->o_conn->c_peer_domain.bv_val ) { continue; }
if ( !op->o_conn->c_peer_name.bv_val ) { continue; }
Could you please clarify whether op->o_conn is guaranteed to be non-NULL for all possible paths reaching these code sections?
If op->o_conn can indeed be NULL here, these dereferences may lead to a NULL pointer dereference and would require additional checks. If op->o_conn is guaranteed to be non-NULL by design, please confirm this (it would also be useful to document this assumption explicitly).
Thank you.
https://bugs.openldap.org/show_bug.cgi?id=10452
--- Comment #1 from Howard Chu hyc@openldap.org --- The NULL check appears to be redundant, added in 3eb87b2faae4b9f59f1270936f70a1781c2abd7a
op->o_conn is always populated.
https://bugs.openldap.org/show_bug.cgi?id=10452
--- Comment #2 from nastentsiya.filimonova@gmail.com --- I have a follow-up question regarding a similar issue in slap_acl_mask() (source file servers/slapd/acl.c, openldap 2.6.8).
There is a null check for `op->o_conn->c_listener` around lines 1245-1248: ``` if ( !BER_BVISEMPTY( &b->a_sockurl_pat ) ) { if ( ! op->o_conn->c_listener ) { continue; } ```
If this field is not present, the current iteration is safely skipped. However, this only happens when `&b->a_sockurl_pat` is not empty.
Later in the same function, around lines 1491-1494, `c_listener` is dereferenced via `c_sock_name` (`c_listener->sl_name`) without an explicit null-check: ``` if ( !BER_BVISEMPTY( &b->a_sockname_pat ) ) { if ( BER_BVISNULL( &op->o_conn->c_sock_name ) ) { continue; } ```
I'm trying to understand whether the conditions `!BER_BVISEMPTY( &b->a_sockname_pat )` and `!BER_BVISEMPTY( &b->a_sockurl_pat )` are correlated in any way that ensures we cannot reach lines 1491-1494 with `op->o_conn->c_listener == NULL`. If not, it might be safer to add a null check here. I'd appreciate any clarification.
https://bugs.openldap.org/show_bug.cgi?id=10452
Howard Chu hyc@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution|--- |INVALID
--- Comment #3 from Howard Chu hyc@openldap.org --- That code is leftover from 5c3909f5671ad15cd370354e5f1cfd2f7bb42770 for ITS#2361 in March 2003 which affected SLAPI plugins.
The SLAPI plugin code was rewritten in 2005 starting with ffe20229dc2a3c0b69c327d49dbbea172d213e4c making all of that moot.
https://bugs.openldap.org/show_bug.cgi?id=10452
--- Comment #4 from Howard Chu hyc@openldap.org --- For future reference: we expect investigators to browse the git history themselves, that's what it's there for.
https://bugs.openldap.org/show_bug.cgi?id=10452
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords|needs_review | Status|RESOLVED |VERIFIED
-
openldap-its＠openldap.org