Page MenuHome GnuPG

calc_header_length
Closed, ResolvedPublic

Description

Consider the following function (from g10/build-packet.c):

/****

  • calculate the length of a header */

static int
calc_header_length( u32 len, int new_ctb )
{

if( !len )

return 1; /* only the ctb */

if( new_ctb ) {

if( len < 192 )

	    return 2;

if( len < 8384 )

	    return 3;

else

	    return 6;
    }
    if( len < 256 )

return 2;

if( len < 65536 )

return 3;

return 5;

}

It's unclear what is meant when LEN = 0. According to the implementation (and
the comment), 0 means the header only consists of the ctb. But, this can only
be the case for old ctbs for packets with an indeterminate length (see RFC 4880,
section 4.2). For new ctbs, a single byte header is impossible:

  old ctb + 0 bytes              = 2 byte header
  old ctb + indeterminate length = 1 byte header
  new ctb + 0 bytes              = 2 byte header
  new ctb + partial body headers = 2 byte header

So, if len == 0 is supposed to mean indeterminate length, for the new ctb case,
we should return 2, not 1. But, then it is not possible to use this function to
compute the header length for a packet with a 0 length body. If instead len ==
0 means a 0 byte header, when we should change the code to return 2 in both the
old and new ctb case.

Looking at the callers, this case is never actually encountered. In other
words, this bug *currently* has no material impact on GnuPG: calc_header_length
is only called from calc_packet_length, which is called twice, once in
encrypt_simple and once in encrypt_crypt (both in g10/encrypt.c), and both of
these callers ensure that PT->LEN (which is passed as the len parameter to
calc_header_length in calc_packet_length) is greater than 0. As such, it is
easy to change the semantics.

Although this bug currently has no negative impact, I feel that it is
nevertheless important to fix it and document the interface so that if the
function is used elsewhere no bug is inadvertently introduced.

I think that we should change this function to be consistent with the principle
of least surprise. Specifically, len=0 should mean a 0 length body. If it
later becomes necessary to specify an indeterminate length body, then we should
add a new macro (defined, say, to -1) and act accordingly if that is passed as
len. Doing the opposite is more difficult.

Event Timeline

werner lowered the priority of this task from Normal to Low.Feb 2 2016, 3:20 PM

Needs to be documented. I see no reason to change this because because it has
no effect.

Patch attached. Is this okay to apply?

Since it was so trivial to create, I've attach an alternative patch with my
proposed change. Please let me know which one to apply.

Given how trivial the fix is, I applied that.