Skip Menu |
 

History Show all quoted text — Show full headers
# Fri Feb 20 13:29:02 2004 Wyllys Ingersoll <wyllys.ingersoll@sun.com> - Ticket created
Subject: wrap_size_limit broken for CFX
From: Wyllys Ingersoll <wyllys.ingersoll@sun.com>
To: krb5-bugs@mit.edu
Date: Fri, 20 Feb 2004 13:25:04 -0500
Download (untitled) / with headers
text/plain 1.1KiB

There are 2 small problems in the wrap_size_limit function
when dealing with cfx->proto==1 and conf_req_flag is set.

Line 113:
if (conf_req_flag) {
while (sz > 0 &&
krb5_encrypt_size(sz, ctx->enc->enctype) + 16 >
req_output_size)
sz--;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
SHOULD BE: sz--;
krb5_encrypt_size(sz, ctx->enc->enctype) + 32 >
req_output_size)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

} else {
if (sz < 16 + ctx->cksum_size)
sz = 0;
else
sz -= (16 + ctx->cksum_size);
}
....


The token header is included twice in the output token,
but its not counted as part of krb5_encrypt_size, so you
must account for it twice when computing the wrap size.

Also, decrement the sz counter before calculating the size to avoid
an off-by-1 error at the end.

For example:
req_output_size = 1076 should result in a 'wrap_size' of 1016.

putting the sz-- at the end of the loop yields a wrap_size of 1015.
Not a fatal problem or anything, obviously, just a nit.

-Wyllys Ingersoll
# Fri Feb 20 15:40:32 2004 raeburn (Ken Raeburn) - Comments added
To: rt-comment@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #2266] wrap_size_limit broken for CFX
From: Ken Raeburn <raeburn@MIT.EDU>
Date: Fri, 20 Feb 2004 15:40:27 -0500
RT-Send-Cc:
Download (untitled) / with headers
text/plain 2KiB
Good catch. I think the subtraction of 64K after this (commented
"While testing only!") will cover up for the bug (except about 1/4096
of the time when CFX_EXERCISE is defined), but it should still be
fixed.

"Wyllys Ingersoll via RT" <rt-comment@krbdev.mit.edu> writes:
Show quoted text
> There are 2 small problems in the wrap_size_limit function
> when dealing with cfx->proto==1 and conf_req_flag is set.
>
> Line 113:
> if (conf_req_flag) {
> while (sz > 0 &&
> krb5_encrypt_size(sz, ctx->enc->enctype) + 16 >
> req_output_size)
> sz--;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> SHOULD BE: sz--;
> krb5_encrypt_size(sz, ctx->enc->enctype) + 32 >
> req_output_size)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Actually, an extra 16 bytes in the input doesn't necessarily
translate to an extra 16 bytes in the output, so I think we want to
use 16 here, and then subtract 16 after this loop.

Show quoted text
> The token header is included twice in the output token,
> but its not counted as part of krb5_encrypt_size, so you
> must account for it twice when computing the wrap size.

Yep.

Show quoted text
> Also, decrement the sz counter before calculating the size to avoid
> an off-by-1 error at the end.
>
> For example:
> req_output_size = 1076 should result in a 'wrap_size' of 1016.
>
> putting the sz-- at the end of the loop yields a wrap_size of 1015.

Huh. For some reason, I'm having trouble seeing it.

Instrumenting the 1.3 branch code and running one of our tests, I get
an input size of 1048576 yielding an output size of 982997. That's
65579 bytes difference, 65535 for potential EC field padding (for the
testing that's not currently enabled, except the compensation for it
isn't properly conditionalized) plus 16 bytes for the token header
plus 12 for the AES checksum plus 16 for the confounder, and missing
the 16 bytes for the encrypted copy of the header.

Actually, with the EC field hackery in there, I'm suprised you don't
get 0 for a req_output_size so small.

Ken
# Fri Feb 20 16:38:40 2004 Wyllys Ingersoll <wyllys.ingersoll@sun.com> - Comments added
Date: Fri, 20 Feb 2004 16:34:16 -0500
From: Wyllys Ingersoll <wyllys.ingersoll@sun.com>
To: rt-comment@krbdev.mit.edu
Cc: krb5-prs@mit.edu, Ken Raeburn <raeburn@mit.edu>
Subject: Re: [krbdev.mit.edu #2266] wrap_size_limit broken for CFX
RT-Send-Cc:
Download (untitled) / with headers
text/plain 2.7KiB
Ken Raeburn via RT wrote:
Show quoted text
> Good catch. I think the subtraction of 64K after this (commented
> "While testing only!") will cover up for the bug (except about 1/4096
> of the time when CFX_EXERCISE is defined), but it should still be
> fixed.

Yes, that block does cover up the bug. I had removed that block in my
code so thats why I noticed the problem. Specifically, I was doing
an encrypted ftp transfer of a 1MB file (in ASCII mode) which
tickled the bug in my ftp client/server (which is not the same as
the ftp client/server that comes with MIT krb5).

Show quoted text
>
> "Wyllys Ingersoll via RT" <rt-comment@krbdev.mit.edu> writes:
>
>>There are 2 small problems in the wrap_size_limit function
>>when dealing with cfx->proto==1 and conf_req_flag is set.
>>
>>Line 113:
>>if (conf_req_flag) {
>> while (sz > 0 &&
>> krb5_encrypt_size(sz, ctx->enc->enctype) + 16 >
>> req_output_size)
>> sz--;
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>SHOULD BE: sz--;
>> krb5_encrypt_size(sz, ctx->enc->enctype) + 32 >
>> req_output_size)
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>
> Actually, an extra 16 bytes in the input doesn't necessarily
> translate to an extra 16 bytes in the output, so I think we want to
> use 16 here, and then subtract 16 after this loop.

Either way, the additional 16 bytes must be accounted for *somewhere*.

Show quoted text
>
>
>>Also, decrement the sz counter before calculating the size to avoid
>>an off-by-1 error at the end.
>>
>>For example:
>>req_output_size = 1076 should result in a 'wrap_size' of 1016.
>>
>>putting the sz-- at the end of the loop yields a wrap_size of 1015.
>
>
> Huh. For some reason, I'm having trouble seeing it.

Leaving "sz--" until the bottom of the loop means that krb5_encrypt_size
is effectively called twice for the same value. decrement the 'sz'
variable before calculating the new size, not after. That way, once
the correct value is found, sz does not get decremented again.

Show quoted text
>
> Instrumenting the 1.3 branch code and running one of our tests, I get
> an input size of 1048576 yielding an output size of 982997. That's
> 65579 bytes difference, 65535 for potential EC field padding (for the
> testing that's not currently enabled, except the compensation for it
> isn't properly conditionalized) plus 16 bytes for the token header
> plus 12 for the AES checksum plus 16 for the confounder, and missing
> the 16 bytes for the encrypted copy of the header.
>
> Actually, with the EC field hackery in there, I'm suprised you don't
> get 0 for a req_output_size so small.

Well, I forgot to mention that I had removed that block before I started
testing or else I probably would never have found the problem.

-Wyllys
# Mon Feb 23 15:52:54 2004 tlyu (Taylor Yu) - Component krb5-libs added
# Mon Feb 23 15:52:54 2004 tlyu (Taylor Yu) - Target_Version 1.3.2 added
# Mon Feb 23 16:22:27 2004 raeburn (Ken Raeburn) - Comments added
To: wyllys.ingersoll@sun.com
Cc: rt-comment@krbdev.mit.edu
Subject: [krbdev.mit.edu #2266] wrap_size_limit broken for CFX
From: Ken Raeburn <raeburn@MIT.EDU>
Date: Mon, 23 Feb 2004 16:22:22 -0500
RT-Send-Cc:
Download (untitled) / with headers
text/plain 615B

Well, I'm still not seeing the off-by-one issue, for some reason, but
this version of the code seems to get the right answer (including your
example of 1076 -> 1016), so I'll be checking it in. Thanks for
catching this.

Ken

while (sz > 0 && krb5_encrypt_size(sz, ctx->enc->enctype) + 16 > req_output_size)
sz--;
/* Allow for encrypted copy of header. */
if (sz > 16)
sz -= 16;
else
sz = 0;
#ifdef CFX_EXERCISE
/* Allow for EC padding. In the MIT implementation, only
added while testing. */
if (sz > 65535)
sz -= 65535;
else
sz = 0;
#endif
# Mon Feb 23 16:25:21 2004 raeburn (Ken Raeburn) - Status changed from 'new' to 'resolved'
# Mon Feb 23 16:25:21 2004 raeburn (Ken Raeburn) - Tags pullup added
# Mon Feb 23 16:25:22 2004 raeburn (Ken Raeburn) - Given to raeburn (Ken Raeburn)
# Mon Feb 23 16:25:22 2004 raeburn (Ken Raeburn) - Correspondence added
From: raeburn@mit.edu
Subject: CVS Commit
Download (untitled) / with headers
text/plain 310B
* wrap_size_limit.c (krb5_gss_wrap_size_limit): Fix calculation for
confidential CFX tokens.


To generate a diff of this commit:



cvs diff -r5.270 -r5.271 krb5/src/kdc/ChangeLog
cvs diff -r1.237 -r1.238 krb5/src/lib/gssapi/krb5/ChangeLog
cvs diff -r1.11 -r1.12 krb5/src/lib/gssapi/krb5/wrap_size_limit.c
# Mon Feb 23 16:27:17 2004 Wyllys Ingersoll <wyllys.ingersoll@sun.com> - Comments added
Subject: Re: [krbdev.mit.edu #2266] wrap_size_limit broken for CFX
From: Wyllys Ingersoll <wyllys.ingersoll@sun.com>
To: Ken Raeburn <raeburn@MIT.EDU>
Cc: rt-comment@krbdev.mit.edu
Date: Mon, 23 Feb 2004 16:23:18 -0500
RT-Send-Cc:
Download (untitled) / with headers
text/plain 547B
On Mon, 2004-02-23 at 16:22, Ken Raeburn wrote:
Show quoted text
> Well, I'm still not seeing the off-by-one issue, for some reason, but
> this version of the code seems to get the right answer (including your
> example of 1076 -> 1016), so I'll be checking it in. Thanks for
> catching this.
>
> Ken
>
> while (sz > 0 && krb5_encrypt_size(sz, ctx->enc->enctype) + 16 > req_output_size)

One more thing - wouldn't it be better to use the newer
krb5_c_encrypt_length() routine here and get rid of one more
use of the old 'krb5_encrypt_size' API?

-Wyllys
# Mon Feb 23 16:48:41 2004 tlyu (Taylor Yu) - Version_Fixed 1.3.2 added
# Mon Feb 23 16:48:41 2004 tlyu (Taylor Yu) - Correspondence added
From: tlyu@mit.edu
Subject: CVS Commit
Download (untitled) / with headers
text/plain 208B
pullup from trunk


To generate a diff of this commit:



cvs diff -r1.218.2.16 -r1.218.2.17
krb5/src/lib/gssapi/krb5/ChangeLog
cvs diff -r1.10.4.1 -r1.10.4.2
krb5/src/lib/gssapi/krb5/wrap_size_limit.c
# Mon Feb 23 16:51:36 2004 raeburn (Ken Raeburn) - Comments added
Date: Mon, 23 Feb 2004 16:51:31 -0500
Subject: Re: [krbdev.mit.edu #2266] wrap_size_limit broken for CFX
Cc: Ken Raeburn <raeburn@mit.edu>, rt-comment@krbdev.mit.edu
To: wyllys.ingersoll@sun.com
From: Ken Raeburn <raeburn@MIT.EDU>
RT-Send-Cc:
Download (untitled) / with headers
text/plain 717B
On Monday, Feb 23, 2004, at 16:23 US/Eastern, Wyllys Ingersoll wrote:
Show quoted text
> One more thing - wouldn't it be better to use the newer
> krb5_c_encrypt_length() routine here and get rid of one more
> use of the old 'krb5_encrypt_size' API?
>

*sigh* Yep. Actually, even krb5_c_encrypt_length goes in the wrong
direction (more obvious if you look at enctypes like DES that round up
to a multiple of a block size); we should instead add to the crypto API
a function that implements some sort of encrypt_size_limit
functionality.

The old crypto API is still in our export lists; I think we're probably
going to leave it as is for now, and fix it up properly for a future
release. I've opened a new ticket on that...
# Wed Dec 16 18:02:42 2015 tlyu (Taylor Yu) - CustomField pullup deleted