From 0aa523cd16f97160ab406db8958a78253f7e31bc Mon Sep 17 00:00:00 2001 From: Noriko Hosoi Date: Fri, 22 Feb 2013 14:40:46 -0800 Subject: [PATCH] Bugzilla 912964 - bug in handling of LDAPv3 control data Bug Description: If we receive an incorrect control, with non NULL bv_val but bv_len=0, it can trigger a crash. This bug occurs because of a call of openldap ber_init with such incorrect control. Fix Description: Before calling ber_init, check that the control lenght is not 0. Note: checking in the patch on behalf of "Thierry bordaz" https://bugzilla.redhat.com/show_bug.cgi?id=913228 Reviewed by: rmeggins@redhat.com, nhosoi@redhat.com Platforms tested: Fedora 18, RHEL 6.4 (RHDS6.4) Flag Day: no Doc impact: possibly document a security bug --- ldap/servers/plugins/chainingdb/cb_controls.c | 6 +--- ldap/servers/plugins/chainingdb/cb_utils.c | 2 +- ldap/servers/plugins/deref/deref.c | 2 +- ldap/servers/plugins/dna/dna.c | 7 +++-- ldap/servers/plugins/replication/repl5_total.c | 6 ++-- ldap/servers/plugins/replication/repl_controls.c | 2 +- ldap/servers/plugins/replication/repl_extop.c | 21 ++++++++++++------- ldap/servers/plugins/replication/windows_private.c | 7 ++++++ ldap/servers/slapd/back-ldbm/sort.c | 9 +++---- ldap/servers/slapd/back-ldbm/vlv.c | 4 +- ldap/servers/slapd/passwd_extop.c | 2 +- ldap/servers/slapd/proxyauth.c | 2 +- ldap/servers/slapd/slapi-plugin.h | 4 +++ 13 files changed, 44 insertions(+), 30 deletions(-) diff --git a/ldap/servers/plugins/chainingdb/cb_controls.c b/ldap/servers/plugins/chainingdb/cb_controls.c index f079901..2a698a0 100644 --- a/ldap/servers/plugins/chainingdb/cb_controls.c +++ b/ldap/servers/plugins/chainingdb/cb_controls.c @@ -220,10 +220,8 @@ int cb_update_controls( Slapi_PBlock * pb, ctrls[dCount]=slapi_dup_control(reqControls[cCount]); dCount++; - } else - if (!strcmp(reqControls[cCount]->ldctl_oid,CB_LDAP_CONTROL_CHAIN_SERVER) && - reqControls[cCount]->ldctl_value.bv_val) { - + } else if (!strcmp(reqControls[cCount]->ldctl_oid,CB_LDAP_CONTROL_CHAIN_SERVER) && + BV_HAS_DATA((&(reqControls[cCount]->ldctl_value)))) { /* Max hop count reached ? */ /* Checked earlier by a call to cb_forward_operation() */ diff --git a/ldap/servers/plugins/chainingdb/cb_utils.c b/ldap/servers/plugins/chainingdb/cb_utils.c index 2492226..f28f5d3 100644 --- a/ldap/servers/plugins/chainingdb/cb_utils.c +++ b/ldap/servers/plugins/chainingdb/cb_utils.c @@ -148,7 +148,7 @@ int cb_forward_operation(Slapi_PBlock * pb ) { int iscritical=0; if (slapi_control_present(ctrls,CB_LDAP_CONTROL_CHAIN_SERVER,&ctl_value,&iscritical) && - ctl_value && ctl_value->bv_val) { + BV_HAS_DATA(ctl_value)) { /* Decode control data */ /* hop INTEGER (0 .. maxInt) */ diff --git a/ldap/servers/plugins/deref/deref.c b/ldap/servers/plugins/deref/deref.c index 7c502df..51394c8 100644 --- a/ldap/servers/plugins/deref/deref.c +++ b/ldap/servers/plugins/deref/deref.c @@ -382,7 +382,7 @@ deref_parse_ctrl_value(DerefSpecList *speclist, const struct berval *ctrlbv, int PR_ASSERT(ctrlbv && ctrlbv->bv_val && ctrlbv->bv_len && ldapcode && ldaperrtext); - if (!ctrlbv || !ctrlbv->bv_val) { + if (!BV_HAS_DATA(ctrlbv)) { *ldapcode = LDAP_PROTOCOL_ERROR; *ldaperrtext = "Empty deref control value"; return; diff --git a/ldap/servers/plugins/dna/dna.c b/ldap/servers/plugins/dna/dna.c index 6babe23..080e357 100644 --- a/ldap/servers/plugins/dna/dna.c +++ b/ldap/servers/plugins/dna/dna.c @@ -1668,7 +1668,7 @@ static int dna_request_range(struct configEntry *config_entry, } /* Parse response */ - if (responsedata && responsedata->bv_val) { + if (BV_HAS_DATA(responsedata)) { respber = ber_init(responsedata); if (ber_scanf(respber, "{aa}", &lower_str, &upper_str) == LBER_ERROR) { ret = LDAP_PROTOCOL_ERROR; @@ -3745,14 +3745,15 @@ static int dna_extend_exop(Slapi_PBlock *pb) /* Fetch the request data */ slapi_pblock_get(pb, SLAPI_EXT_OP_REQ_VALUE, &reqdata); - if (!reqdata || !reqdata->bv_val) { + if (!BV_HAS_DATA(reqdata)) { slapi_log_error(SLAPI_LOG_FATAL, DNA_PLUGIN_SUBSYSTEM, "dna_extend_exop: No request data received.\n"); goto free_and_return; } /* decode the exop */ - if ((reqdata->bv_val == NULL) || (tmp_bere = ber_init(reqdata)) == NULL) { + tmp_bere = ber_init(reqdata); + if (tmp_bere == NULL) { goto free_and_return; } diff --git a/ldap/servers/plugins/replication/repl5_total.c b/ldap/servers/plugins/replication/repl5_total.c index 99ba838..f22246d 100644 --- a/ldap/servers/plugins/replication/repl5_total.c +++ b/ldap/servers/plugins/replication/repl5_total.c @@ -726,10 +726,10 @@ decode_total_update_extop(Slapi_PBlock *pb, Slapi_Entry **ep) slapi_pblock_get(pb, SLAPI_EXT_OP_REQ_OID, &extop_oid); slapi_pblock_get(pb, SLAPI_EXT_OP_REQ_VALUE, &extop_value); - if (NULL == extop_oid || + if ((NULL == extop_oid) || ((strcmp(extop_oid, REPL_NSDS50_REPLICATION_ENTRY_REQUEST_OID) != 0) && - (strcmp(extop_oid, REPL_NSDS71_REPLICATION_ENTRY_REQUEST_OID) != 0)) || - NULL == extop_value || NULL == extop_value->bv_val) + (strcmp(extop_oid, REPL_NSDS71_REPLICATION_ENTRY_REQUEST_OID) != 0)) || + !BV_HAS_DATA(extop_value)) { /* Bogus */ goto loser; diff --git a/ldap/servers/plugins/replication/repl_controls.c b/ldap/servers/plugins/replication/repl_controls.c index 980bdd8..e6aeaaa 100644 --- a/ldap/servers/plugins/replication/repl_controls.c +++ b/ldap/servers/plugins/replication/repl_controls.c @@ -216,7 +216,7 @@ decode_NSDS50ReplUpdateInfoControl(LDAPControl **controlsp, if (slapi_control_present(controlsp, REPL_NSDS50_UPDATE_INFO_CONTROL_OID, &ctl_value, &iscritical)) { - if ((ctl_value->bv_val == NULL) || (tmp_bere = ber_init(ctl_value)) == NULL) + if (!BV_HAS_DATA(ctl_value) || (tmp_bere = ber_init(ctl_value)) == NULL) { rc = -1; goto loser; diff --git a/ldap/servers/plugins/replication/repl_extop.c b/ldap/servers/plugins/replication/repl_extop.c index e842c62..68aed62 100644 --- a/ldap/servers/plugins/replication/repl_extop.c +++ b/ldap/servers/plugins/replication/repl_extop.c @@ -343,10 +343,10 @@ decode_startrepl_extop(Slapi_PBlock *pb, char **protocol_oid, char **repl_root, slapi_pblock_get(pb, SLAPI_EXT_OP_REQ_OID, &extop_oid); slapi_pblock_get(pb, SLAPI_EXT_OP_REQ_VALUE, &extop_value); - if (NULL == extop_oid || + if ((NULL == extop_oid) || ((strcmp(extop_oid, REPL_START_NSDS50_REPLICATION_REQUEST_OID) != 0) && - (strcmp(extop_oid, REPL_START_NSDS90_REPLICATION_REQUEST_OID) != 0)) || - NULL == extop_value || NULL == extop_value->bv_val) + (strcmp(extop_oid, REPL_START_NSDS90_REPLICATION_REQUEST_OID) != 0)) || + !BV_HAS_DATA(extop_value)) { /* bogus */ rc = -1; @@ -480,9 +480,9 @@ decode_endrepl_extop(Slapi_PBlock *pb, char **repl_root) slapi_pblock_get(pb, SLAPI_EXT_OP_REQ_OID, &extop_oid); slapi_pblock_get(pb, SLAPI_EXT_OP_REQ_VALUE, &extop_value); - if (NULL == extop_oid || - strcmp(extop_oid, REPL_END_NSDS50_REPLICATION_REQUEST_OID) != 0 || - NULL == extop_value || NULL == extop_value->bv_val) + if ((NULL == extop_oid) || + (strcmp(extop_oid, REPL_END_NSDS50_REPLICATION_REQUEST_OID) != 0) || + !BV_HAS_DATA(extop_value)) { /* bogus */ rc = -1; @@ -545,8 +545,8 @@ decode_repl_ext_response(struct berval *bvdata, int *response_code, PR_ASSERT(NULL != response_code); PR_ASSERT(NULL != ruv_bervals); - if (NULL == bvdata || NULL == response_code || NULL == ruv_bervals || - NULL == data_guid || NULL == data || NULL == bvdata->bv_val) + if ((NULL == response_code) || (NULL == ruv_bervals) || + (NULL == data_guid) || (NULL == data) || !BV_HAS_DATA(bvdata)) { return_value = -1; } @@ -1412,6 +1412,11 @@ decode_cleanruv_payload(struct berval *extop_value, char **payload) { BerElement *tmp_bere = NULL; int rc = 0; + + if (!BV_HAS_DATA(extop_value)) { + rc = -1; + goto free_and_return; + } if ((tmp_bere = ber_init(extop_value)) == NULL){ rc = -1; diff --git a/ldap/servers/plugins/replication/windows_private.c b/ldap/servers/plugins/replication/windows_private.c index 355abd5..20360a3 100644 --- a/ldap/servers/plugins/replication/windows_private.c +++ b/ldap/servers/plugins/replication/windows_private.c @@ -782,6 +782,13 @@ void windows_private_update_dirsync_control(const Repl_Agmt *ra,LDAPControl **co dirsync = slapi_dup_control( controls[i-1]); } + if (!dirsync || !BV_HAS_DATA((&(dirsync->ldctl_value)))) { +#ifdef FOR_DEBUGGING + return_value = LDAP_CONTROL_NOT_FOUND; +#endif + goto choke; + } + ber = ber_init( &dirsync->ldctl_value ) ; if (ber_scanf( ber, "{iiO}", &hasMoreData, &maxAttributeCount, &serverCookie) == LBER_ERROR) diff --git a/ldap/servers/slapd/back-ldbm/sort.c b/ldap/servers/slapd/back-ldbm/sort.c index 501765c..6984467 100644 --- a/ldap/servers/slapd/back-ldbm/sort.c +++ b/ldap/servers/slapd/back-ldbm/sort.c @@ -299,15 +299,14 @@ int parse_sort_spec(struct berval *sort_spec_ber, sort_spec **ps) char *matchrule = NULL; int rc = LDAP_SUCCESS; - if (NULL == sort_spec_ber->bv_val) { + if (!BV_HAS_DATA(sort_spec_ber)) { return LDAP_PROTOCOL_ERROR; } ber = ber_init(sort_spec_ber); - if(ber==NULL) - { - return -1; - } + if (ber == NULL) { + return -1; + } /* Work our way along the BER, one sort spec at a time */ for ( tag = ber_first_element( ber, &len, &last ); !term_tag(tag); tag = ber_next_element( ber, &len, last )) { diff --git a/ldap/servers/slapd/back-ldbm/vlv.c b/ldap/servers/slapd/back-ldbm/vlv.c index 39d16bf..2c09531 100644 --- a/ldap/servers/slapd/back-ldbm/vlv.c +++ b/ldap/servers/slapd/back-ldbm/vlv.c @@ -1861,11 +1861,11 @@ vlv_parse_request_control( backend *be, struct berval *vlv_spec_ber,struct vlv_r */ BerElement *ber = NULL; int return_value = LDAP_SUCCESS; - + vlvp->value.bv_len = 0; vlvp->value.bv_val = NULL; - if (NULL == vlv_spec_ber->bv_val) + if (!BV_HAS_DATA(vlv_spec_ber)) { return_value= LDAP_OPERATIONS_ERROR; return return_value; diff --git a/ldap/servers/slapd/passwd_extop.c b/ldap/servers/slapd/passwd_extop.c index 3c050d6..4aebee3 100644 --- a/ldap/servers/slapd/passwd_extop.c +++ b/ldap/servers/slapd/passwd_extop.c @@ -534,7 +534,7 @@ passwd_modify_extop( Slapi_PBlock *pb ) /* Get the ber value of the extended operation */ slapi_pblock_get(pb, SLAPI_EXT_OP_REQ_VALUE, &extop_value); - if (extop_value->bv_val == NULL) + if (!BV_HAS_DATA(extop_value)) { /* The request field wasn't provided. We'll * now try to determine the userid and verify diff --git a/ldap/servers/slapd/proxyauth.c b/ldap/servers/slapd/proxyauth.c index fe36cf1..562ac93 100644 --- a/ldap/servers/slapd/proxyauth.c +++ b/ldap/servers/slapd/proxyauth.c @@ -96,7 +96,7 @@ parse_LDAPProxyAuth(struct berval *spec_ber, int version, char **errtextp, break; } - if ( !spec_ber || !spec_ber->bv_val ) { + if (!BV_HAS_DATA(spec_ber)) { break; } diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h index 4962b2a..f78787e 100644 --- a/ldap/servers/slapd/slapi-plugin.h +++ b/ldap/servers/slapd/slapi-plugin.h @@ -107,6 +107,10 @@ NSPR_API(PRUint32) PR_fprintf(struct PRFileDesc* fd, const char *fmt, ...) #define BERLEN_T "u" #endif +/* Common check on berval before accessing the contents. */ +/* bv is a struct berval *bv */ +#define BV_HAS_DATA(bv) ((bv != NULL) && (bv->bv_len > 0) && (bv->bv_val != NULL)) + /* * The slapi_attr_get_flags() routine returns a bitmap that contains one or * more of these values. -- 1.7.7.6