Debian Bug report logs - #480432
libkrb53: ret_flags check at g_accept_sec_context.c:248 looks wrong

Toggle useless messages

Message #5 received at submit@bugs.debian.org (full text, mbox):

From: Bryan Kadzban <debbugs@kdzbn.homelinux.net>
To: Debian Bug Tracking System <submit@bugs.debian.org>
Subject: libkrb53: ret_flags check at g_accept_sec_context.c:248 looks wrong
Date: Fri, 09 May 2008 19:40:32 -0400

Package: libkrb53
Version: 1.6.dfsg.3~beta1-4
Severity: normal
Tags: patch

The check "(ret_flags && GSS_C_DELEG_FLAG)" looks extremely wrong.
GSS_C_DELEG_FLAGS is a bitmask, and should not be logically compared
against anything (since it will always be logical true).  ret_flags is a
pointer, and its value should not be bitwise compared against anything,
either.

Proposed fix: do two checks.  First check whether ret_flags is non-NULL
(ret_flags on its own), then check whether the pointed-to value has the
GSS_C_DELEG_FLAG bit turned on.

A patch to make this change is attached.  (It was generated from a
libkrb53 tree that already had the rest of the Debian patches applied.)

*** krb5-fix-comparison.patch
Don't do a logical AND between a pointer and a bitmask.  Check the
pointer first, and do a bitwise AND between the pointer's value (if
the pointer is non-NULL) and the bitmask instead.

diff -ur a/src/lib/gssapi/mechglue/g_accept_sec_context.c b/src/lib/gssapi/mechglue/g_accept_sec_context.c
--- a/src/lib/gssapi/mechglue/g_accept_sec_context.c	2007-10-01 22:43:12.000000000 -0400
+++ b/src/lib/gssapi/mechglue/g_accept_sec_context.c	2008-05-09 14:10:23.000000000 -0400
@@ -245,7 +245,7 @@
 	    }
 
 	    /* Ensure we're returning correct creds format */
-	    if ((ret_flags && GSS_C_DELEG_FLAG) &&
+	    if (ret_flags && ((*ret_flags) & GSS_C_DELEG_FLAG) &&
 		tmp_d_cred != GSS_C_NO_CREDENTIAL) {
 		gss_union_cred_t d_u_cred = NULL;
 


-- System Information:
Debian Release: lenny/sid
  APT prefers testing
  APT policy: (990, 'testing'), (500, 'unstable'), (500, 'stable')
Architecture: i386 (i686)

Kernel: Linux 2.6.22-3-686 (SMP w/1 CPU core)
Locale: LANG=C, LC_CTYPE=C (charmap=ANSI_X3.4-1968)
Shell: /bin/sh linked to /bin/bash

Versions of packages libkrb53 depends on:
ii  libc6                         2.7-10     GNU C Library: Shared libraries
ii  libcomerr2                    1.40.8-2   common error description library
ii  libkeyutils1                  1.2-7      Linux Key Management Utilities (li

libkrb53 recommends no packages.

-- no debconf information

Message #10 received at 480432@bugs.debian.org (full text, mbox):

From: Russ Allbery <rra@debian.org>
To: Bryan Kadzban <debbugs@kdzbn.homelinux.net>
Cc: 480432@bugs.debian.org
Subject: Re: Bug#480432: libkrb53: ret_flags check at g_accept_sec_context.c:248 looks wrong
Date: Fri, 09 May 2008 17:04:30 -0700

Bryan Kadzban <debbugs@kdzbn.homelinux.net> writes:

> The check "(ret_flags && GSS_C_DELEG_FLAG)" looks extremely wrong.
> GSS_C_DELEG_FLAGS is a bitmask, and should not be logically compared
> against anything (since it will always be logical true).  ret_flags is a
> pointer, and its value should not be bitwise compared against anything,
> either.

Thanks!  I'm checking with upstream just to be sure, but this looks
correct to me.

-- 
Russ Allbery (rra@debian.org)               <http://www.eyrie.org/~eagle/>

Message #15 received at 480432@bugs.debian.org (full text, mbox):

From: Bryan Kadzban <debbugs@kdzbn.homelinux.net>
To: Russ Allbery <rra@debian.org>
Cc: 480432@bugs.debian.org
Subject: Re: Bug#480432: libkrb53: ret_flags check at g_accept_sec_context.c:248
 looks wrong
Date: Sat, 10 May 2008 11:28:03 -0400

Russ Allbery wrote:
> Bryan Kadzban <debbugs@kdzbn.homelinux.net> writes:
> 
>> The check "(ret_flags && GSS_C_DELEG_FLAG)" looks extremely wrong. 
>> GSS_C_DELEG_FLAGS is a bitmask, and should not be logically
>> compared against anything (since it will always be logical true).
>> ret_flags is a pointer, and its value should not be bitwise
>> compared against anything, either.
> 
> Thanks!  I'm checking with upstream just to be sure, but this looks 
> correct to me.

After some looking at the upstream bug database, this appears to be the
same as their bug #5802:

http://krbdev.mit.edu/rt/Ticket/Display.html?id=5802

and that page says the bug was fixed in 1.6.3.  And indeed, the source
for 1.6.3 (from upstream) handles the flags differently: it passes the
address of a local variable to the mechanism-specific accept_sec_context
function, then copies it back to the address that the user passed in if
non-NULL.  It also uses the local variable in the bitfield comparison.

So upgrading to upstream version 1.6.3 (or adopting this part of the
code, at least) should fix this as well.

Message #20 received at 480432@bugs.debian.org (full text, mbox):

From: Russ Allbery <rra@debian.org>
To: Bryan Kadzban <debbugs@kdzbn.homelinux.net>
Cc: 480432@bugs.debian.org
Subject: Re: Bug#480432: libkrb53: ret_flags check at g_accept_sec_context.c:248 looks wrong
Date: Sat, 10 May 2008 11:19:22 -0700

Bryan Kadzban <debbugs@kdzbn.homelinux.net> writes:

> After some looking at the upstream bug database, this appears to be the
> same as their bug #5802:
>
> http://krbdev.mit.edu/rt/Ticket/Display.html?id=5802
>
> and that page says the bug was fixed in 1.6.3.  And indeed, the source
> for 1.6.3 (from upstream) handles the flags differently: it passes the
> address of a local variable to the mechanism-specific accept_sec_context
> function, then copies it back to the address that the user passed in if
> non-NULL.  It also uses the local variable in the bitfield comparison.
>
> So upgrading to upstream version 1.6.3 (or adopting this part of the
> code, at least) should fix this as well.

Oh, in that case, it should already be fixed; 1.6.3 is already in Debian
unstable.

-- 
Russ Allbery (rra@debian.org)               <http://www.eyrie.org/~eagle/>

Message #25 received at 480432-done@bugs.debian.org (full text, mbox):

From: Bryan Kadzban <debbugs@kdzbn.homelinux.net>
To: Russ Allbery <rra@debian.org>
Cc: 480432-done@bugs.debian.org
Subject: Re: Bug#480432: libkrb53: ret_flags check at g_accept_sec_context.c:248
 looks wrong
Date: Sat, 10 May 2008 16:09:38 -0400

Version: 1.6.dfsg.3-1

Russ Allbery wrote:
> Bryan Kadzban <debbugs@kdzbn.homelinux.net> writes:
> 
>> So upgrading to upstream version 1.6.3 (or adopting this part of the
>> code, at least) should fix this as well.
> 
> Oh, in that case, it should already be fixed; 1.6.3 is already in Debian
> unstable.

...You're right.  I've just pulled the source for libkrb53 in unstable,
and it is fixed in that version.  (That's what I get for only checking
the version from testing.)

So never mind; this isn't actually a problem.  Thanks!

Send a report that this bug log contains spam.