[krbdev.mit.edu #2266] wrap_size_limit broken for CFX

[Wyllys Ingersoll via RT]

Ken Raeburn via RT wrote:
> 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).

> 
> "Wyllys Ingersoll via RT" <rt-comment at 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*.

>
> 
>>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.

> 
> 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