From 0aa523cd16f97160ab406db8958a78253f7e31bc Mon Sep 17 00:00:00 2001
From: Noriko Hosoi <nhosoi@redhat.com>
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"
<tbordaz@redhat.com>
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