[Freeipa-devel] Design Review Keytab Retrieval
Lukas Slebodnik
lslebodn at redhat.com
Sat Jul 12 18:38:07 UTC 2014
On (23/06/14 14:35), Simo Sorce wrote:
>----- Original Message -----
>> ----- Original Message -----
>> > > Can you check if ipaProtectedOperation is in the aci attribute in the
>> > > base tree object ?
>> > > It should be there as excluded, and that should cause admin to not be
>> > > able to retrieve keytabs.
>> >
>> > It was not. While running ipa-ldap-updater I got the following:
>> > InvalidSyntax: ACL Syntax Error(-5):(targetattr=
>> > \22ipaProtectedOperation;write_keys\22)(version 3.0; acl \22Admins are
>> > allowed to rekey any entity\22; allow(write) groupdn =
>> > \22ldap:///cn=admins: Invalid syntax.
>>
>> Uhmm I do not see anything obviously wrong with ACI instruction, it looks
>> just like the one I replace, Ideas ?
>> Do you have ipaProtectedOperation in the schema ?
>>
>> (I rebased patch 3 but will wait to send a patchset until we understand (and
>> fix) why this is failing to update.
>
>Ok, apparently it was a quoting issue in the .update files, hopefully that's
>the only issue (I am at a conference today and do not have my test env. handy).
>
>The attached patches are rebased on the latest master.
>
>Simo.
>From af7364cda7f5ef5948ce782bd5eded0b57583029 Mon Sep 17 00:00:00 2001
>From: Simo Sorce <simo at redhat.com>
>Date: Tue, 17 Sep 2013 00:30:14 -0400
>Subject: [PATCH 3/6] keytab: Add new extended operation to get a keytab.
>
>This new extended operation allow to create new keys or retrieve
>existing ones. The new set of keys is returned as a ASN.1 structure
>similar to the one that is passed in by the 'set keytab' extended
>operation.
>
>Access to the operation is regulated through a new special ACI that
>allows 'retrieval' only if the user has access to an attribute named
>ipaProtectedOperation postfixed by the subtypes 'read_keys' and
>'write_keys' to distinguish between creation and retrieval operation.
>
>For example for allowing retrieval by a specific user the following ACI
>is set on cn=accounts:
>
>(targetattr="ipaProtectedOperation;read_keys") ...
> ... userattr=ipaAllowedToPerform;read_keys#USERDN)
>
>This ACI matches only if the service object hosts a new attribute named
>ipaAllowedToPerform that holds the DN of the user attempting the
>operation.
>
>Resolves:
>https://fedorahosted.org/freeipa/ticket/3859
>---
> .../ipa-pwd-extop/ipa_pwd_extop.c | 571 +++++++++++++++++++++
> install/share/60basev3.ldif | 3 +
> install/share/default-aci.ldif | 7 +
> install/updates/20-aci.update | 13 +-
> util/ipa_krb5.h | 1 +
> 5 files changed, 594 insertions(+), 1 deletion(-)
>
>diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
>index c0bb9fda26172699e9ae7628f61b763c746188fe..65a5d41f9ea85689952e818fa4e8018c39786db8 100644
>--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
>+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
>@@ -1268,6 +1268,571 @@ free_and_return:
> return SLAPI_PLUGIN_EXTENDED_SENT_RESULT;
> }
>
>+/* Format of getkeytab request
>+ *
>+ * KeytabGetRequest ::= CHOICE {
>+ * newkeys [0] Newkeys,
>+ * curkeys [1] CurrentKeys,
>+ * reply [2] Reply
>+ * }
>+ *
>+ * NewKeys ::= SEQUENCE {
>+ * serviceIdentity [0] OCTET STRING,
>+ * enctypes [1] SEQUENCE OF Int16
>+ * password [2] OCTET STRING OPTIONAL,
>+ * }
>+ *
>+ * CurrentKeys ::= SEQUENCE {
>+ * serviceIdentity [0] OCTET STRING,
>+ * }
>+ */
>+
>+#define GK_REQUEST_NEWKEYS (LBER_CLASS_CONTEXT | LBER_CONSTRUCTED | 0)
>+#define GK_REQUEST_CURKEYS (LBER_CLASS_CONTEXT | LBER_CONSTRUCTED | 1)
>+#define GKREQ_SVCNAME_TAG (LBER_CLASS_CONTEXT | LBER_CONSTRUCTED | 1)
>+#define GKREQ_ENCTYPES_TAG (LBER_CLASS_CONTEXT | LBER_CONSTRUCTED | 1)
>+#define GKREQ_PASSWORD_TAG (LBER_CLASS_CONTEXT | LBER_CONSTRUCTED | 2)
>+
>+static int decode_getkeytab_request(struct berval *extop, bool *wantold,
>+ char **_svcname, char **_password,
>+ krb5_key_salt_tuple **kenctypes,
>+ int *num_kenctypes, char **_err_msg)
>+{
>+ int rc = LDAP_OPERATIONS_ERROR;
>+ char *err_msg = NULL;
>+ BerElement *ber = NULL;
>+ ber_len_t tlen;
>+ ber_tag_t rtag;
>+ ber_tag_t ttag;
>+ ber_tag_t ctag;
>+ char *svcname = NULL;
>+ char *password = NULL;
>+ ber_int_t enctype;
>+ krb5_key_salt_tuple *enctypes = NULL;
>+ int num = 0;
>+
>+ ber = ber_init(extop);
>+ if (ber == NULL) {
>+ err_msg = "KeytabGet Request decode failed.\n";
>+ rc = LDAP_PROTOCOL_ERROR;
>+ goto done;
>+ }
>+
>+ /* check this is a request */
>+ rtag = ber_peek_tag(ber, &tlen);
>+ if (rtag != GK_REQUEST_NEWKEYS && rtag != GK_REQUEST_CURKEYS) {
>+ LOG_FATAL("ber_peek_tag failed, wrong request type\n");
>+ err_msg = "Invalid payload.\n";
>+ rc = LDAP_PROTOCOL_ERROR;
>+ goto done;
>+ }
>+
>+ /* ber parse code */
>+ ttag = ber_scanf(ber, "{t[a]", &ctag, &svcname);
>+ if (ttag == LBER_ERROR || ctag != GKREQ_SVCNAME_TAG) {
>+ LOG_FATAL("ber_scanf failed to decode service name\n");
>+ err_msg = "Invalid payload.\n";
>+ rc = LDAP_PROTOCOL_ERROR;
>+ goto done;
>+ }
>+
>+ if (rtag == GK_REQUEST_CURKEYS) {
>+ rc = LDAP_SUCCESS;
>+ goto done;
>+ }
>+
>+ ttag = ber_peek_tag(ber, &tlen);
>+ if (ttag != GKREQ_ENCTYPES_TAG) {
>+ LOG_FATAL("ber_peek_tag failed to find enctypes\n");
>+ err_msg = "Invalid payload.\n";
>+ rc = LDAP_PROTOCOL_ERROR;
>+ goto done;
>+ }
>+ ttag = ber_peek_tag(ber, &tlen);
>+ for (num = 0; ttag == LBER_INTEGER; num++) {
>+ if ((num % 10) == 0) {
>+ /* allocate space for at least 10 more enctypes */
>+ enctypes = realloc(enctypes,
>+ (num + 10) * sizeof(krb5_key_salt_tuple));
step 1:
successful allocation
>+ if (!enctypes) {
>+ LOG_FATAL("allocation failed\n");
>+ err_msg = "Internal error\n";
>+ rc = LDAP_OPERATIONS_ERROR;
>+ goto done;
>+ }
>+ }
>+
>+ ttag = ber_scanf(ber, "i", &enctype);
step 2:
ber_scanf can return LBER_ERROR
>+ if (ttag == LBER_ERROR) {
>+ LOG_FATAL("ber_scanf failed to decode enctype\n");
>+ err_msg = "Invalid payload.\n";
>+ rc = LDAP_PROTOCOL_ERROR;
step 3:
variable rc is set to LDAP_PROTOCOL_ERROR
>+ goto done;
>+ }
>+
>+ enctypes[num].ks_enctype = enctype;
>+ enctypes[num].ks_salttype = KRB5_KDB_SALTTYPE_NORMAL;
>+ ttag = ber_peek_tag(ber, &tlen);
>+ }
>+
>+ /* ttag peek done as last step of the previous for loop */
>+ if (ttag == GKREQ_PASSWORD_TAG) {
>+ /* optional password present */
>+ ttag = ber_scanf(ber, "[a]", &password);
>+ if (ttag == LBER_ERROR) {
>+ LOG_FATAL("ber_scanf failed to decode password\n");
>+ err_msg = "Invalid payload.\n";
>+ rc = LDAP_PROTOCOL_ERROR;
>+ goto done;
>+ }
>+ }
>+
>+ rc = LDAP_SUCCESS;
>+
>+done:
>+ if (rc != LDAP_SUCCESS) {
step 4:
taking true branch beacuse rc is LDAP_PROTOCOL_ERROR
>+ free(password);
>+ free(svcname);
>+ *_err_msg = err_msg;
>+ } else {
>+ *_password = password;
>+ *_svcname = svcname;
>+ *wantold = (rtag == GK_REQUEST_CURKEYS);
>+ *kenctypes = enctypes;
>+ *num_kenctypes = num;
>+ }
>+ if (ber) ber_free(ber, 1);
>+ return rc;
step 5:
Variable "enctypes" going out of scope leaks the storage it points to.
Patch0004 is attached.
LS
-------------- next part --------------
>From 0f931433707d8a5ddbb9870192a28ce7bbd6a624 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Sat, 12 Jul 2014 18:31:42 +0200
Subject: [PATCH 3/4] Remove unsed local variable ber.
---
daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
index ca021cac71da690a498fe3003fae1babb30456c1..2f9bed4f8559c171e794420aaedd5407e330c2e3 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
@@ -1639,7 +1639,6 @@ static int ipapwd_getkeytab(Slapi_PBlock *pb, struct ipapwd_krbcfg *krbcfg)
krb5_context krbctx = NULL;
krb5_error_code krberr;
struct berval *extop_value = NULL;
- BerElement *ber = NULL;
char *service_name = NULL;
char *svcname;
Slapi_Entry *target_entry = NULL;
@@ -1827,7 +1826,6 @@ free_and_return:
}
free(svals);
}
- if (ber) ber_free(ber, 1);
if (bvp) ber_bvfree(bvp);
return SLAPI_PLUGIN_EXTENDED_SENT_RESULT;
--
1.9.3
-------------- next part --------------
>From 8c1443fd22e60129202818814a485dbcea2fc18f Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Sat, 12 Jul 2014 18:39:11 +0200
Subject: [PATCH 4/4] Fix memory leak in case of failure in decode_getkeytab_re
daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c:1355:
alloc_fn: Storage is returned from allocation function "realloc".
daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c:1355:
var_assign: Assigning: "enctypes" = storage returned from "realloc(enctypes, (num + 10) * 8UL)".
daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c:1393:
Condition rc != LDAP_SUCCESS, taking true branch
daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c:1405:
leaked_storage: Variable "enctypes" going out of scope leaks the storage it points to.
---
daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
index 2f9bed4f8559c171e794420aaedd5407e330c2e3..f0346a343188930dfc90e19d2e5d38cb30741b90 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
@@ -1393,6 +1393,7 @@ done:
if (rc != LDAP_SUCCESS) {
free(password);
free(svcname);
+ free(enctypes);
*_err_msg = err_msg;
} else {
*_password = password;
--
1.9.3
More information about the Freeipa-devel
mailing list