Re: CVS commit: src/lib/librumpuser

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Tue, 24 Mar 2020 01:56:56 +0000
    From:        "Kamil Rytarowski" <kamil%netbsd.org@localhost>
    Message-ID:  <20200324015656.33DF1FB27%cvs.NetBSD.org@localhost>

  | Module Name:	src
  | Committed By:	kamil
  | Date:		Tue Mar 24 01:56:56 UTC 2020
  |
  | Modified Files:
  | 	src/lib/librumpuser: rumpuser_sp.c
  |
  | Log Message:
  | Avoid buffer overflow
  |
  | Detected with ASan + RUMPKERNEL.

This "fix" cannot possibly be correct.

The code was (twice, the two cases are the same, so consider
just one of them)

        /* ensure comm is 0-terminated */  
        /* TODO: make sure it contains sensible chars? */
        comm[commlen] = '\0';

where the "fix" is for the final line to be changed into

	comm[commlen-1] = '\0';

Now I can see that the original may have been writing (perhaps just
sometimes, I haven't analysed it fully) one byte beyond the end of
the allocated space, and so something should be fixed, but your
change cannot possibly be the right way.

There are two possibilities here, in any particular case, either comm
was already nul-terminated, in which case the assignment isn't needed
at all, or it wasn't, in which case the changed code just destroyed the
final data byte by dumping a \0 on it - turning what was most probably
a harmless (though technically broken) one byte buffer overrun into
guaranteed broken code.

My guess is that the buffer is (always) already nul-terminated, and
the assignment was just paranoid over protection - and that the change
that was made simply overwrites one \0 with a different \0 (and so is
harmless, but stupid, the correct fix would be to delete the assignment
completely, similarly the "TODO" is nonsense, whatever the app decided
to exec() is its business, 'sensible' chars or not - either it works, or
doesn't, it isn't rump's business to second guess the client code).

I assume that because for exec() (which is what this is handling), the
arg strings are all \0 terminated - detecting that \0 is the only way the
arg copyin code knows where to stop.

If my guess is wrong, then the bug is where the args are collected. and
should be fixed there, not by plonking a \0 in here.

Kamil, once again, as I have asked before - if you don't have time to
fully analyse the problems that the sanitisers detect, please DO NOT
"fix" them - just file a PR and let someone who has the time deal with
it.    Making the code generate no sanitiser bug reports by breaking it
is not our objective (I hope).   There was no great urgency to "fix"
this particular one, it doesn't seem to have been causing any real problems,
and could have easily waited a few days (or weeks, or even months) until
the correct solution was found.

kre