Project

General

Profile

Actions

Bug #14899

closed

lib9p: Remove potential buffer overwrite in l9p_puqids()

Added by Andy Fiddaman about 2 months ago. Updated about 2 months ago.

Status:
Closed
Priority:
High
Assignee:
Category:
lib - userland libraries
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Bite-size
Tags:
Gerrit CR:
External Bug:

Description

See
When a 9P server sends an L9P_RWALK reply, it specifies the number of
qids enclosed as a 16-bit number. l9p_puqids() unpacks the specified
number of qids into its qids argument, which is the wqid element of a
struct l9p_f_rwalk:

  struct l9p_f_rwalk {
        struct l9p_hdr hdr;
        uint16_t nwqid;
        struct l9p_qid wqid[L9P_MAX_WELEM];
  };

#define L9P_MAX_WELEM   256

l9p_puqids() doesn't check the server's number against this maximum:

static ssize_t
l9p_puqids(struct l9p_message *msg, uint16_t *num, struct l9p_qid *qids)
{
        size_t i, lim;
        ssize_t ret, r;

        r = l9p_pu16(msg, num);

        if (r > 0) {
                for (i = 0, lim = *num; i < lim; i++) {
                        ret = l9p_puqid(msg, &qids[i]);
                        if (ret < 0)
                                return (-1);
                        r += ret;
                }
        }
        return (r);
}

So if a malicious or enthusiastic server sends back more than 256
qids, the client will write them beyond the end of wqid[].
Actions #1

Updated by Electric Monk about 2 months ago

  • Gerrit CR set to 2301
Actions #2

Updated by Andy Fiddaman about 2 months ago

I tested this on OmniOS with a Fedora bhyve VM, mounting the filesystem via

mount -t 9p -o trans=virtio,rw,uname=root,dirsync bob /a

and then checked out the illumos-gate tree under /a and did various operations on it such as ls -lR and find ., git gc etc. Everything appeared to work as before.

I verified that the modified function was being called using dtrace:

# dtrace -n 'pid14314::l9p_puqids:entry{@[*args[1]] = count()}'
dtrace: description 'pid14314::l9p_puqids:entry' matched 1 probe
^C

     0            19917
     1           378132
Actions #3

Updated by Andy Fiddaman about 2 months ago

  • Description updated (diff)
Actions #4

Updated by Electric Monk about 2 months ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 64121b135066abca1808f49288c947e236922532

commit  64121b135066abca1808f49288c947e236922532
Author: Konrad Sewiłło-Jopek <kjopek@gmail.com>
Date:   2022-08-10T19:24:02.000Z

    14899 lib9p: Remove potential buffer overwrite in l9p_puqids()
    Reviewed by: Jason King <jason.brian.king+illumos@gmail.com>
    Reviewed by: Andy Fiddaman <illumos@fiddaman.net>
    Approved by: Garrett D'Amore <garrett@damore.org>

Actions

Also available in: Atom PDF