On Tue, Apr 12, 2016 at 12:46:00PM +0200, Lukas Slebodnik wrote:
> On (12/04/16 12:07), Sumit Bose wrote:
> >On Tue, Apr 12, 2016 at 11:08:54AM +0200, Pavel Březina wrote:
> >> On 04/11/2016 05:58 PM, Lukas Slebodnik wrote:
> >> >On (11/04/16 15:01), Lukas Slebodnik wrote:
> >> >>ehlo,
> >> >>
> >> >>attached patch fix crash in #2980
> >> >>
> >> >>LS
> >> >
> >> >>From 422abe6e6263c3c907611a8611fa3f28d6e93ae0 Mon Sep 17 00:00:00
2001
> >> >>From: Lukas Slebodnik <lslebodn(a)redhat.com>
> >> >>Date: Mon, 11 Apr 2016 14:46:47 +0200
> >> >>Subject: [PATCH] IPA: Check RDN in ipa_add_ad_memberships_get_next
> >> >>
> >> >>LDB functions ldb_dn_get_component_val and ldb_dn_get_rdn_val
> >> >>validate dn before returning component value.
> >> >>It should be valid DN according to RFC4514.
> >> >>
> >> >>IPA/389ds might return problematic DN due to replication conflicts.
> >> >>e.g. "cn=System: Read Service
Delegations+nsuniqueid=b0736336-d06e11e5-8e8acabe-ce8d458d,cn=permissions,dc=example,dc=com"
> >> >>
> >> >>It's better to check return value of these LDb function rather
than
> >> >>crash because of dereference of NULL pointer.
> >> >>
> >> >>Resolves:
> >> >>https://fedorahosted.org/sssd/ticket/2980
> >> >>---
> >> >>src/providers/ipa/ipa_subdomains_ext_groups.c | 8 +++++++-
> >> >>1 file changed, 7 insertions(+), 1 deletion(-)
> >> >>
> >> >>diff --git a/src/providers/ipa/ipa_subdomains_ext_groups.c
b/src/providers/ipa/ipa_subdomains_ext_groups.c
> >> >>index
8e006663a31ff60b86cf6392c15ce711c52cf0fc..445538be8798d58aee5d0cabf53ce91d94467a26 100644
> >> >>--- a/src/providers/ipa/ipa_subdomains_ext_groups.c
> >> >>+++ b/src/providers/ipa/ipa_subdomains_ext_groups.c
> >> >>@@ -862,7 +862,13 @@ static void
ipa_add_ad_memberships_get_next(struct tevent_req *req)
> >> >> goto fail;
> >> >> }
> >> >>
> >> >>- val = ldb_dn_get_component_val(group_dn, 0);
> >> >>+ val = ldb_dn_get_rdn_val(group_dn);
> >> >>+ if (val == NULL) {
> >> >>+ DEBUG(SSSDBG_OP_FAILURE,
> >> >>+ "Invalid group DN [%s].\n",
state->groups[state->iter]);
> >> >>+ ret = EINVAL;
> >> >>+ goto fail;
> >> >>+ }
> >> >
> >> >Alternative solution is to validate group_dn with ldb_dn_validate
> >> >but it's already done in
ldb_dn_get_component_val/ldb_dn_get_rdn_val
> >> >
> >>
> >> Did you consider using ipa_get_rdn instead? It would also check that dn has
> >> proper format.
> >
> >I don't think this DN check is needed here, nevertheless like in
> >_ipa_get_rdn() the result of ldb_dn_get_rdn_val() should be checked for
> >
> >+ if (val == NULL || val->data == NULL) {
> >
> Not a bad idea.
Patch looks good and passes CI
http://sssd-ci.duckdns.org/logs/job/41/11/summary.html.
ACK
Thank you.
master:
* 22eead9590e11c7adab33ec5ab8b46d3c3cb4406
sssd-1-13:
* 3b2c92f7ed1890060f527f703fabf9dbecca259a
LS