Ticket History #242: kadmind ACL processing totally broken

From hartmans@MIT.EDU Tue Nov 26 02:08:19 1996
Received: from MIT.EDU (SOUTH-STATION-ANNEX.MIT.EDU [18.72.1.2]) by rt-11.MIT.EDU (8.7.5/8.7.3) with SMTP id CAA12725 for <bugs@RT-11.MIT.EDU>; Tue, 26 Nov 1996 02:08:19 -0500
Received: from OPUS.MIT.EDU by MIT.EDU with SMTP
id AA06240; Tue, 26 Nov 96 01:58:55 EST
Received: by opus.MIT.EDU (940816.SGI.8.6.9/4.7) id GAA19247; Tue, 26 Nov 1996 06:58:54 GMT
Message-Id: <199611260658.GAA19247@opus.MIT.EDU>
Date: Tue, 26 Nov 1996 06:58:54 GMT
From: hartmans@MIT.EDU
Reply-To: hartmans@MIT.EDU
To: krb5-bugs@MIT.EDU
Cc: krbdev@MIT.EDU
Subject: critical: kadmind ACL processing totally broken
X-Send-Pr-Version: 3.99

mit

System: IRIX opus 5.3 11091812 IP22 mips



Barry's patch to kadmind broke the ACL handling so that it
oesn't work on any platform; this breaks all tests besides kpasswd ,
and will fail in release environments; this bug is sufficient to cause
a thaw.





gmake[3]: Entering directory `/var/tmp/krb5/build/lib/rpc/unit-test'
./../../../kadmin/testing/scripts/env-setup.sh ../../../../krb5-1.0/src/lib/rpc/unit-test/../../../kadmin/testing/scripts/start_servers
RPC_TEST_SRVTAB=/tmp/rpc_test_v5srvtab ./../../../kadmin/testing/scripts/env-setup.sh ../../../../krb5-1.0/src/lib/rpc/unit-test/rpc_test_setup.sh
ERROR OVSEC_KADM_AUTH_ADD {Operation requires ``add'' privilege}
ERROR KADM5_AUTH_CHANGEPW {Operation requires ``change-password'' privilege}
ERROR OVSEC_KADM_AUTH_ADD {Operation requires ``add'' privilege}
ERROR KADM5_AUTH_CHANGEPW {Operation requires ``change-password'' privilege}
RPC_TEST_SRVTAB=/tmp/rpc_test_v5srvtab ./../../../kadmin/testing/scripts/env-setup.sh \
runtest --debug --srcdir ../../../../krb5-1.0/src/lib/rpc/unit-test --host mips-sgi-irix5.3 SERVER=./server CLIENT=./client \
KINIT=./../../../clients/kinit/kinit \
KDESTROY=./../../../clients/kdestroy/kdestroy \
PROT=-t --tool rpc_test
Test Run By hartmans on Tue Nov 26 00:00:00 EST 1996
Native configuration is mips-sgi-irix5.3

=== rpc_test tests ===

Running ../../../../krb5-1.0/src/lib/rpc/unit-test/rpc_test.0/expire.exp ...
Running ../../../../krb5-1.0/src/lib/rpc/unit-test/rpc_test.0/fullrun.exp ...
Running ../../../../krb5-1.0/src/lib/rpc/unit-test/rpc_test.0/gsserr.exp ...
FAIL: gss err: timeout waiting for server output

Nov 26 01:27:01 opus kadmind[19106](Notice): Reques
t: kadm5_init (V1), admin@SECURE-TEST.OV.COM, success, client=admin@SECURE-TEST.OV.COM, service=ovsec_adm/admin@SECURE-TEST.OV.COM, addr=18.70.0.252
Nov 26 01:27:01 opus kadmind[19106](Notice): Unauthorized request: kadm5_create_principal, server/opus.mit.edu@SECURE-TEST.OV.COM, client=admin@SECURE-TEST.OV.COM, service=ovsec_adm/admin@SECURE-TEST.OV.COM, addr=18.70.0.252
Nov 26 01:27:01 opus kadmind[19106](Notice): Unauthorized request: kadm5_randkey_principal (V1), server/opus.mit.edu@SECURE-TEST.OV.COM, client=admin@SECURE-TEST.OV.COM, service=ovsec_adm/admin@SECURE-TEST.OV.COM, addr=18.70.0.252
Nov 26 01:27:01 opus kadmind[19106](Notice): Unauthorized request: kadm5_create_principal, notserver/opus.mit.edu@SECURE-TEST.OV.COM, client=admin@SECURE-TEST.OV.COM, service=ovsec_adm/admin@SECURE-TEST.OV.COM, addr=18.70.0.252
Nov 26 01:27:01 opus kadmind[19106](Notice): Unauthorized request: kadm5_randkey_principal (V1), notserver/opus.mit.edu@SECURE-TEST.OV.COM, client=admin@SECURE-TEST.OV.COM, service=ovsec_adm/admin@SECURE-TEST.OV.COM, addr=18.70.0.252
Nov 26 01:27:05 opus krb5kdc[19104](info): AS_REQ 18.70.0.252(1750): ISSUE: authtime 848989625, testuser@SECURE-TEST.OV.COM for krbtgt/SECURE-TEST.OV.COM@SECURE-TEST.OV.COM

Here is the ACL:
se


Thanks to Marc, I understand the problem. Basically, Barry's
patches replaces the catchall ACL entry with a empty string, which
does not parse. This causes kadmind to assume that there is a syntax
error in the ACL file (the catchall entry is always parsed even if
there are other entries.) This causes the brilliantly designed ACL
parsing routines to free the entire ACL and only accept password
changing requests.

You should change the catchal entry to "* O" or something like that
and consider redesigning this vestage of the Beta5 admin system.

From: "Theodore Y. Ts'o" <tytso@MIT.EDU>
To: krb5-bugs@MIT.EDU, hartmans@MIT.EDU
Cc: bjaspan@MIT.EDU, gnats-admin@RT-11.MIT.EDU, krb5-prs@RT-11.MIT.EDU
Subject: Re: krb5-admin/242: critical: kadmind ACL processing totally broken
Date: Tue, 26 Nov 1996 02:22:32 -0500

Backing out the change may the right answer. The "O" privilege doesn't
exist. (Password changing seems to be automatically handled by the
server, independemnt of what is in the kadmin acl file.)

- Ted

State-Changed-From-To: open-open
State-Changed-By: marc
State-Changed-When: Tue Nov 26 02:39:48 1996
State-Changed-Why:

You don't want to back out the change because then the bug barry was
trying to fix will recur. This is the bug that if the acl_file can't
be opened, the kadmind will core dump. I recommend the following fix, instead of barry's:

*** /mit/krb5/sandbox/src/lib/kadm5/srv/server_acl.c Tue Nov 26 02:45:10 1996
--- /tmp/server_acl.c Tue Nov 26 02:47:05 1996
***************
*** 276,282 ****
}
else {
com_err(acl_acl_file, errno, acl_cantopen_msg);
! if (acl_list_head = acl_parse_line(acl_catchall_entry)) {
acl_list_tail = acl_list_head;
}
else {
--- 276,283 ----
}
else {
com_err(acl_acl_file, errno, acl_cantopen_msg);
! if (acl_catchall_entry &&
! (acl_list_head = acl_parse_line(acl_catchall_entry))) {
acl_list_tail = acl_list_head;
}
else {


From: "Barry Jaspan" <bjaspan@MIT.EDU>
To: krb5-bugs@MIT.EDU, hartmans@MIT.EDU, krbdev@MIT.EDU
Cc: Subject: Re: krb5-admin/242: critical: kadmind ACL processing totally broken
Date: Tue, 26 Nov 1996 17:07:12 GMT

Mea culpa. After making the change, I tested to make sure that a
missing kadm5.acl file prevented all access (whic it did, obviously),
but did not think to check that a correct acl file still worked. The
irony here is that I considered the patch Marc made and, for no
particularly good reason, chose instead the one I actually made
because I thought it was safer. I *did* read the code to see what
would happen on an empty ACL entry, and I concluded it would just be
skipped. I guess I was wrong. Oops.

Sick programmers shouldn't make last-minute changes. :-/

On the other hand, this is what the friendly-test cycle is for.

Barry

State-Changed-From-To: open-closed
State-Changed-By: tlyu
State-Changed-When: Sat Nov 30 17:48:57 1996
State-Changed-Why:

Closing this PR because change has been checked in and tested.
Somehow someone forgot to close it earlier.