1546056 - Possible Out-of-bounds access in morkWriter::StartGroup()

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Ben Campbell]

Found by Coverity

Writing into fixed-size buffer in morkWriter::StartGroup() doesn't leave enough size for some wrapper bytes, if the maximum size of data is written.

Comment 1

[Ben Campbell]

Attachment: 1546056-fix-potential-morkwriter-bufferoverrun.patch

I don't think this one would ever be likely to bite us in the real world, but...

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 9059766 [details] [diff] [review]
1546056-fix-potential-morkwriter-bufferoverrun.patch

Thanks. It appears that the length of the `groupID` is limited by `morkWriter_kGroupBufSize`, but then there is code that checks it separately:
```
  mork_fill idFill = ev->TokenAsHex(p, groupID);
  mWriter_GroupBufFill = 0;
  // ev->TokenAsHex(mWriter_GroupBuf, groupID);
  if ( idFill < morkWriter_kGroupBufSize )
```

By that time, we've already written to `buf` via `p`, hmm.

Comment 3

[Ben Campbell]

I don't understand enough of the wider context, but my guess is that the limit (64) is pretty arbitrary, and way above the size of any tokens passing through... but I couldn't say that for certain.
Besides, a clean coverity scan seems like a good thing to aim for anyway. Even if 95% of the issues it flags up were benign, the last 5% could be nasty little hidden ones.

Comment 4

[Jorg K (CEST = GMT+2)]

Well, as I said, I'd like to understand why we have this:

  mork_token groupID = mWriter_CommitGroupIdentity;
  mork_fill idFill = ev->TokenAsHex(p, groupID);
  mWriter_GroupBufFill = 0;
  // ev->TokenAsHex(mWriter_GroupBuf, groupID);
  if ( idFill < morkWriter_kGroupBufSize )
  {
    MORK_MEMCPY(mWriter_GroupBuf, p, idFill + 1);
    mWriter_GroupBufFill = idFill;
  }
  else
    *mWriter_GroupBuf = 0;

If it's OK to use TokenAsHex() on p, it should be good to use on mWriter_GroupBuf with no further checking and no memcpy from p needed.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/49b8214906f2
Fix a potential buffer overrun in morkWriter by allocating the correct size. r=jorgk

Comment 6

[Wayne Mery (:wsmwk)]

This is covered by tests? With more prospect of reducing rather than introducing dataloss?

Comment 7

[Jorg K (CEST = GMT+2)]

No tests. To our best knowledge, this never failed, we just fixed it for correctness.