Skip to content

Fix the possible out-of-bound issue when reading file #14

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 6, 2022
Merged

Fix the possible out-of-bound issue when reading file #14

merged 1 commit into from
Nov 6, 2022

Conversation

Cycatz
Copy link
Contributor

@Cycatz Cycatz commented Nov 4, 2022 •
edited
Loading

When the size of file content greater than BLOCK_SIZE (4096), it's possible to access the fileContent string out of bounds.

For example, in my computer, the program /usr/bin/qemu-system-x86_64 has a VERY LONG command line name.

>>> cat /proc/887248/cmdline
/usr/bin/qemu-system-x86_64-nameguest=Playground,debug-threads=on-S-object{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain-1-Playground/master-key.aes"}-machinepc-q35-7.0,usb=off,vmport=off,dump-guest-core=off,memory-backend=pc.ram-accelkvm-cpuhost,migratable=on-m4096-object{"qom-type":"memory-backend-memfd","id":"pc.ram","share":true,"x-use-canonical-path-for-ramblock-id":false,"size":4294967296}-overcommitmem-lock=off-smp4,sockets=4,cores=1,threads=1-uuid17318c9f-8c88-4c3d-81bd-95e8e10b0193-no-user-config-nodefaults-chardevsocket,id=charmonitor,fd=33,server=on,wait=off-monchardev=charmonitor,id=monitor,mode=control-rtcbase=utc,driftfix=slew-globalkvm-pit.lost_tick_policy=delay-no-hpet-no-shutdown-globalICH9-LPC.disable_s3=1-globalICH9-LPC.disable_s4=1-bootstrict=on-device{"driver":"pcie-root-port","port":16,"chassis":1,"id":"pci.1","bus":"pcie.0","multifunction":true,"addr":"0x2"}-device{"driver":"pcie-root-port","port":17,"chassis":2,"id":"pci.2","bus":"pcie.0","addr":"0x2.0x1"}-device{"driver":"pcie-root-port","port":18,"chassis":3,"id":"pci.3","bus":"pcie.0","addr":"0x2.0x2"}-device{"driver":"pcie-root-port","port":19,"chassis":4,"id":"pci.4","bus":"pcie.0","addr":"0x2.0x3"}-device{"driver":"pcie-root-port","port":20,"chassis":5,"id":"pci.5","bus":"pcie.0","addr":"0x2.0x4"}-device{"driver":"pcie-root-port","port":21,"chassis":6,"id":"pci.6","bus":"pcie.0","addr":"0x2.0x5"}-device{"driver":"pcie-root-port","port":22,"chassis":7,"id":"pci.7","bus":"pcie.0","addr":"0x2.0x6"}-device{"driver":"pcie-root-port","port":23,"chassis":8,"id":"pci.8","bus":"pcie.0","addr":"0x2.0x7"}-device{"driver":"pcie-root-port","port":24,"chassis":9,"id":"pci.9","bus":"pcie.0","multifunction":true,"addr":"0x3"}-device{"driver":"pcie-root-port","port":25,"chassis":10,"id":"pci.10","bus":"pcie.0","addr":"0x3.0x1"}-device{"driver":"pcie-root-port","port":26,"chassis":11,"id":"pci.11","bus":"pcie.0","addr":"0x3.0x2"}-device{"driver":"pcie-root-port","port":27,"chassis":12,"id":"pci.12","bus":"pcie.0","addr":"0x3.0x3"}-device{"driver":"pcie-root-port","port":28,"chassis":13,"id":"pci.13","bus":"pcie.0","addr":"0x3.0x4"}-device{"driver":"pcie-root-port","port":29,"chassis":14,"id":"pci.14","bus":"pcie.0","addr":"0x3.0x5"}-device{"driver":"qemu-xhci","p2":15,"p3":15,"id":"usb","bus":"pci.2","addr":"0x0"}-device{"driver":"virtio-serial-pci","id":"virtio-serial0","bus":"pci.3","addr":"0x0"}-blockdev{"driver":"file","filename":"/var/lib/libvirt/images/Playground.qcow2","node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}-blockdev{"node-name":"libvirt-2-format","read-only":false,"discard":"unmap","driver":"qcow2","file":"libvirt-2-storage","backing":null}-device{"driver":"virtio-blk-pci","bus":"pci.4","addr":"0x0","drive":"libvirt-2-format","id":"virtio-disk0","bootindex":1}-device{"driver":"ide-cd","bus":"ide.0","id":"sata0-0-0"}-fsdevlocal,security_model=mapped,id=fsdev-fs0,path=/home/cycatz/VM-device{"driver":"virtio-9p-pci","id":"fs0","fsdev":"fsdev-fs0","mount_tag":"/myhome","bus":"pci.7","addr":"0x0"}-netdevtap,fd=34,vhost=on,vhostfd=36,id=hostnet0-device{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"52:54:00:66:6f:c4","bus":"pci.1","addr":"0x0"}-chardevpty,id=charserial0-device{"driver":"isa-serial","chardev":"charserial0","id":"serial0","index":0}-chardevsocket,id=charchannel0,fd=32,server=on,wait=off-device{"driver":"virtserialport","bus":"virtio-serial0.0","nr":1,"chardev":"charchannel0","id":"channel0","name":"org.qemu.guest_agent.0"}-chardevspicevmc,id=charchannel1,name=vdagent-device{"driver":"virtserialport","bus":"virtio-serial0.0","nr":2,"chardev":"charchannel1","id":"channel1","name":"com.redhat.spice.0"}-device{"driver":"usb-tablet","id":"input0","bus":"usb.0","port":"1"}-audiodev{"id":"audio1","driver":"spice"}-spiceport=5900,addr=127.0.0.1,disable-ticketing=on,image-compression=off,seamless-migration=on-device{"driver":"virtio-vga","id":"video0","max_outputs":1,"bus":"pcie.0","addr":"0x1"}-device{"driver":"ich9-intel-hda","id":"sound0","bus":"pcie.0","addr":"0x1b"}-device{"driver":"hda-duplex","id":"sound0-codec0","bus":"sound0.0","cad":0,"audiodev":"audio1"}-chardevspicevmc,id=charredir0,name=usbredir-device{"driver":"usb-redir","chardev":"charredir0","id":"redir0","bus":"usb.0","port":"2"}-chardevspicevmc,id=charredir1,name=usbredir-device{"driver":"usb-redir","chardev":"charredir1","id":"redir1","bus":"usb.0","port":"3"}-device{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.5","addr":"0x0"}-object{"qom-type":"rng-random","id":"objrng0","filename":"/dev/urandom"}-device{"driver":"virtio-rng-pci","rng":"objrng0","id":"rng0","bus":"pci.6","addr":"0x0"}-sandboxon,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny-msgtimestamp=on

And the length of the name is greater than BLOCK_SIZE, which is defined as 4096.

>>> cat /proc/887248/cmdline | wc
      0       1    4943

Thus the program will crash.

zsh: segmentation fault (core dumped)  ./zps

This commit fixes the issue by checking if the index variable i is less than BLOCK_SIZE - 1 and fill '\0' at the end of the string.

It also modifies the return value checking condition of read function to be > 0 rather then != 0, which terminates file reading when unexpected error occurs.

@Cycatz
Fix the possible out-of-bound issue when reading file
4a270fd 
When the size of file content greater than BLOCK_SIZE (4096),
    it's possible to access the fileContent string out of bounds.

This commit fixes the issue by checking if the index variable i
    is less than BLOCK_SIZE - 1 and fill '\0' at the end of the
    string. It also modifies the return value checking condition
    of read function to be '> 0' rather then '!= 0', which
    terminates file reading when unexpected error occurs.
orhun
orhun approved these changes Nov 6, 2022
View reviewed changes
Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sexy.

@orhun orhun merged commit 6668856 into orhun:master Nov 6, 2022
@orhun
Copy link
Owner

orhun commented Nov 6, 2022

Have any idea about this?

Screenshot 2022-11-06 at 20-25-29 zps - Codacy - Issues

https://app.codacy.com/gh/orhun/zps/issues

@Cycatz
Copy link
Contributor Author

Cycatz commented Nov 7, 2022 •
edited
Loading

Have any idea about this?

I guessed it happened because we do not check the return value of read when it > 0. But we only read one byte at a time, the return value is always 1. I think it doesn't introduce any security bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orhun orhun orhun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Cycatz @orhun