Closed Bug 1546056 Opened 9 months ago Closed 9 months ago

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

Categories

(MailNews Core :: Database, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 68.0

People

(Reporter: benc, Assigned: benc)

References

(Blocks 1 open bug)

Details

(Keywords: coverity)

Attachments

(1 file)

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.

Assignee: nobody → benc
Status: NEW → ASSIGNED
Keywords: coverity

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

Attachment #9059766 - Flags: review?(jorgk)
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.
Attachment #9059766 - Flags: review?(jorgk) → review+

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.

Keywords: checkin-needed
Component: General → Database
Product: Thunderbird → MailNews Core

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.

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

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0

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

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

You need to log in before you can comment on or make changes to this bug.