A forgotten patch?

[Werner Koch]

On Sat, 28 Feb 2015 03:02, aef at raxys.net said:

> of GnuPG in 2009. According to him, the patch fixes lots of problems
> that might be usable as in attack vectors on GnuPG. It seems however, as
> if this patch was never included into upstream GnuPG. Because of that,

This comes up every once in a while and I try not to spend time on that
silly thing.  At least by private mail I explained it to him years ago,
but he seem not to understand.  Let's look at two examples:

We have this function:

  /* Return a string which is used as a kind of process ID */
  const byte *
  get_session_marker( size_t *rlen )
  {
      static byte marker[SIZEOF_UNSIGNED_LONG*2];
      static int initialized;

      if ( !initialized ) {
          volatile ulong aa, bb; /* we really want the uninitialized value */
          ulong a, b;

          initialized = 1;
          /* also this marker is guessable it is not easy to use this
           * for a faked control packet because an attacker does not
           * have enough control about the time the verification does
           * take place.  Of course, we can add just more random but
           * than we need the random generator even for verification
           * tasks - which does not make sense. */
          a = aa ^ (ulong)getpid();
          b = bb ^ (ulong)time(NULL);
          memcpy( marker, &a, SIZEOF_UNSIGNED_LONG );
          memcpy( marker+SIZEOF_UNSIGNED_LONG, &b, SIZEOF_UNSIGNED_LONG );
      }
      *rlen = sizeof(marker);
      return marker;
  }

Fefe changes it to use /dev/urandom by inserting this code before the
above initialization test:

      int fd;

      if (!initialized) {
        fd=open("/dev/urandom",O_RDONLY);
        if (fd!=-1) {
  	if (read(fd,marker,sizeof(marker))==sizeof(marker))
  	  initialized=1;
  	close(fd);
        }
      }

Let's ignore the fact that a failed open falls back to the, in his
book broken, existing scheme.   The real trouble here is that he does
not look for the use of this function.  Now, for what is it used: If a
clearsigned messages is received that message has an indication which
hash algorithms are used (the "Hash: XXXX" line).  We need to convey
this information from the unarmor layer to the actual parsing code. This
is done by inserting a faked packet with information on the hash
algorithms as well as a plaintext packet.  The final message the parser
then sees resembles this valid OpenPGP message

   One-Pass Signed Message :- One-Pass Signature Packet,
               OpenPGP Message, Corresponding Signature Packet.

Now an attacker might be able to insert a bogus control packet in an
arbitrary non-armored messages and in such a way resembles cleartext
message.  However, the only thing he could do with this is to announce a
different hash algorithm and switch the parsed to a different
interpretation of white spaces at line ends.  The first is entirely
harmless because gpg checks that the used hash algorithms matches those
in the actual Signature Packet which comes after the signed text.  It is
annoying if that happens but it merely leads to a BAD signature.

The slightly changed interpretation of trailing line spaces (clearsigned
versus text mode signatures) might be useful to insert extra trailing
spaces into a text file.  But that is something which happens to mails
anyway and the whole reason for text mode messages.

Why the session marker packet?  That is simply to help gpg to stop
earlier on bogus input data.  No need for cryptographical strong random.


Second example:

  struct private_membuf_s {
    size_t len;
    size_t size;
    char *buf;
    int out_of_core;
  };
  typedef struct private_membuf_s membuf_t;

  void
  put_membuf (membuf_t *mb, const void *buf, size_t len)
  {
    if (mb->out_of_core)
      return;

    assert(mb->len + len > mb->len);
    if (mb->len + len >= mb->size)
      {
        char *p;

        assert(len + 1024 > len);
        assert(mb->size + len > mb->size);
        mb->size += len + 1024;
        p = xrealloc (mb->buf, mb->size);
        mb->buf = p;
      }
    memcpy (mb->buf + mb->len, buf, len);
    mb->len += len;
  }

The assert calls are inserted by Fefe.  Their intention is to detect
integer overflows.  Given that unsigned integers are used these checks
do work.  Are they required?

Looking at the first one: MB->LEN tracks the used length of a malloced
buffer.  Thus it this value is bound at a reasonable value.  LEN give
the length of BUG which is either a static array or another malloced
region.  Thus this is also bound.  To trigger this assertion it needs
two buffers which together allocate more that 2^32 bytes (or 2^64 on
systems with sizeof(size_t)==8). And before the process comes to
put_membuf it has already allocated other buffers.

Thus the answer from the engineering department is: No.

The QA department would give an unfavorable statement about the use of
assert calls for conditions which are supposed to happen (in Fefe's
short sighted analysis).

> is a professional code security auditor and a reputable member of the

Right he lists Microsoft and a German "newspaper", to which many people
would never talk, as his clients.

I wonder a bit why a he did not caught obvious bugs which could indeed
lead to a segv.  Like the faulty shifting of MSBs fixed with commit
57af33d9e7c9b20b413b96882e670e75a67a5e65 lingering for many years in the
code.

And why pusblishing a patch and no bug reports?


Salam-Shalom,

   Werner

--
Die Gedanken sind frei.  Ausnahmen regelt ein Bundesgesetz.