Обсуждение: Possible memory corruption (src/timezone/zic.c b/src/timezone/zic.c)

Ranier Vilela <ranier.vf@gmail.com> writes:
> Per Coverity.
> CID 1412632 (#1 of 1): Out-of-bounds access (OVERRUN)1.
> overrun-buffer-val: Overrunning buffer pointed to by &c of 1 bytes by
> passing it to a function which accesses it at byte offset 4.

> For some people, Coverity opinions count zero.

This particular complaint seems to match a pattern that Coverity has
been generating a lot lately.  I've yet to see one that wasn't a
false positive, so it looks like a Coverity bug to me.

> It doesn't matter if WideCharToMultiByte, it will fail or not, the danger
> exists.
> If WideCharToMultiByte returns 4, memmove will possibly destroy 4 bytes.

This analysis seems to me to be nonsense.

(1) sizeof(char) is one, per the C standard.  Therefore, the existing
coding in itssymlink accurately describes the size of the buffer it's
providing.  The alternative you propose also accurately describes
the size of the buffer it's providing.  It's nonsense to suppose that
one is safer than the other --- if readlink is willing to write past
the specified buffer size, they're both equally dangerous.  So this
fix fixes nothing.

(2) As an independent matter, we should worry about whether our
pgreadlink() implementation is capable of writing past the specified
buffer size.  I don't think that WideCharToMultiByte will do so;
Microsoft's documentation clearly says that "cbMultiByte" is the
size *in bytes* of the buffer indicated by "lpMultiByteStr".
However it's fair to question whether that bit of code for deleting
"\??\" is safe.  I think it is though.  Per the Microsoft docs,
the return value of WideCharToMultiByte is:

  If successful, returns the number of bytes written to the buffer pointed
  to by lpMultiByteStr. If the function succeeds and cbMultiByte is 0, the
  return value is the required size, in bytes, for the buffer indicated by
  lpMultiByteStr.  [ but we aren't passing zero for cbMultiByte ]

  The function returns 0 if it does not succeed.
  [ and one of the failure cases is: ]
  ERROR_INSUFFICIENT_BUFFER. A supplied buffer size was not large enough,
  or it was incorrectly set to NULL.

So I don't believe that it will return r > 4 when the supplied buffer size
is only 1.  What's going to happen instead is a failure return, because
the string doesn't fit.

Hence, we do have a problem here, which is that pgreadlink is pretty
much always going to fail when used in the way zic.c is using it, and
thus zic is going to fail to recognize symlinks when run on Windows.

The IANA crew are unlikely to care: they're going to say that they're
using readlink() per the POSIX specification for it, and they'll be
right.

So the question for us is whether it's worth trying to make pgreadlink
conform to the letter of the POSIX spec in this detail.  TBH, I can't
get excited about that, at least not so far as zic's usage is concerned.
What Windows user is going to be using our version of zic to install
timezone files into a subdirectory that has pre-existing symlinks?

By the same token, I'm pretty unexcited about working around pgreadlink's
deficiency by modifying the IANA code in the way you suggest.  It's
painful enough to keep our copy of their code in sync with their updates;
we don't need hacks like that added.

In short, I don't see much of a case for doing anything; but if somebody
were really excited about this they could try to make pgreadlink() fill
the supplied buffer without failing when it's too short.

            regards, tom lane

I wrote:
> So the question for us is whether it's worth trying to make pgreadlink
> conform to the letter of the POSIX spec in this detail.  TBH, I can't
> get excited about that, at least not so far as zic's usage is concerned.

Hmmm ... on closer inspection, though, it might not be that hard.
pgreadlink is already using a fixed-length buffer (with only enough
room for MAX_PATH WCHARs) for the input of WideCharToMultiByte.  So
it could use a fixed-length buffer of say 4 * MAX_PATH bytes for the
output, and then transfer just the appropriate amount of data to the
caller's buffer.

            regards, tom lane

At Sat, 15 May 2021 11:35:13 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
> Em sex., 14 de mai. de 2021 às 19:52, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
>
> > I wrote:
> > > So the question for us is whether it's worth trying to make pgreadlink
> > > conform to the letter of the POSIX spec in this detail.  TBH, I can't
> > > get excited about that, at least not so far as zic's usage is concerned.
> >
> > Hmmm ... on closer inspection, though, it might not be that hard.
> > pgreadlink is already using a fixed-length buffer (with only enough
> > room for MAX_PATH WCHARs) for the input of WideCharToMultiByte.  So
> > it could use a fixed-length buffer of say 4 * MAX_PATH bytes for the
> > output, and then transfer just the appropriate amount of data to the
> > caller's buffer.
> >
> Following your directions, maybe something like this will solve?

-    DWORD        attr;
-    HANDLE        h;

Why the patch moves the definitions for "attr" and "h"?


+    Assert(path != NULL && buf != NULL);

I don't think it's required.  Even if we want to imitate readlink,
they should (maybe) return EFALUT in that case.


+    buf[r] = '\0';

readlink is defined as not appending a terminator.  In the first place
the "buf[r] = '\0'" is overrunning the given buffer.


-    return 0 <= readlink(name, &c, 1);
+    return 0 <= readlink(name, linkpath, sizeof(linkpath));

According to the discussion, we don't want to modify zic.c at
all. (Maybe forgot to remove?)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center